[Security Solution][Detections] - Fix remaining render and validation bug with query preview + tests #80110

yctercero · 2020-10-10T20:55:45Z

Summary

This PR is meant to address the remaining rule query preview bugs. It addresses the following:

gives same loading experience for all rule query types (previously there were two different loading states)
makes sure noise warnings are showing for all query types and disappear on timeframe or query change
updates when to/from are set so it is set on preview click

Custom query

Threshold query

Eql query

Eql sequence query

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

yctercero · 2020-10-10T20:56:58Z

x-pack/plugins/security_solution/public/common/components/matrix_histogram/index.tsx

Removed these changes as I'm no longer using this component.

x-pack/plugins/security_solution/public/common/containers/matrix_histogram/index.ts

...plugins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.tsx

rylnd

A few quick fixes should get the validity logic a little tighter, here, especially around EQL rules. Otherwise, this looks great!

...plugins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.tsx

x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx

x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx

…tegy

kibanamachine · 2020-10-13T05:55:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 34bd856

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`securitySolution`	2057	2059	+2

async chunks size

id	before	after	diff
`securitySolution`	10.6MB	10.6MB	+10.4KB

page load bundle size

id	before	after	diff
`securitySolution`	593.2KB	600.5KB	+7.2KB

History

💚 Build #81170 succeeded bf088c0
💚 Build #81131 succeeded 59d57fd3e2ada128abaa4f3c0380aa04669616e1
💚 Build #81119 succeeded ec1d63476b0fbd2397a904c6422c8f7dbd10d4bf
💚 Build #81081 succeeded 151e47c591f3bb70dfa16633205aafa8a8aa7f5c
💚 Build #80969 succeeded fbadd78a509b98c3a2fb16af21ea0767d2499acc

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

yctercero · 2020-10-13T16:35:15Z

@MikePaquette @marrasherrier getting feedback on the sequence histogram, that it's not very intuitive and not sure that it's of any added value. @madirey suggested maybe embedding resolver as it is able to visually represent event sequences, or maybe we could just show total hits when we detect that the query is a sequence?

rylnd · 2020-10-13T16:49:33Z

@yctercero I was able to somewhat reliably reproduce an issue where hitting Preview did not update the UI:

(re)Load the Create Rule page
Choose EQL type
Fill a sequence query: sequence [process where true] [process where true]
Click "Preview Results"

Often, the preview request would be issued, but no UI would update/no histogram would be displayed.

rylnd

This looks great! I'm really liking the rxjs stuff, I can't wait to play more with observables 🤗 .

I had one (product) question about the sequences preview histogram, and another issue with the preview button occasionally not working.

rylnd · 2020-10-13T15:56:13Z

x-pack/plugins/security_solution/public/common/hooks/eql/use_eql_preview.ts

+              abortSignal: abortCtrl.current.signal,
+            }
+          )
+          .pipe(takeUntil(unsubscribeStream.current))


I really like this pattern! The semantics of emitting to this to unsubscribe isn't exactly intuitive (to me), but it seems like a common solution to this scoping/unsubscribing issue. There are also some eslint hooks for rxjs (e.g. no-unsafe-takeuntil) that we might want to enable

rylnd · 2020-10-13T16:08:35Z

x-pack/plugins/security_solution/public/common/hooks/eql/use_eql_preview.ts

