-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security Solution] Add rule snoozing on the rule editing page #155612
[Security Solution] Add rule snoozing on the rule editing page #155612
Conversation
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked and tested the PR locally. Rule snoozing on the editing page works as expected. There is just one issue with outgoing snooze status requests on the rules management page that needs to be addressed. Regardless, I approve in advance so the PR can be merged before the feature freeze.
const { | ||
data: rulesSnoozeSettings, | ||
isFetching: isSingleSnoozeSettingsFetching, | ||
isError: isSingleSnoozeSettingsError, | ||
} = useFetchRulesSnoozeSettings([id]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please pass { enabled: false }
to this hook when there's a table context available. Otherwise, it sends tens of snooze requests from the rules management page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++, in my case with 5 installed rules I'm getting 6 requests to /internal/alerting/rules/_find
on page load. So I guess it's N+1 in general: one for the whole page and one for each rule on the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem has been fixed by this commit 583c096
x-pack/plugins/security_solution/public/detection_engine/components/rule_snooze_badge/index.ts
Outdated
Show resolved
Hide resolved
...security_solution/public/detection_engine/components/rule_snooze_badge/rule_snooze_badge.tsx
Outdated
Show resolved
Hide resolved
const { | ||
data: rulesSnoozeSettings, | ||
isFetching: isSingleSnoozeSettingsFetching, | ||
isError: isSingleSnoozeSettingsError, | ||
} = useFetchRulesSnoozeSettings([id]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++, in my case with 5 installed rules I'm getting 6 requests to /internal/alerting/rules/_find
on page load. So I guess it's N+1 in general: one for the whole page and one for each rule on the page.
...curity_solution/public/detections/components/rules/step_rule_actions/rule_snooze_section.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/detections/components/rules/step_rule_actions/index.tsx
Outdated
Show resolved
Hide resolved
<EuiFlexItem grow={false}> | ||
<RuleSnoozeBadge id={id} showTooltipInline /> | ||
</EuiFlexItem> | ||
<EuiFlexItem> | ||
<EuiText size="s"> | ||
<strong>{i18n.SNOOZED_ACTIONS_WARNING}</strong> | ||
</EuiText> | ||
</EuiFlexItem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we show the Actions will not be preformed until it is unsnoozed
message unconditionally, i.e. regardless of whether the rule is snoozed or not. Since it's in bold it feels like a warning in the case where it doesn't really matter:
Can we hide it when the rule is not snoozed? If it's not trivial, can we make it look less dangerous by making the font regular and playing with the copy a little bit? E.g. If snoozed actions will not be triggered
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@banderror your comment is addressed by this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zoomed with @maximpn and went through the comments. Approving the PR in advance.
@@ -211,6 +212,15 @@ export const fetchRulesSnoozeSettings = async ({ | |||
} | |||
); | |||
|
|||
return response.data?.map((x) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try to avoid using variables named as x
, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed x
to snoozeSettings
.
@vitaliidm thank you for reviewing my PR 🙏
There is a PR #153219 to address z-index issues which solves the current problem as well (see the screenshot below). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alerts area LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @maximpn |
…age (#155850) **Addresses:** #147737 **Relates to:** #155612 ## Summary After merging #155612 back there is one issue is left unresolved described in this [comment](#155612 (comment)): > Looks like we show the `Actions will not be preformed until it is unsnoozed` message unconditionally, i.e. regardless of whether the rule is snoozed or not. Since it's in bold it feels like a warning in the case where it doesn't really matter: > Can we hide it when the rule is not snoozed? If it's not trivial, can we make it look less dangerous by making the font regular and playing with the copy a little bit? E.g. `If snoozed actions will not be triggered`. This PR resolves rule snooze description text issue. As snooze settings are resolved outside the security solution plugin having any logic to conditionally display a message will increase the complexity. This way the message was changes to avoid any text to appear conditionally. *Before:* <img width="703" alt="Screenshot 2023-04-24 at 18 36 31" src="https://user-images.githubusercontent.com/7359339/234060523-fe9161a1-0e83-4d39-a193-81c946d95106.png"> *After:* ![image](https://user-images.githubusercontent.com/3775283/236254424-533a6502-49ba-444e-87e5-9cda7e84c315.png) ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
…age (elastic#155850) **Addresses:** elastic#147737 **Relates to:** elastic#155612 ## Summary After merging elastic#155612 back there is one issue is left unresolved described in this [comment](elastic#155612 (comment)): > Looks like we show the `Actions will not be preformed until it is unsnoozed` message unconditionally, i.e. regardless of whether the rule is snoozed or not. Since it's in bold it feels like a warning in the case where it doesn't really matter: > Can we hide it when the rule is not snoozed? If it's not trivial, can we make it look less dangerous by making the font regular and playing with the copy a little bit? E.g. `If snoozed actions will not be triggered`. This PR resolves rule snooze description text issue. As snooze settings are resolved outside the security solution plugin having any logic to conditionally display a message will increase the complexity. This way the message was changes to avoid any text to appear conditionally. *Before:* <img width="703" alt="Screenshot 2023-04-24 at 18 36 31" src="https://user-images.githubusercontent.com/7359339/234060523-fe9161a1-0e83-4d39-a193-81c946d95106.png"> *After:* ![image](https://user-images.githubusercontent.com/3775283/236254424-533a6502-49ba-444e-87e5-9cda7e84c315.png) ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) (cherry picked from commit 6714d92)
…age (elastic#155850) **Addresses:** elastic#147737 **Relates to:** elastic#155612 ## Summary After merging elastic#155612 back there is one issue is left unresolved described in this [comment](elastic#155612 (comment)): > Looks like we show the `Actions will not be preformed until it is unsnoozed` message unconditionally, i.e. regardless of whether the rule is snoozed or not. Since it's in bold it feels like a warning in the case where it doesn't really matter: > Can we hide it when the rule is not snoozed? If it's not trivial, can we make it look less dangerous by making the font regular and playing with the copy a little bit? E.g. `If snoozed actions will not be triggered`. This PR resolves rule snooze description text issue. As snooze settings are resolved outside the security solution plugin having any logic to conditionally display a message will increase the complexity. This way the message was changes to avoid any text to appear conditionally. *Before:* <img width="703" alt="Screenshot 2023-04-24 at 18 36 31" src="https://user-images.githubusercontent.com/7359339/234060523-fe9161a1-0e83-4d39-a193-81c946d95106.png"> *After:* ![image](https://user-images.githubusercontent.com/3775283/236254424-533a6502-49ba-444e-87e5-9cda7e84c315.png) ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) (cherry picked from commit 6714d92)
…ting page (#155850) (#156888) # Backport This will backport the following commits from `main` to `8.8`: - [[Security Solution] Fix rule snooze description on the rule editing page (#155850)](#155850) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Maxim Palenov","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-05T16:14:41Z","message":"[Security Solution] Fix rule snooze description on the rule editing page (#155850)\n\n**Addresses:** https://github.com/elastic/kibana/issues/147737\r\n**Relates to:** https://github.com/elastic/kibana/pull/155612\r\n\r\n## Summary\r\n\r\nAfter merging #155612 back there is one issue is left unresolved described in this [comment](https://github.com/elastic/kibana/pull/155612#discussion_r1175545697):\r\n\r\n> Looks like we show the `Actions will not be preformed until it is unsnoozed` message unconditionally, i.e. regardless of whether the rule is snoozed or not. Since it's in bold it feels like a warning in the case where it doesn't really matter:\r\n\r\n> Can we hide it when the rule is not snoozed? If it's not trivial, can we make it look less dangerous by making the font regular and playing with the copy a little bit? E.g. `If snoozed actions will not be triggered`.\r\n\r\nThis PR resolves rule snooze description text issue. As snooze settings are resolved outside the security solution plugin having any logic to conditionally display a message will increase the complexity. This way the message was changes to avoid any text to appear conditionally.\r\n\r\n*Before:*\r\n\r\n<img width=\"703\" alt=\"Screenshot 2023-04-24 at 18 36 31\" src=\"https://user-images.githubusercontent.com/7359339/234060523-fe9161a1-0e83-4d39-a193-81c946d95106.png\">\r\n\r\n*After:*\r\n\r\n![image](https://user-images.githubusercontent.com/3775283/236254424-533a6502-49ba-444e-87e5-9cda7e84c315.png)\r\n\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)","sha":"6714d926ee041d61c627037189263905918303b3","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Detections and Resp","Team: SecuritySolution","Feature:Rule Management","Team:Detection Rules","ui-copy","v8.8.0","v8.9.0"],"number":155850,"url":"https://github.com/elastic/kibana/pull/155850","mergeCommit":{"message":"[Security Solution] Fix rule snooze description on the rule editing page (#155850)\n\n**Addresses:** https://github.com/elastic/kibana/issues/147737\r\n**Relates to:** https://github.com/elastic/kibana/pull/155612\r\n\r\n## Summary\r\n\r\nAfter merging #155612 back there is one issue is left unresolved described in this [comment](https://github.com/elastic/kibana/pull/155612#discussion_r1175545697):\r\n\r\n> Looks like we show the `Actions will not be preformed until it is unsnoozed` message unconditionally, i.e. regardless of whether the rule is snoozed or not. Since it's in bold it feels like a warning in the case where it doesn't really matter:\r\n\r\n> Can we hide it when the rule is not snoozed? If it's not trivial, can we make it look less dangerous by making the font regular and playing with the copy a little bit? E.g. `If snoozed actions will not be triggered`.\r\n\r\nThis PR resolves rule snooze description text issue. As snooze settings are resolved outside the security solution plugin having any logic to conditionally display a message will increase the complexity. This way the message was changes to avoid any text to appear conditionally.\r\n\r\n*Before:*\r\n\r\n<img width=\"703\" alt=\"Screenshot 2023-04-24 at 18 36 31\" src=\"https://user-images.githubusercontent.com/7359339/234060523-fe9161a1-0e83-4d39-a193-81c946d95106.png\">\r\n\r\n*After:*\r\n\r\n![image](https://user-images.githubusercontent.com/3775283/236254424-533a6502-49ba-444e-87e5-9cda7e84c315.png)\r\n\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)","sha":"6714d926ee041d61c627037189263905918303b3"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/155850","number":155850,"mergeCommit":{"message":"[Security Solution] Fix rule snooze description on the rule editing page (#155850)\n\n**Addresses:** https://github.com/elastic/kibana/issues/147737\r\n**Relates to:** https://github.com/elastic/kibana/pull/155612\r\n\r\n## Summary\r\n\r\nAfter merging #155612 back there is one issue is left unresolved described in this [comment](https://github.com/elastic/kibana/pull/155612#discussion_r1175545697):\r\n\r\n> Looks like we show the `Actions will not be preformed until it is unsnoozed` message unconditionally, i.e. regardless of whether the rule is snoozed or not. Since it's in bold it feels like a warning in the case where it doesn't really matter:\r\n\r\n> Can we hide it when the rule is not snoozed? If it's not trivial, can we make it look less dangerous by making the font regular and playing with the copy a little bit? E.g. `If snoozed actions will not be triggered`.\r\n\r\nThis PR resolves rule snooze description text issue. As snooze settings are resolved outside the security solution plugin having any logic to conditionally display a message will increase the complexity. This way the message was changes to avoid any text to appear conditionally.\r\n\r\n*Before:*\r\n\r\n<img width=\"703\" alt=\"Screenshot 2023-04-24 at 18 36 31\" src=\"https://user-images.githubusercontent.com/7359339/234060523-fe9161a1-0e83-4d39-a193-81c946d95106.png\">\r\n\r\n*After:*\r\n\r\n![image](https://user-images.githubusercontent.com/3775283/236254424-533a6502-49ba-444e-87e5-9cda7e84c315.png)\r\n\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)","sha":"6714d926ee041d61c627037189263905918303b3"}}]}] BACKPORT--> Co-authored-by: Maxim Palenov <[email protected]>
Addresses: #147737
Summary
This PR adds rule snooze feature on the Rule editing page.
Screen.Recording.2023-04-25.at.07.51.57.mov
Checklist