[Security Solution][Alerts] resolves alerts suppression review feedback #155839

vitaliidm · 2023-04-26T08:29:29Z

Summary

addresses review feedback on [Security Solution][Alerts] adds suppression for missing fields options for security rules #155055
addresses UI changes from [Security Solution][Alerts][Discuss] Alert suppression on docs with missing fields #150101
- removes accordion in favour of intended suppression components
- adds popover with a link to documentation
- changes wording
addresses [Security Solution]Suppression Configuration showing for all Rule Types instead of just Custom Query #156247

Before

After

Intended Fields

Tooltip

…taliidm/kibana into alerts_8_8/suppression-feedback

vitaliidm · 2023-04-27T11:37:10Z

@elasticmachine merge upstream

…taliidm/kibana into alerts_8_8/suppression-feedback

elasticmachine · 2023-05-05T09:47:03Z

Pinging @elastic/security-solution (Team: SecuritySolution)

spong · 2023-05-05T20:13:59Z

...plugins/security_solution/public/detections/components/rules/suppression_info_icon/index.tsx

+
+  return (
+    <EuiPopover button={button} isOpen={isPopoverOpen} closePopover={closePopover}>
+      <EuiText style={{ width: POPOVER_WIDTH }}>


nit: Text size feels a bit big in the popover at 16px IMO, especially since the regular text below is at 14px. To be honest though, we're not consistent here with our rich text 'tooltips', so could go either way here.

Suppression 16px

Execution Log 14px

Integrations 14px

Rule Monitoring 16px

it makes sense to set text size to 14px in my case.
Thanks for the suggestion

spong · 2023-05-05T20:20:48Z

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

+          <IntendedRuleTypeEuiFormRow
+            $isVisible={isQueryRule(ruleType)}
+            data-test-subj="alertSuppressionMissingFields"
+            label={
+              <span>
+                {i18n.ALERT_SUPPRESSION_MISSING_FIELDS_FORM_ROW_LABEL} <SuppressionInfoIcon />
+              </span>
+            }
+          >
+            <UseMultiFields
+              fields={{
+                suppressionMissingFields: {
+                  path: 'suppressionMissingFields',
+                },
+              }}


nit: might be nice to add a little extra width here to prevent text wrapping and save on vertical space

current:

w/ extra width so no line-wrap

like this idea!
Implemented

spong · 2023-05-05T20:27:38Z

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

+          <RuleTypeEuiFormRow
+            $isVisible={isQueryRule(ruleType)}
+            data-test-subj="alertSuppressionInput"


ux nit: May want to consider hiding the suppression configuration UI until a field is selected, as it's in a disabled state until the user selects a field, which adds extra information to an already well populated form

current:

hidden w/o selection:

visible w/ selection:

We do something similar w/ the expanded Severity/Risk Score override UI:

Thanks for suggestions with different approaches on UI.

We, actually, considered each of them + accordion, to wrap all suppression settings, on Advanced Correlation Workgroup meetings with Product and UI teams.
#150101 (comment)
#150101 (comment)

In the end, we decided to stick to current implementation: just intend suppression settings fields on rules form.

spong · 2023-05-05T20:35:35Z

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

+const IntendedRuleTypeEuiFormRow = styled(RuleTypeEuiFormRow)`
+  ${({ theme }) => `padding-left: ${theme.eui.euiSizeXL};`}
+`;
+


I heard in passing that we're encouraged to use emotion over styled-components these days. I honestly prefer the style-components way you're using here by declaring a new semantic 'component', but figured I'd bring it up in case others know more or could point to relevant resources, as we currently have conflicting information in our dev docs here & here. 🤷‍♂️

tried first emotion, but it wasn't working. That's how I discovered issues and workaround related to CSS-in-JS, when it styled-components conflict with emotion, when speaking with eui team up.
I decided then to leave it as it was implemented, to not mix for one component both css-in-js and styled components

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

spong

Checked out, tested locally, code reviewed, and LGTM! 👍 🚀 🙌

Left a few ux nits, but not required -- pick and choose as you and the design folks see fit. Thanks for these enhancements @vitaliidm, appreciate it! 🙂

joepeeples · 2023-05-05T21:09:03Z

@gchaps Thanks for pinging about this. Looks like that help message is actually already a popover component and not a tooltip, so the message will be "sticky" and the user should be able to easily click the "Learn more" link. I confirmed this with a local build, and also confirmed that the docs link points to the correct page/heading.

As for any revisions to popover text, I think we'll go with what we have. The first line does echo the option name/title a bit, but with a complex feature like this I'm OK with a little repetition for the sake of additional explanation.

e40pud

LGTM!

dplumlee

UI and review changes look good to me, make sure to update the i18n string in x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/schema.tsx before merging

…rules/step_define_rule/schema.tsx Co-authored-by: Garrett Spong <spong@users.noreply.github.com>

vitaliidm · 2023-05-09T08:51:24Z

@gchaps Thanks for pinging about this. Looks like that help message is actually already a popover component and not a tooltip, so the message will be "sticky" and the user should be able to easily click the "Learn more" link. I confirmed this with a local build, and also confirmed that the docs link points to the correct page/heading.

Yes, component is popover, that allows clicking on link documentation

kibana-ci · 2023-05-09T10:57:47Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 344c901

Failed CI Steps

Security Solution Tests #3

Test Failures

[job] [logs] Security Solution Tests #3 / timeline flyout button the (+) button popover menu owns focus

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3866	3867	+1

Async chunks

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

id	before	after	diff
`lists`	157.7KB	157.8KB	+84.0B
`securitySolution`	9.1MB	9.1MB	+1.2KB
total			+1.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	363.7KB	363.8KB	+84.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	399	403	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	479	483	+4
total			+6

History

💔 Build #126474 failed 11ce622
💛 Build #125765 was flaky f0f6bf6
💔 Build #125736 failed 347c25a
💔 Build #125587 failed 91a5eea
💛 Build #125490 was flaky aefb724
💔 Build #124350 failed a8bb035

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

cc @vitaliidm

gchaps

doclink LGTM

…ck (elastic#155839) ## Summary - addresses review feedback on elastic#155055 - addresses UI changes from elastic#150101 - removes accordion in favour of intended suppression components - adds popover with a link to documentation - changes wording - addresses elastic#156247 ### Before <img width="1017" alt="Screenshot 2023-04-24 at 19 44 33" src="https://user-images.githubusercontent.com/92328789/234824612-b0ed2870-8aa0-44af-a37d-c061358c54a3.png"> ### After #### Intended Fields <img width="1016" alt="Screenshot 2023-05-05 at 10 42 46" src="https://user-images.githubusercontent.com/92328789/236426053-279d2f5b-46ea-434b-9cfa-696c71321661.png"> #### Tooltip <img width="1016" alt="Screenshot 2023-05-05 at 10 43 41" src="https://user-images.githubusercontent.com/92328789/236426061-1c39a5c2-63ca-4a36-b15e-2a1c1943481d.png"> --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Garrett Spong <spong@users.noreply.github.com> (cherry picked from commit 31b6062)

kibanamachine · 2023-05-09T16:53:23Z

💚 All backports created successfully

Status	Branch	Result
✅	8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…feedback (#155839) (#157192) # Backport This will backport the following commits from `main` to `8.8`: - [[Security Solution][Alerts] resolves alerts suppression review feedback (#155839)](#155839)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com> Co-authored-by: Pedro Jaramillo <pedro.jaramillo@elastic.co>

[Security Solution][Alerts] resolves suppression review feedback

ad3696f

vitaliidm mentioned this pull request Apr 26, 2023

[Security Solution][Alerts] adds suppression for missing fields options for security rules #155055

Merged

6 tasks

vitaliidm changed the title ~~[Security Solution][Alerts] resolves suppression review feedback~~ [Security Solution][Alerts] resolves alerts suppression review feedback Apr 26, 2023

vitaliidm and others added 8 commits April 26, 2023 09:31

Merge branch 'main' into alerts_8_8/suppression-feedback

295a2fa

Merge branch 'main' into alerts_8_8/suppression-feedback

2b6804c

Merge branch 'main' into alerts_8_8/suppression-feedback

44576e7

fix on creation

a19fe30

Merge branch 'alerts_8_8/suppression-feedback' of ssh://github.com/vi…

3e4714a

…taliidm/kibana into alerts_8_8/suppression-feedback

Merge branch 'main' into alerts_8_8/suppression-feedback

d9af903

remove accordion

c703a34

Merge branch 'main' into alerts_8_8/suppression-feedback

98ee755

Merge branch 'main' into alerts_8_8/suppression-feedback

28e5569

joepeeples mentioned this pull request Apr 27, 2023

Alert suppression options for missing suppression fields elastic/security-docs#3191

Closed

vitaliidm and others added 4 commits April 28, 2023 10:13

refactoring

4be4930

Merge branch 'alerts_8_8/suppression-feedback' of ssh://github.com/vi…

43d5042

…taliidm/kibana into alerts_8_8/suppression-feedback

add popover/change wording

a8bb035

Merge branch 'main' into alerts_8_8/suppression-feedback

aefb724

vitaliidm mentioned this pull request May 4, 2023

[Security Solution]Suppression Configuration showing for all Rule Types instead of just Custom Query #156247

Closed

vitaliidm and others added 3 commits May 4, 2023 17:18

change text

df09f5c

Update index.tsx

32c6ed5

fix typos

072defb

vitaliidm added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels May 4, 2023

text

91a5eea

vitaliidm requested a review from a team as a code owner May 5, 2023 09:47

vitaliidm requested a review from spong May 5, 2023 09:47

vitaliidm self-assigned this May 5, 2023

vitaliidm requested a review from marshallmain May 5, 2023 09:47

spong reviewed May 5, 2023

View reviewed changes

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

spong approved these changes May 5, 2023

View reviewed changes

e40pud approved these changes May 8, 2023

View reviewed changes

dplumlee approved these changes May 8, 2023

View reviewed changes

vitaliidm and others added 2 commits May 9, 2023 09:06

Update x-pack/plugins/security_solution/public/detections/components/…

11ce622

…rules/step_define_rule/schema.tsx Co-authored-by: Garrett Spong <spong@users.noreply.github.com>

fix typos

8cdbe88

vitaliidm and others added 3 commits May 9, 2023 09:58

add extra width

9be48da

text size

f368b35

Merge branch 'main' into alerts_8_8/suppression-feedback

344c901

vitaliidm enabled auto-merge (squash) May 9, 2023 15:55

gchaps approved these changes May 9, 2023

View reviewed changes

vitaliidm merged commit 31b6062 into elastic:main May 9, 2023

kibanamachine mentioned this pull request May 9, 2023

[8.8] [Security Solution][Alerts] resolves alerts suppression review feedback (#155839) #157192

Merged

vitaliidm mentioned this pull request May 15, 2023

[Security Solution][Alerts][Discuss] Alert suppression on docs with missing fields #150101

Closed

vitaliidm deleted the alerts_8_8/suppression-feedback branch March 4, 2024 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Alerts] resolves alerts suppression review feedback #155839

[Security Solution][Alerts] resolves alerts suppression review feedback #155839

vitaliidm commented Apr 26, 2023 •

edited by kibanamachine

Loading

vitaliidm commented Apr 27, 2023

elasticmachine commented May 5, 2023

spong May 5, 2023

vitaliidm May 9, 2023

spong May 5, 2023

vitaliidm May 9, 2023

spong May 5, 2023

vitaliidm May 9, 2023 •

edited

Loading

spong May 5, 2023

vitaliidm May 9, 2023

spong left a comment

joepeeples commented May 5, 2023 •

edited

Loading

e40pud left a comment

dplumlee left a comment

vitaliidm commented May 9, 2023

kibana-ci commented May 9, 2023

ESLint disabled line counts

Total ESLint disabled count

gchaps left a comment

kibanamachine commented May 9, 2023

[Security Solution][Alerts] resolves alerts suppression review feedback #155839

[Security Solution][Alerts] resolves alerts suppression review feedback #155839

Conversation

vitaliidm commented Apr 26, 2023 • edited by kibanamachine Loading

Summary

Before

After

Intended Fields

Tooltip

vitaliidm commented Apr 27, 2023

elasticmachine commented May 5, 2023

spong May 5, 2023

Choose a reason for hiding this comment

vitaliidm May 9, 2023

Choose a reason for hiding this comment

spong May 5, 2023

Choose a reason for hiding this comment

vitaliidm May 9, 2023

Choose a reason for hiding this comment

spong May 5, 2023

Choose a reason for hiding this comment

vitaliidm May 9, 2023 • edited Loading

Choose a reason for hiding this comment

spong May 5, 2023

Choose a reason for hiding this comment

vitaliidm May 9, 2023

Choose a reason for hiding this comment

spong left a comment

Choose a reason for hiding this comment

joepeeples commented May 5, 2023 • edited Loading

e40pud left a comment

Choose a reason for hiding this comment

dplumlee left a comment

Choose a reason for hiding this comment

vitaliidm commented May 9, 2023

kibana-ci commented May 9, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

gchaps left a comment

Choose a reason for hiding this comment

kibanamachine commented May 9, 2023

💚 All backports created successfully

Questions ?

vitaliidm commented Apr 26, 2023 •

edited by kibanamachine

Loading

vitaliidm May 9, 2023 •

edited

Loading

joepeeples commented May 5, 2023 •

edited

Loading