-
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
[RAC][Alerting][Security Solution] Adds Rule Execution UUID #113058
Conversation
Pinging @elastic/security-solution (Team: SecuritySolution) |
@@ -16,7 +16,7 @@ interface EqualCondition { | |||
equal: number; | |||
} | |||
|
|||
function isEqualConsition( | |||
function isEqualCondition( |
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.
❤️
// Verify unique executionId generated per `action:execute` grouping | ||
const eventExecutionIdSet = new Set(); | ||
events.forEach((event) => { | ||
if (event?.event?.action === 'execute') { | ||
eventExecutionIdSet.add(event?.kibana?.alert?.rule?.execution?.uuid); | ||
expect(eventExecutionIdSet.size).to.equal(1); | ||
eventExecutionIdSet.clear(); | ||
} else { | ||
eventExecutionIdSet.add(event?.kibana?.alert?.rule?.execution?.uuid); | ||
} | ||
}); | ||
|
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'll have to look closer at this - but I believe you are correct. But the assumption "ordering" with the notion of "after". This is all end up based on the timestamp, I believe (from the EL query), so there probably is some kind of race condition that if the rule runs again, before processing the events, that you'd have more of a mish-mash. Seems very unlikely though.
// Verify unique executionId generated per `action:execute` grouping | ||
const eventExecutionIdSet = new Set(); | ||
events.forEach((event) => { | ||
if (event?.event?.action === 'execute') { | ||
eventExecutionIdSet.add(event?.kibana?.alert?.rule?.execution?.uuid); | ||
expect(eventExecutionIdSet.size).to.equal(1); | ||
eventExecutionIdSet.clear(); | ||
} else { | ||
eventExecutionIdSet.add(event?.kibana?.alert?.rule?.execution?.uuid); | ||
} | ||
}); | ||
|
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.
That logic seems fine for now.
It feels like we should add another test case along side this one that validates that the same uuid appears on multiple alerts detected by the same rule execution
Not sure about that - doesn't it already do that? Or it should make sure that there's at least > 1 *-instance events?
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.
LGTM; there's some comments in x-pack/test/alerting_api_integration/spaces_only/tests/alerting/event_log_alerts.ts about adding some additional tests, which I think would be simple and nice to have (ensure different execution ids are used for different executions, and make sure all of the different *-instance event types have the execution id in them.
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.
Tested the changes locally. Created a rule, waited till it ran a couple of times, and checked that I could query execution events from the event log and correlate them to a particular execution. Excellent work, everything worked as expected 🚀 Thank you, @spong 👍
I think we should also log executionId
to default Kibana log along with the other rule execution information. Added a comment on that.
x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts
Outdated
Show resolved
Hide resolved
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.
infra
plugin changes LGTM
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @spong |
Summary
Resolves: #110135
This PR is for introducing a new UUID (
kibana.alert.rule.execution.uuid
as defined in the AAD schema) for identifying individual rule executions. This id is introduced as aprivate readonly
member of the alerting server task_manager, and plumbed through theexecutionHandler
and to all appropriate alert event and event-log touch points.For persistence when writing alerts within the RuleRegistry,
kibana.alert.rule.execution.uuid
is plumbed throughgetCommonAlertFields()
so it is grouped with like fields and is picked up by both thecreatePersistenceRuleTypeWrapper
used by Security Solution, andcreateLifecycleExecutor
used by Observability rules.Additionally on the Security Solution side,
kibana.alert.rule.execution.uuid
was plumbed through theRuleExecutionLog
so that all events written to the event-log will now include this id so individual rule status events/metrics can be correlated with specific rule executions.No UI facing changes were made, however
kibana.alert.rule.execution.uuid
is now available within the Alerts Table FieldBrowser, and can be toggled and viewed alongside alerts:As visible when exploring
event-log
in Discover:Checklist
Delete any items that are not applicable to this PR.