-
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
[Detection Engine] ML Rule Alert Suppression - Followup #188267
Changes from 8 commits
a5fcf4d
d5aa551
983945b
d2282c9
8b6c460
b96ad5d
824d760
9407230
f0bd4b9
02f20f8
e32f082
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -472,82 +472,69 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({ | |
indexPatternsFields: indexPattern.fields, | ||
}); | ||
|
||
/** Suppression fields being selected is a special case for our form logic, as we can't | ||
* disable these fields and leave users in a bad state that they cannot change. | ||
* The exception is threshold rules, which use an existing threshold field for the same | ||
* purpose and so are treated as if the field is always selected. */ | ||
const areSuppressionFieldsSelected = isThresholdRule || groupByFields.length > 0; | ||
|
||
const areSuppressionFieldsDisabledBySequence = | ||
isEqlRule(ruleType) && | ||
isEqlSequenceQuery(queryBar?.query?.query as string) && | ||
groupByFields.length === 0; | ||
|
||
const isSuppressionGroupByDisabled = | ||
/** If we don't have ML field information, users can't meaningfully interact with suppression fields */ | ||
const areSuppressionFieldsDisabledByMlFields = | ||
isMlRule(ruleType) && (noMlJobsStarted || mlFieldsLoading || !mlSuppressionFields.length); | ||
|
||
const isThresholdSuppressionDisabled = isThresholdRule && !enableThresholdSuppression; | ||
|
||
/** Suppression fields are generally disabled if either: | ||
* - License is insufficient (i.e. less than platinum) | ||
* - An EQL Sequence is used | ||
* - ML Field information is not available | ||
*/ | ||
const areSuppressionFieldsDisabled = | ||
!isAlertSuppressionLicenseValid || | ||
areSuppressionFieldsDisabledBySequence || | ||
isEsqlSuppressionLoading || | ||
(isMlRule(ruleType) && (noMlJobsStarted || mlFieldsLoading || !mlSuppressionFields.length)); | ||
areSuppressionFieldsDisabledByMlFields; | ||
|
||
const suppressionGroupByDisabledText = areSuppressionFieldsDisabledBySequence | ||
? i18n.EQL_SEQUENCE_SUPPRESSION_DISABLE_TOOLTIP | ||
: isMlRule(ruleType) && noMlJobsStarted | ||
? i18n.MACHINE_LEARNING_SUPPRESSION_DISABLED_LABEL | ||
: alertSuppressionUpsellingMessage; | ||
const isSuppressionGroupByDisabled = | ||
(areSuppressionFieldsDisabled || isEsqlSuppressionLoading) && !areSuppressionFieldsSelected; | ||
|
||
const suppressionGroupByDisabledText = useMemo(() => { | ||
if (areSuppressionFieldsDisabledBySequence) { | ||
return i18n.EQL_SEQUENCE_SUPPRESSION_DISABLE_TOOLTIP; | ||
} else if (isMlRule(ruleType) && noMlJobsStarted) { | ||
return i18n.MACHINE_LEARNING_SUPPRESSION_DISABLED_LABEL; | ||
} else { | ||
return alertSuppressionUpsellingMessage; | ||
} | ||
}, [ | ||
alertSuppressionUpsellingMessage, | ||
areSuppressionFieldsDisabledBySequence, | ||
noMlJobsStarted, | ||
ruleType, | ||
]); | ||
|
||
const suppressionGroupByFields = isEsqlRule(ruleType) | ||
? esqlSuppressionFields | ||
: isMlRule(ruleType) | ||
? mlSuppressionFields | ||
: termsAggregationFields; | ||
const suppressionGroupByFields = useMemo(() => { | ||
if (isEsqlRule(ruleType)) { | ||
return esqlSuppressionFields; | ||
} else if (isMlRule(ruleType)) { | ||
return mlSuppressionFields; | ||
} else { | ||
return termsAggregationFields; | ||
} | ||
}, [esqlSuppressionFields, mlSuppressionFields, ruleType, termsAggregationFields]); | ||
|
||
/** | ||
* Component that allows selection of suppression intervals disabled: | ||
* - if suppression license is not valid(i.e. less than platinum) | ||
* - or for not threshold rule - when groupBy fields not selected | ||
* - Eql sequence is used | ||
*/ | ||
const isGroupByChildrenDisabled = | ||
areSuppressionFieldsDisabledBySequence || !isAlertSuppressionLicenseValid || isThresholdRule | ||
? false | ||
: !groupByFields?.length; | ||
|
||
/** | ||
* Per rule execution radio option is disabled | ||
* - if suppression license is not valid(i.e. less than platinum) | ||
* - always disabled for threshold rule | ||
* - Eql sequence is used and suppression fields are in the default state | ||
*/ | ||
const isPerRuleExecutionDisabled = | ||
areSuppressionFieldsDisabledBySequence || !isAlertSuppressionLicenseValid || isThresholdRule; | ||
|
||
/** | ||
* Per time period execution radio option is disabled | ||
* - if suppression license is not valid(i.e. less than platinum) | ||
* - disabled for threshold rule when enabled suppression is not checked | ||
* - Eql sequence is used and suppression fields are in the default state | ||
*/ | ||
areSuppressionFieldsDisabled || isThresholdSuppressionDisabled || !areSuppressionFieldsSelected; | ||
const isPerRuleExecutionDisabled = areSuppressionFieldsDisabled || isThresholdRule; | ||
const isPerTimePeriodDisabled = | ||
areSuppressionFieldsDisabledBySequence || | ||
!isAlertSuppressionLicenseValid || | ||
(isThresholdRule && !enableThresholdSuppression); | ||
|
||
/** | ||
* Suppression duration is disabled when | ||
* - if suppression license is not valid(i.e. less than platinum) | ||
* - when suppression by rule execution is selected in radio button | ||
* - when threshold suppression is not enabled and no group by fields selected | ||
* - Eql sequence is used and suppression fields are in the default state | ||
* */ | ||
areSuppressionFieldsDisabled || isThresholdSuppressionDisabled || !areSuppressionFieldsSelected; | ||
const isDurationDisabled = | ||
vitaliidm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
areSuppressionFieldsDisabledBySequence || | ||
!isAlertSuppressionLicenseValid || | ||
(!enableThresholdSuppression && groupByFields?.length === 0); | ||
|
||
/** | ||
* Suppression missing fields is disabled when | ||
* - if suppression license is not valid(i.e. less than platinum) | ||
* - when no group by fields selected | ||
* - Eql sequence is used and suppression fields are in the default state | ||
* */ | ||
const isMissingFieldsDisabled = | ||
areSuppressionFieldsDisabledBySequence || | ||
!isAlertSuppressionLicenseValid || | ||
!groupByFields.length; | ||
areSuppressionFieldsDisabled || isThresholdSuppressionDisabled || !areSuppressionFieldsSelected; | ||
const isMissingFieldsDisabled = areSuppressionFieldsDisabled || !areSuppressionFieldsSelected; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Screen.Recording.2024-07-19.at.09.54.54.movI see some confusing behaviour here. a1 job is started 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spike in logon events is not installed, when selected with running job, no warning is displayed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vitaliidm did you notice that this behavior was specific to custom ML jobs? I can't reproduce that with just prebuilt ones.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Screen.Recording.2024-07-19.at.17.02.40.mov
Screen.Recording.2024-07-19.at.16.59.43.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.movAnd 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. |
||
const GroupByChildren = useCallback( | ||
({ groupByRadioSelection, groupByDurationUnit, groupByDurationValue }) => ( | ||
|
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling this out. I agree that the UX wasn't great, so I fixed it in 8b6c460.