-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Adds max signals warning to UI propagated rule warnings #154112
Conversation
...tion/server/lib/detection_engine/rule_types/query/alert_suppression/group_and_bulk_create.ts
Outdated
Show resolved
Hide resolved
...rver/lib/detection_engine/rule_types/indicator_match/threat_mapping/create_threat_signals.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/ml/ml.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
e3e53f8
to
79943da
Compare
@elasticmachine merge upstream |
x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/ml/ml.ts
Outdated
Show resolved
Hide resolved
...ity_solution/server/lib/detection_engine/rule_types/new_terms/create_new_terms_alert_type.ts
Outdated
Show resolved
Hide resolved
...tion/server/lib/detection_engine/rule_types/query/alert_suppression/group_and_bulk_create.ts
Outdated
Show resolved
Hide resolved
...s/security_solution/server/lib/detection_engine/rule_types/utils/search_after_bulk_create.ts
Outdated
Show resolved
Hide resolved
Also is there a warning for EQL rules implemented? |
...s/security_solution/server/lib/detection_engine/rule_types/utils/search_after_bulk_create.ts
Outdated
Show resolved
Hide resolved
43208d8
to
dbee6f9
Compare
} else if (maxAlerts === 0) { | ||
return { createdAlerts: [], errors: {}, alertsWereTruncated: true }; |
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.
Short circuiting here if maxAlerts
is passed in as zero. We're using this function's deduping ability to check that: if max signals was hit, are any remaining alerts actual alerts or are they duplicates? If there are remaining alerts in filteredAlerts
at this point, we can assume they are actual alerts and don't need any additional function logic
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.
response ops changes lgtm
@dplumlee thanks again for looping docs in on the UI copy review. A few suggestions and questions for you:
|
973aa8f
to
345ea57
Compare
We can definitely change the language to be something more familiar but I think we want to avoid the specific numbers as the message can occur through different methods for different rule types. Sometimes we do have that hard alert count, sometimes we don't which is why I kept it more vague. I think the overall goal is to get the user to look more into their rule configuration to see what might be causing them to get so many alerts.
Yes but only through the API as listed here under
They are alerts that we have yet to analyze for a number of reasons but we see that they're there. We have this maximum count limit so we don't run a rule over endless alerts and bog down the system. They could be actual alerts, but often times, especially with certain rule types, we can't be certain they are. They could be alerts that match a value list exception that would be filtered out later, duplicate alerts that wouldn't be written in the end, etc. We just see them when searching but can't be sure if they're legitimate data hence the ambiguity of the statement. There's also not really a good immediate action we could recommend the user take as there are a number of reasons this could be happening to their rule. |
Thanks for the additional information, @dplumlee. Here are some options that don't have a hard number in them:
With all of these, I'd still recommend giving users some direction for resolving the error message and bringing the rule back from its partial failure state. It sounds like offering specific advice would be tricky since a number of things could be happening, but maybe we could recommend something that temporarily fixes the rule's state? Would it make sense to suggest that they reduce the number of open alerts or temporarily raise the max alert limit? For example:
Another option is to link to rule troubleshooting docs that suggest next steps while users are troubleshooting or fine-tuning the rule. For example:
|
@elasticmachine merge upstream |
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.
One fix in indicator match rules, otherwise it looks great!
if (results.warningMessages.includes(getMaxSignalsWarning())) { | ||
results.warningMessages = uniq(results.warningMessages); | ||
} else { | ||
results.warningMessages.push(getMaxSignalsWarning()); | ||
} |
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.
if (results.warningMessages.includes(getMaxSignalsWarning())) { | |
results.warningMessages = uniq(results.warningMessages); | |
} else { | |
results.warningMessages.push(getMaxSignalsWarning()); | |
} | |
if (results.warningMessages.includes(getMaxSignalsWarning())) { | |
results.warningMessages = uniq(results.warningMessages); | |
} else if (documentCount > 0) { | |
results.warningMessages.push(getMaxSignalsWarning()); | |
} |
d1f0815
to
c27a998
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @dplumlee |
Summary
Addresses #120678
Adds a warning message that is propagated to the user when a rule execution hits max signals. This will also set the rule in a partial failure state
Screenshots
Checklist
Delete any items that are not applicable to this PR.
For maintainers