[Response Ops][Alerting] Refactor ExecutionHandler stage 2#193807
[Response Ops][Alerting] Refactor ExecutionHandler stage 2#193807ymao1 merged 7 commits intoelastic:mainfrom
ExecutionHandler stage 2#193807Conversation
ymao1
force-pushed
the
refactor-execution-handler-stage-2
branch
from
a79dbed to
d3e831f
Compare
| delete logActions[r.id]; | ||
| const uuid = r.uuid; | ||
| if (uuid) { | ||
| actionsToNotLog.push(uuid); |
There was a problem hiding this comment.
Previously, we were creating a logActions record where the key was the actionId and the value was the information to pass to the event log in order to log the execute-action document. After adding some more unit tests in the PerAlertActionScheduler, I noticed that if we have multiple alerts for the same action or multiple actions that use the same connector, storing this information by the action ID would only allow one event log document to be written, so I switched to passing and using the action UUID instead.
There was a problem hiding this comment.
Not understanding why an action not having a uuid is treated differently here. Based on this, but mainly the assignment of actionsToLog below, it seems like we won't log an event log doc for these?
Wondering if we could create a placeholder uuid when the action doesn't have one, just for the action scheduling bits here, so we wouldn't have to worry about it potentially being empty. So I guess in at least ExecutionResponseItem and maybe ExecuteOptions, uuid wouldn't be optional.
There was a problem hiding this comment.
I created an issue about the optional typing and added comments about the optional typing here: 32beee0. I can add a placeholder UUID if you think that's better but I think that might cause more confusion in the future
There was a problem hiding this comment.
the comment is fine, no placeholder needed, thx!
ymao1
changed the title
ExecutionHandler stage 2
ymao1
added
release_note:skip
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
| export interface ExecuteOptions | ||
| extends Pick<ActionExecutorOptions, 'params' | 'source' | 'relatedSavedObjects' | 'consumer'> { | ||
| id: string; | ||
| uuid?: string; |
There was a problem hiding this comment.
Curious why this is optional. I think it's because for really old rules that haven't been updated since we added the uuid to the actions, it won't be there, and we don't have a migration. But double-checking in case it's BWC or something.
Just from a quick scan of the code, it doesn't seem like we added many tests in this PR that would have a non-existing uuid. Maybe some of the tests already deal with this, that I wouldn't have noticed?
I see one reference to a uuid!, which was just refactor/move from old code - but ... scares me :-). As I take a closer look at this PR, maybe I'll figure out it's ok.
There was a problem hiding this comment.
It is typed as optional even though it should always exist on the action :(. I think because we use a single schema in the API and the persistence layer and we don't expect the UUID to be passed into the API but we create one before it reaches the persistence layer.
There was a problem hiding this comment.
I found the PR where we added the uuid. For some reason I thought we hadn't done a migration to add to old rules, but we did! Add uuid to rule actions. My fear was we didn't, so an upgrade from an old version wouldn't have uuids, which got me wondering about the cases where you check it.
Seems like something we should fix. Open an issue? No need to fix in this PR (and may be nasty, presumably another type to deal with!).
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: cc @ymao1 |
|
@elasticmachine merge upstream |
| errorReason: `maxExecutableActions`, | ||
| logAlert: 4, | ||
| logAction: 3, | ||
| logAction: 5, |
There was a problem hiding this comment.
curious if this change indicates we were dropping some EL docs when we weren't using the UUIDs ...
There was a problem hiding this comment.
Yes, that's correct. If there were N actions configured in a rule with the same connector, then only one execute-action event log doc per alert was written instead of N
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]
History
cc @ymao1 |
|
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/11262993933 |
…#193807) Resolves elastic#186534 ## Summary This PR splits the for-loop in the `ActionScheduler.run` function into the appropriate scheduler classes. Previously, each scheduler had a `generateExecutables` function that would return an array of executables and the `ActionScheduler` would loop through the array and convert the executable to a scheduleable action depending on whether it was a per-alert action, summary action or system action. This refactor renames `generateExecutables` into `getActionsToSchedule` and moves the logic to convert the executables into a schedulable action into the appropriate scheduler class. ## To Verify Create some rules with per-alert and summary and system actions and verify they are triggered as expected. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> (cherry picked from commit 9221ab1)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…stage 2 (#193807) (#195676) # Backport This will backport the following commits from `main` to `8.x`: - [[Response Ops][Alerting] Refactor `ExecutionHandler` stage 2 (#193807)](#193807) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Ying Mao","email":"ying.mao@elastic.co"},"sourceCommit":{"committedDate":"2024-10-09T21:01:16Z","message":"[Response Ops][Alerting] Refactor `ExecutionHandler` stage 2 (#193807)\n\nResolves https://github.com/elastic/kibana/issues/186534\r\n\r\n## Summary\r\n\r\nThis PR splits the for-loop in the `ActionScheduler.run` function into\r\nthe appropriate scheduler classes. Previously, each scheduler had a\r\n`generateExecutables` function that would return an array of executables\r\nand the `ActionScheduler` would loop through the array and convert the\r\nexecutable to a scheduleable action depending on whether it was a\r\nper-alert action, summary action or system action. This refactor renames\r\n`generateExecutables` into `getActionsToSchedule` and moves the logic to\r\nconvert the executables into a schedulable action into the appropriate\r\nscheduler class.\r\n\r\n## To Verify\r\n\r\nCreate some rules with per-alert and summary and system actions and\r\nverify they are triggered as expected.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>","sha":"9221ab19e86ca7d3215205110fc709f7ba4739af","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Alerting","release_note:skip","Team:ResponseOps","v9.0.0","backport:prev-minor","v8.16.0"],"title":"[Response Ops][Alerting] Refactor `ExecutionHandler` stage 2","number":193807,"url":"https://github.com/elastic/kibana/pull/193807","mergeCommit":{"message":"[Response Ops][Alerting] Refactor `ExecutionHandler` stage 2 (#193807)\n\nResolves https://github.com/elastic/kibana/issues/186534\r\n\r\n## Summary\r\n\r\nThis PR splits the for-loop in the `ActionScheduler.run` function into\r\nthe appropriate scheduler classes. Previously, each scheduler had a\r\n`generateExecutables` function that would return an array of executables\r\nand the `ActionScheduler` would loop through the array and convert the\r\nexecutable to a scheduleable action depending on whether it was a\r\nper-alert action, summary action or system action. This refactor renames\r\n`generateExecutables` into `getActionsToSchedule` and moves the logic to\r\nconvert the executables into a schedulable action into the appropriate\r\nscheduler class.\r\n\r\n## To Verify\r\n\r\nCreate some rules with per-alert and summary and system actions and\r\nverify they are triggered as expected.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>","sha":"9221ab19e86ca7d3215205110fc709f7ba4739af"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193807","number":193807,"mergeCommit":{"message":"[Response Ops][Alerting] Refactor `ExecutionHandler` stage 2 (#193807)\n\nResolves https://github.com/elastic/kibana/issues/186534\r\n\r\n## Summary\r\n\r\nThis PR splits the for-loop in the `ActionScheduler.run` function into\r\nthe appropriate scheduler classes. Previously, each scheduler had a\r\n`generateExecutables` function that would return an array of executables\r\nand the `ActionScheduler` would loop through the array and convert the\r\nexecutable to a scheduleable action depending on whether it was a\r\nper-alert action, summary action or system action. This refactor renames\r\n`generateExecutables` into `getActionsToSchedule` and moves the logic to\r\nconvert the executables into a schedulable action into the appropriate\r\nscheduler class.\r\n\r\n## To Verify\r\n\r\nCreate some rules with per-alert and summary and system actions and\r\nverify they are triggered as expected.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>","sha":"9221ab19e86ca7d3215205110fc709f7ba4739af"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Ying Mao <ying.mao@elastic.co>
5 participants
Resolves #186534
Summary
This PR splits the for-loop in the
ActionScheduler.runfunction into the appropriate scheduler classes. Previously, each scheduler had agenerateExecutablesfunction that would return an array of executables and theActionSchedulerwould loop through the array and convert the executable to a scheduleable action depending on whether it was a per-alert action, summary action or system action. This refactor renamesgenerateExecutablesintogetActionsToScheduleand moves the logic to convert the executables into a schedulable action into the appropriate scheduler class.To Verify
Create some rules with per-alert and summary and system actions and verify they are triggered as expected.