[Security Solution][Detections] Rule Preview should process override fields and exceptions (#4680) #140221

e40pud · 2022-09-07T18:48:33Z

Summary

This PR restructures the Rule Preview functionality to allow better UX. Main changes are:

Rule Preview component extracted from the Define rule step into a separate Flyout component
override fields and exceptions are respected by rule preview
provide Rule Preview for pre-built rules

Main ticket #4680

Checklist

Delete any items that are not applicable to this PR.

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)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

…ns (elastic#4680)

elasticmachine · 2022-09-07T18:48:36Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

- Types - Unused translations - Unit tests

kibanamachine · 2022-09-09T15:10:48Z

merge conflict between base and head

e40pud · 2022-09-09T15:12:38Z

@elasticmachine merge upstream

e40pud · 2022-09-09T16:31:15Z

@elasticmachine merge upstream

jpdjere · 2022-09-09T17:49:00Z

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

@@ -68,29 +68,21 @@ TagContainer.displayName = 'TagContainer';

 const StepAboutRuleComponent: FC<StepAboutRuleProps> = ({
  addPadding = false,
-  defaultValues,
+  defaultValues: initialState,


One confusing behaviour of this step:

I tested the overrides locally and realized that two of the possible overridable fields (Severity override and Risk score override) are "normal" settings -not hidden-, while two others (Rule name override and Timestamp override) are part of the "Advanced settings" submenu, which the user has to unfold to see.

When testing updating these overrides and refreshing the preview, I realised there is a confusing difference between these two groups of overrides. The first two are applied to the Rule Preview as soon as you update them and click on "Refresh" in the Rule Preview flyout. On the contrary, if you update the "advanced" overrides, and click on "Refresh" the previewed alerts won't be updated. You first have to click on the "Continue" button of the "About rule" step, and only then click on "Refresh". Only then will the Rule Preview be updated.

Not exactly sure why this happens since they are all part of the same form, bu I think, considering all overrides live within the same step, that they should share the same behaviour.

Yeah, that is a good point. I need to add those few fields to be observed and thus the value to be used to affect Rule Preview immediately. Thanks for catching that!

jpdjere · 2022-09-09T18:13:31Z

...gins/security_solution/public/detections/components/rules/rule_preview/loading_histogram.tsx

-            </EuiText>
-          </>
-        </EuiFlexItem>
+        <EuiSpacer />


I realize this is not part of this PR but is the "loading" text (passed as subtitle prop to HeaderSection) when creating the Histogram intended? Looks a little weird to me, especially having the loading icon already there:

Yes it is intended and was like this since a long time (at least I see the same in 7.17.5 version). Though, we can ask what design team thinks about it and whether we need to update this behaviour. @yiyangliu9286 What do you think about this one?

e40pud · 2022-09-12T09:20:18Z

@elasticmachine merge upstream

…-ref HEAD~1..HEAD --fix'

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.tsx

...gins/security_solution/public/detections/components/rules/rule_preview/use_preview_route.tsx

michaelolo24 · 2022-09-12T16:49:34Z

...gins/security_solution/public/detections/components/rules/rule_preview/use_preview_route.tsx

    if (isRequestTriggered && rule === null) {
      setRule(
        formatPreviewRule({
-          index,


Nice work cleaning this up 👌🏾

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

michaelolo24 · 2022-09-12T17:36:17Z

Nice work with implementing this @e40pud! There were just a couple things I noticed doing some clickthrough testing. The rule preview seems to unmount when the window is resized? The other thing I saw was the data was retained when switching between rule types? It looks like a a refresh of a query takes place, but sometimes data is still shown without an actual associated query in the rule type selection view.

jpdjere

Thanks for the fix. 👍 from Detections rules

e40pud · 2022-09-13T09:42:26Z

@elasticmachine merge upstream

e40pud · 2022-09-13T10:32:06Z

Thanks @michaelolo24 for the feedback! I addressed your comments in code and here are couple more notes:

The rule preview seems to unmount when the window is resized?

Looks like the whole create rule page is being unmounted due to restricted space. I might need to fix that in a separate PR due to limited time before the FF. @peluja1012 what do you think?

Screen.Recording.2022-09-13.at.12.20.36.mov

The other thing I saw was the data was retained when switching between rule types? It looks like a a refresh of a query takes place, but sometimes data is still shown without an actual associated query in the rule type selection view.

We do not clear preview results until user manually refreshes them. Even when user switches between different rule types.

kibana-ci · 2022-09-13T10:57:55Z

💚 Build Succeeded

Buildkite Build
Commit: 6ca40e1

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3065	3066	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	6.4MB	6.4MB	-16.9KB

History

💚 Build #71240 succeeded 3f06c0b
💚 Build #71190 succeeded 615423b
💚 Build #71165 succeeded ae15c99
💚 Build #70945 succeeded 3256203
💔 Build #70901 failed d6afc3d

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

cc @e40pud

michaelolo24 · 2022-09-13T18:55:52Z

This issue is happening on main already so not associated with this particular PR. 👍🏾

Great, thanks for the updates and making the changes. I agree the unmounting should be handled in a separate PR. Everything looks good, only other thing I saw was that the histogram count didn't match what's actually in the table.

michaelolo24 · 2022-09-13T19:09:26Z

One thing to add with the unmounting-remounting is that when you switch the rule type the "refresh" in the rule preview might lead a user to think the query has been rerun with the new rule type, but it actually hasn't. It might be worth just completely unmounting the component if the user changes the rule? Even though I start in ML in the video below, I was originally on a threshold rule. When I got to custom query nothing changes in the panel, but it does a little reload. When I hit refresh it still shows the threshold results. The results are only updated when I close and re-open the panel

Screen.Recording.2022-09-13.at.3.04.29.PM.mov

michaelolo24

Thanks for the work on this @e40pud 👍🏾 ! We discussed this ticket offline and the mounting issues will be taken care of on a follow up PR.

e40pud · 2022-09-13T19:19:15Z

Thank you @michaelolo24! @peluja1012 could you please review those issues that Micheal found while testing? We need to decide whether those could be fixed in one of the upcoming PRs.

[Detections] Rule Preview should process override fields and exceptio…

ae0a3d6

…ns (elastic#4680)

e40pud self-assigned this Sep 7, 2022

e40pud requested review from a team as code owners September 7, 2022 18:48

e40pud requested a review from jpdjere September 7, 2022 18:48

e40pud added 2 commits September 8, 2022 15:34

CI fixes

d6750e8

- Types - Unused translations - Unit tests

Fix cypress tests

4117001

e40pud requested a review from a team as a code owner September 9, 2022 15:07

Merge branch 'main' into security/rules/4680-new-preview

b9e7a96

Merge branch 'main' into security/rules/4680-new-preview

d6afc3d

Merge branch 'main' into security/rules/4680-new-preview

3256203

jpdjere reviewed Sep 9, 2022

View reviewed changes

kibanamachine and others added 3 commits September 12, 2022 03:20

Merge branch 'main' into security/rules/4680-new-preview

ae15c99

Fix broken alerts table in fullscreen mode

615423b

Update rule configuration state on about step overrides chnges

512c349

e40pud requested a review from jpdjere September 12, 2022 12:22

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

3f06c0b

…-ref HEAD~1..HEAD --fix'

michaelolo24 reviewed Sep 12, 2022

View reviewed changes

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.tsx Outdated Show resolved Hide resolved

michaelolo24 reviewed Sep 12, 2022

View reviewed changes

...gins/security_solution/public/detections/components/rules/rule_preview/use_preview_route.tsx Outdated Show resolved Hide resolved

michaelolo24 reviewed Sep 12, 2022

View reviewed changes

x-pack/plugins/security_solution/public/detections/components/rules/rule_preview/index.tsx Outdated Show resolved Hide resolved

jpdjere approved these changes Sep 13, 2022

View reviewed changes

e40pud changed the title ~~[Detections] Rule Preview should process override fields and exceptions (#4680)~~ [Security Solution][Detections] Rule Preview should process override fields and exceptions (#4680) Sep 13, 2022

Review comments

62eb95d

Merge branch 'main' into security/rules/4680-new-preview

6ca40e1

e40pud requested a review from michaelolo24 September 13, 2022 10:13

e40pud requested a review from peluja1012 September 13, 2022 19:15

michaelolo24 approved these changes Sep 13, 2022

View reviewed changes

e40pud merged commit cbe7dc8 into elastic:main Sep 13, 2022

kibanamachine added v8.5.0 backport:skip This commit does not require backporting labels Sep 13, 2022

e40pud mentioned this pull request Sep 14, 2022

[Security Solution][Detections] Rule Preview: Refreshing does not work on override fields changes #140736

Merged

9 tasks

joepeeples mentioned this pull request Sep 19, 2022

[DOCS] Rule preview enhancements: side panel, exceptions, overrides, prebuilt rules elastic/security-docs#2469

Closed

This was referenced Sep 22, 2022

[Security Solution][Detections] Hoist Timestamp Override configuration to Rule Preview to more accurately represent preview results #124763

Closed

Preview Detections on Builtin Elastic Security Rules #109330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Rule Preview should process override fields and exceptions (#4680) #140221

[Security Solution][Detections] Rule Preview should process override fields and exceptions (#4680) #140221

e40pud commented Sep 7, 2022 •

edited

Loading

elasticmachine commented Sep 7, 2022

kibanamachine commented Sep 9, 2022

e40pud commented Sep 9, 2022

e40pud commented Sep 9, 2022

jpdjere Sep 9, 2022 •

edited

Loading

e40pud Sep 12, 2022

jpdjere Sep 9, 2022 •

edited

Loading

e40pud Sep 12, 2022

e40pud commented Sep 12, 2022

michaelolo24 Sep 12, 2022

michaelolo24 commented Sep 12, 2022

jpdjere left a comment

e40pud commented Sep 13, 2022

e40pud commented Sep 13, 2022

kibana-ci commented Sep 13, 2022

michaelolo24 commented Sep 13, 2022 •

edited

Loading

michaelolo24 commented Sep 13, 2022

michaelolo24 left a comment

e40pud commented Sep 13, 2022

[Security Solution][Detections] Rule Preview should process override fields and exceptions (#4680) #140221

[Security Solution][Detections] Rule Preview should process override fields and exceptions (#4680) #140221

Conversation

e40pud commented Sep 7, 2022 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Sep 7, 2022

kibanamachine commented Sep 9, 2022

e40pud commented Sep 9, 2022

e40pud commented Sep 9, 2022

jpdjere Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

e40pud Sep 12, 2022

Choose a reason for hiding this comment

jpdjere Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

e40pud Sep 12, 2022

Choose a reason for hiding this comment

e40pud commented Sep 12, 2022

michaelolo24 Sep 12, 2022

Choose a reason for hiding this comment

michaelolo24 commented Sep 12, 2022

jpdjere left a comment

Choose a reason for hiding this comment

e40pud commented Sep 13, 2022

e40pud commented Sep 13, 2022

kibana-ci commented Sep 13, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

michaelolo24 commented Sep 13, 2022 • edited Loading

michaelolo24 commented Sep 13, 2022

michaelolo24 left a comment

Choose a reason for hiding this comment

e40pud commented Sep 13, 2022

e40pud commented Sep 7, 2022 •

edited

Loading

jpdjere Sep 9, 2022 •

edited

Loading

jpdjere Sep 9, 2022 •

edited

Loading

michaelolo24 commented Sep 13, 2022 •

edited

Loading