[Detection Engine] ML Rule Alert Suppression - Followup #188267
Conversation
It would be painful to try to write a pure function that captures this same logic (see the deps lists), but as a compromise we can at least encapsulate these in functions and use more expressive implementation.
This was requested during review of elastic#181926, and I'm circling back to that now.
We did not previously extend our full disabling logic to all suppression fields. Now, when we determine that alert suppression is invalid/disabled, we disable all those relevant fields together. This commit accomplishes the above, and also factors out some shared boolean logic into the more general `areSuppressionFieldsDisabled` boolean, which is shared by most of these fields.
|
/ci |
rylnd
added
v8.15.0
backport:prev-minor
...rity_solution/public/detection_engine/rule_creation_ui/components/step_define_rule/index.tsx
Show resolved
Hide resolved
| /** | ||
| * If we don't have ML field information, users can't meaningfully interact with these fields */ | ||
| const areSuppressionFieldsDisabledByMlFields = | ||
| isMlRule(ruleType) && (noMlJobsStarted || mlFieldsLoading || !mlSuppressionFields.length); |
There was a problem hiding this comment.
I added two jobs (one running, one not). Selected field, configured suppression and then removed running job.
So, suppression config is disabled, but field actually was from a running job.
If I save rule, with "disabled" suppression and start job - suppression would work, but the field stored there would be from another job.
I can't clear this suppression on form , because when I removed job, it is still disabled
So, I have to add the same running job to clear it.
Similar can happen when user changes index, without adjusting suppression fields. But in this case, suppression controls are not disabled and user can easily change those fields
Here, after removing running job, controls become disabled. It requires user to add this job again to change fields or start another.
I am not sure, how critical it can be - just an observation on behaviour.
What do you think about it?
There was a problem hiding this comment.
Thanks for calling this out. I agree that the UX wasn't great, so I fixed it in 8b6c460.
…rrectly enabled When a non-threshold suppression field had been selected, the previous logic was causing the duration input to be enabled, because it was based on the non-threshold suppression input.
This commit abstracts a `areSuppressionFieldsSelected` boolean to capture the case where we can't disable fields due to the user needing to be able to change them. We then refactored the relevant predicates in terms of that new boolean.
Now that these are refactored in terms of more general predicates, it doesn't make much sense to enumerate all these situations for each variable, nor are these lists particularly helpful to understanding the code. So that we only have one source of truth to maintain moving forward (the code itself), I'm removing these "leaf" comments and instead annotating their parents.
|
@vitaliidm thanks for the feedback, your comments should now be addressed. Please take another look when you can! |
⏳ Build in-progress, with failures
Failed CI Steps
Test Failures
Historycc @rylnd |
|
@vitaliidm hold off on that review. As the latest build shows, I've still got some bugs 👀 |
Besides `areSuppressionFieldsSelected` not being defined as the comment documents, we had an issue where threshold suppression fields were not properly disabled in the default state (which also had some consequences for switching between rule types). This is now fixed, and IMO these predicates now read much better as well.
| !isAlertSuppressionLicenseValid || | ||
| !groupByFields.length; | ||
| areSuppressionFieldsDisabled || isThresholdSuppressionDisabled || !areSuppressionFieldsSelected; | ||
| const isMissingFieldsDisabled = areSuppressionFieldsDisabled || !areSuppressionFieldsSelected; | ||
|
|
There was a problem hiding this comment.
Screen.Recording.2024-07-19.at.09.54.54.mov
I see some confusing behaviour here.
a1 job is started
Spike in logon job is not installed
When only one selected everything seemed to work as expected.
When both of them - behaviour becomes unpredictable. When I removed a1 job, selected field still there, dropdown shows list of available fields(presumably from removed a1 job). No warnings that some of the job not started
When I deselect suppression fields - control becomes disabled with tooltip message
There was a problem hiding this comment.
Spike in logon events is not installed, when selected with running job, no warning is displayed
There was a problem hiding this comment.
@vitaliidm did you notice that this behavior was specific to custom ML jobs? I can't reproduce that with just prebuilt ones.
- Can you provide the steps you took to create that
a1job? - Can you reproduce this behavior in main, or just on this branch?
There was a problem hiding this comment.
- Job was created through wizard
Screen.Recording.2024-07-19.at.17.02.40.mov
- This is behaviour in main
Screen.Recording.2024-07-19.at.16.59.43.mov
There was a problem hiding this comment.
@vitaliidm this is certainly an interesting one that I hadn't encountered. I'm still not able to reproduce; after creating a custom job (and getting it in a persistent "started" state), I see the expected behavior:
Screen.Recording.2024-07-19.at.4.51.00.PM.mov
And you can see in the following screenshot that the custom job shows up with the rest of the jobs and is otherwise treated no differently (which was my previous theory on this behavior):
So perhaps there's something in your job's state that's different than what I'm seeing, or our environments are otherwise different? Let's pair on Monday and figure it out.
We continue to show a warning if not all of the selected jobs are running, but we no longer disable the UI based purley on whether none of the selected jobs are running; it's now based purely on whether we have fields to display.
I had previously used the concept of "some jobs started" because that is the case that we want to warn users about, and it was conceptually simpler than "not all jobs started." However, from both the perspective of the hook and the consumer, it makes more sense to return an "all jobs started" boolean and use that as necessary. This is logically equivalent to the previous commit, just nicer to read.
💚 Build Succeeded
Metrics [docs]Async chunks
To update your PR or re-run it, just comment with: cc @rylnd |
## Summary This PR is a followup to elastic#181926. It includes the following changes: - Refactoring some Rule Form logic with `useMemo` - Requested [in this discussion](elastic#181926 (comment)) - Addressed in a5fcf4d - Adds FTR tests validating ML Suppression supports alert enrichment - Requested [during previous review](elastic#181926 (comment)) - Addressed in d5aa551 - Disables ML Suppression fields as a group - Requested in [this comment](elastic#181926 (comment)) - Addressed by 983945b ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) (cherry picked from commit e2150de)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
|
Issue has been discovered: ML warning appears at other rule types |
… for non-ML rule types (#189003) Followup to #188267. A demo instance with these changes can be found [here](https://rylnd-pr-189003-fix-sticky-ml-warning.kbndev.co/app/security/rules/create). When not all ML jobs are started, we display a message conveying that the list of suppression fields might not be complete. In the case of a non-ML rule, this warning isn't helpful and should be hidden. It also only shows the warning if one more more ML jobs have been selected.
## Summary This PR is a followup to elastic#181926. It includes the following changes: - Refactoring some Rule Form logic with `useMemo` - Requested [in this discussion](elastic#181926 (comment)) - Addressed in a5fcf4d - Adds FTR tests validating ML Suppression supports alert enrichment - Requested [during previous review](elastic#181926 (comment)) - Addressed in d5aa551 - Disables ML Suppression fields as a group - Requested in [this comment](elastic#181926 (comment)) - Addressed by 983945b ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
… for non-ML rule types (elastic#189003) Followup to elastic#188267. A demo instance with these changes can be found [here](https://rylnd-pr-189003-fix-sticky-ml-warning.kbndev.co/app/security/rules/create). When not all ML jobs are started, we display a message conveying that the list of suppression fields might not be complete. In the case of a non-ML rule, this warning isn't helpful and should be hidden. It also only shows the warning if one more more ML jobs have been selected.
Summary
This PR is a followup to #181926. A demo instance can be found here. It includes the following changes:
useMemoChecklist