ML Rule Suppression UI Improvements #9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rylnd
changed the title
7 tasks
rylnd
force-pushed
the
ml_rule_suppression_warnings
branch
from
e59a189
to
5d0f0b3
Compare
* Disables suppression fields if no relevant ML jobs are running (as we cannot retrieve field info) * Adds a warning message if not all relevant ML jobs are running (as we may be missing some field info) Next step is testing this; we don't currently have a way to run ML rules in cypress, but I'm going to attempt to copy the logic in our FTR to accomplish this.
This was previously only available on the About step, via the useRuleIndices hook in combination with the useFetchIndex hook. Add new composite hook that encapsulates the same logic, and provides it to the define step. Unlike on the About step, we are currently only using this for ML fields as other situations derive their field list from a passed prop (which might be a performance optimization, or a bug, or both).
To do this we need to get the ML API to recognize our jobs as installed and running. They are currently _not_ recognizing this (although there are anomalies in the index). Still troubleshooting to see what's missing, here. This logic was cribbed from the analogous FTR tests, but those also aren't working so *shrug*.
rylnd
force-pushed
the
ml_rule_suppression_warnings
branch
from
5d0f0b3
to
4955831
Compare
Specifying the `groups` parameter when using the "setup module" API causes the corresponding jobs to be installed _with only the specified group_. This meant that in our FTR tests, we have been installing jobs with the `auditbeat` group. However, part of the contract between ML and Detection Engine is that we use the `group` parameter to determine relevance: if it doesn't belong to either the `security` or (legacy) `siem` group(s), it effectively does not exist to the Detection Engine. This fixes the (very confusing) issue of jobs being installed but not recognized, by specifying a recognized group id (and using our shared constant for it), both in the FTR and cypress utilities.
I have seen the _ecs prefix in a few places, but I'm not quite sure if it's actually part of official ML naming or not. Regardless, using the incorrect name caused the "start datafeed" request to fail with a "no datafeed for job ID" error.
The existing one also references the shared constants for our group IDs, so 👍.
* Cleans up debugging logs * Adds helper for ensuring that jobs are not started at beginning of suite * Fixes form filling utility to support single values for machine_learning_job_id * Updates suppression fields now that we're actually using real fields from anomaly indices
yctercero
reviewed
Jun 13, 2024
| @@ -22,7 +23,7 @@ export const executeSetupModuleRequest = async ({ | |||
| .set(getCommonRequestHeader('1')) | |||
| .send({ | |||
| prefix: '', | |||
| groups: ['auditbeat'], | |||
| groups: [ML_GROUP_ID], | |||
There was a problem hiding this comment.
Amazing job sticking with it to figure this out. Woohoo to finally having our ML tests able to run.
...lution/public/detection_engine/rule_creation_ui/components/step_define_rule/translations.tsx
Outdated
Show resolved
Hide resolved
...lution/public/detection_engine/rule_creation_ui/components/step_define_rule/translations.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Nastasha Solomon <79124755+nastasha-solomon@users.noreply.github.com>
Co-authored-by: Nastasha Solomon <79124755+nastasha-solomon@users.noreply.github.com>
The way the cypress helper that consumes these is written, both of these forms work, but we're never going to encounter a rule with the display name params, and knowing the display name but not the ID is not useful for investigative purposes. I can see how this might have been done to prevent needing to change these jobs as their IDs change, but I think it's more likely that those will change than their IDs.
There's not a lot here, but I feel bad for adding anything to step_define_rule so this is an attempt to minimize that. In the course of refactoring I also caught a bug (perhaps just a test environment one) where the form fields are temporarily `undefined` when the hooks are run. I updated the form type to reflect this; hopefully that doesn't have broader impact (but if it does, those are probably also uncaught bugs).
The combination of shared state and retry logic means that asserting exactly 1 rule exists will never work if rule creation succeeds in a previous step. If we instead assert that there is _at least_ the expected number of rules, we have a chance of the retry working.
* Stop datafeeds before creating rule * Simplify jobId logic
Turns out the reason the "Job IDs" were persisted as human-readable text was so that they could be reused for assertions. I still think these should be separate, so I'm adding them back for this specific assertion.
* Adds necessary setup/teardown for ML integration
1. Use "proper" combobox text, and capture it within a helper method
I swear I saw this working when I was doing the same stuff for the ML
Job picker, but I must have only been dealing with one item, or the
items I was selecting were somehow different. Downarrow is _required_ on
the first option (a simple "enter" will select nothing), but using
downarrow on subsequent options will cause the _second_ suggested item
to be selected. E.g. if I type "by_field_value", it suggests both
"by_field_value" and "client.by_field_value," and {downarrow}{enter}
would cause the latter to be selected.
2. I also ensure that our new ML validations have run (which causes
suppression fields to be disabled) before attempting to interact with
the suppression fields, as this was causing some flakiness now that
these checks are done async
3. I also fixed the broken `clearAlertSuppressionFields` task, which had
never work but also had never been exercised since the relevant test
was skipped.
There were no less than four assertions in this test that relied on there being no other rules present in the environment, but nothing was being done to ensure that was the case. I can't imagine why these were skipped!
I want to run these in the flaky runner to get a sense of how/where they're still failing, for now.
We were over-eagerly disabling these fields when the ML checks were not relevant.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Re-enables and adds additional ML cypress tests
Adds ML fields to Define Step
Disables suppression UI when no relevant ML jobs are enabled
Adds warning text when some relevant ML jobs are not enabled