+                if (!didCancel.current) {
+                  setLoading(false);
+                  if (hasEqlSequenceQuery(query)) {
+                    setResponse(getSequenceAggs(res, refetch.current));


I think getSequenceAggs is the source of these console warnings:

Since the x-values are time strings and not numbers. However, wrapping them in moment().valueOf() fixes the warning but breaks the "buckets", since the x-axis is now back to using a linear scale:

Taking a step back, the existing histogram is somewhat surprising in its behavior. There appear to be only two buckets in this graph:

Or perhaps each unique timestamp string is its own bucket? I'm not sure what the default behavior for their ordinal scale is, but the latter seems likely.

To me the graph is conveying three data points: the number of results, the number of sequences, and their distribution over time. However, distribution over time is ambiguous and potentially lossy/misleading; we may want to reconsider how we present this data for sequences.

Could we just preview this in an embedded Resolver?

I'm not sure what you have against rainbow graphs @rylnd 😜 I have a draft where I'm trying to see if displaying it in more of a Gantt type form helps. Will definitely follow up with @MikePaquette and @marrasherrier on this.

@madirey I wonder how heavy that would be, or is it already easily embeddable?

madirey

Tested manually with several different EQL queries. Aside from the pending questions about the histogram, this LGTM. Thank you!

yctercero · 2020-10-13T22:19:52Z

Merging, with the following issues being addressed as follow up:

the graph sometimes not showing as pointed out by @rylnd is a race condition where some query bar state is updating after button is clicked and this sets showHistogram to true - need to memoize a few values to minimize unnecessary re-renders, that will fix this
continue discussion with team about best display for sequences. Working on a Gantt looking type graph, that could be useful, if not at the very least of more use than what's there now

… bug with query preview + tests (elastic#80110) ## Summary This PR is meant to address the remaining rule query preview bugs. It addresses the following: - gives same loading experience for all rule query types (previously there were two different loading states) - makes sure noise warnings are showing for all query types and disappear on timeframe or query change - updates when to/from are set so it is set on preview click

… bug with query preview + tests (#80110) (#80424) ## Summary This PR is meant to address the remaining rule query preview bugs. It addresses the following: - gives same loading experience for all rule query types (previously there were two different loading states) - makes sure noise warnings are showing for all query types and disappear on timeframe or query change - updates when to/from are set so it is set on preview click

… bug with query preview + tests (#80110) (#80425) ## Summary This PR is meant to address the remaining rule query preview bugs. It addresses the following: - gives same loading experience for all rule query types (previously there were two different loading states) - makes sure noise warnings are showing for all query types and disappear on timeframe or query change - updates when to/from are set so it is set on preview click

yctercero added v8.0.0 v7.10.0 v7.11.0 labels Oct 10, 2020

yctercero self-assigned this Oct 10, 2020

yctercero commented Oct 10, 2020

View reviewed changes

x-pack/plugins/security_solution/public/common/containers/matrix_histogram/index.ts Outdated Show resolved Hide resolved

yctercero commented Oct 10, 2020

View reviewed changes

...plugins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.tsx Outdated Show resolved Hide resolved

yctercero added release_note:fix Team:Detections and Resp Security Detection Response Team labels Oct 10, 2020

yctercero mentioned this pull request Oct 10, 2020

[Security Solution][Detections] - Fix query preview bugs + tests #79947

Closed

7 tasks

yctercero marked this pull request as ready for review October 12, 2020 16:34

yctercero requested review from a team as code owners October 12, 2020 16:34

rylnd requested changes Oct 12, 2020

View reviewed changes

yctercero added 13 commits October 12, 2020 14:11

typed buildquerfilter to remove some icky casting

a3c5aff

removed unnecessary types

c1890f8

fixed subscription bug with way i had written call to eql search stra…

5ada8eb

…tegy

moved to use adError toast to include error details

5cd8a3b

fixed subscribe

cceca3c

remove infamous someAriaId

b722d9d

remove something

637265a

fixes remaining preview query bugs and adds tests for all components

9f80ffb

fix type issues

f85d98b

fixed warnings not dissapearing on query change

de4835e

updated preview eql hook to use ame to from logi as matrix hook

2ee78fc

set loading back to false if error occurs on matric histogram

b167ecb

apply feedback changes

e914beb

yctercero force-pushed the preview_buggies_2 branch from 59d57fd to e914beb Compare October 12, 2020 18:24

yctercero added 2 commits October 12, 2020 14:33

updated validity check for non eql query bar

42a87fc

had to invert validity logic for non eql query bar

bf088c0

rylnd self-requested a review October 12, 2020 20:22

fixing eql preview subsciber

34bd856

rylnd reviewed Oct 13, 2020

View reviewed changes

madirey approved these changes Oct 13, 2020

View reviewed changes

yctercero requested a review from rylnd October 13, 2020 22:14

rylnd approved these changes Oct 13, 2020

View reviewed changes

yctercero merged commit f8d821c into elastic:master Oct 13, 2020

yctercero mentioned this pull request Oct 13, 2020

[7.x] [Security Solution][Detections] - Fix remaining render and validation bug with query preview + tests (#80110) #80424

Merged

yctercero mentioned this pull request Oct 13, 2020

[7.10] [Security Solution][Detections] - Fix remaining render and validation bug with query preview + tests (#80110) #80425

Merged

yctercero deleted the preview_buggies_2 branch October 14, 2020 11:58

[Security Solution][Detections] - Fix remaining render and validation bug with query preview + tests #80110

[Security Solution][Detections] - Fix remaining render and validation bug with query preview + tests #80110

Uh oh!

Conversation

yctercero commented Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Custom query

Threshold query

Eql query

Eql sequence query

Checklist

Uh oh!

yctercero Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Oct 13, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

Uh oh!

yctercero commented Oct 13, 2020

Uh oh!

rylnd commented Oct 13, 2020

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

rylnd Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

rylnd Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

rylnd Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

madirey Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

yctercero Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

madirey left a comment

Choose a reason for hiding this comment

Uh oh!

yctercero commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yctercero commented Oct 10, 2020 •

edited

Loading