-
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
Added ability to fire actions when an alert instance is resolved #82799
Added ability to fire actions when an alert instance is resolved #82799
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
@@ -225,6 +226,14 @@ export class TaskRunner { | |||
alertInstance.hasScheduledActions() | |||
); | |||
const currentAlertInstanceIds = Object.keys(instancesWithScheduledActions); | |||
|
|||
scheduleActionsForResolvedInstances( |
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 believe this will send a notification for a resolved instance event if the alert instance is muted (or if all instances are muted). Is that the expected behavior?
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.
Correct, we shouldn't be scheduling actions if the instance or alert is throttled/muted. We may want to make that check check a reusable function somewhere, wherever it's done today, so other call sites (like this one), can use it. Either that, or figure out how to include these new actions to be scheduled in the same place they are done today - sorry I haven't looked at where these places might be yet, but can poke around a bit more if you want - LMK.
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.
Correct, we shouldn't be scheduling actions if the instance or alert is throttled/muted
I believe only muted
since throttle resets when action group changes?
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.
Great catch! By the way, I'm not sure about the reusable function here - we need to identify if the alert is muted to skip the whole scheduleActionsForResolvedInstances
call and passing down the list of mutedInstanceIds
to skip it in the schedule function as well. I think we already have this information on the current function level about the specific alert. Or we should get here the most fresh by calling alertsClient get by alertId?
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've tested it with the UI PR part using PagerDuty incidents and it works fine now with handling the mute
state. Will provide the api integration test as well
x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api.ts
Outdated
Show resolved
Hide resolved
…-action-group # Conflicts: # x-pack/plugins/alerts/server/task_runner/task_runner.ts
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.
Changes LGTM! Left a few nits.
One question I had is, should we prevent alert types from calling scheduleActions
with the resolved
action group?
x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts_base.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts_base.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts_base.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts_base.ts
Outdated
Show resolved
Hide resolved
…-action-group # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
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.
Most of the things I noticed Mike seems to have noted too, so nothing much to add beyond a couple of nits. :)
context: {}, | ||
state: {}, |
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.
Wondering if we're going to find the empty context and state to be a problem 🤔
Should we perhaps remove the values expected in context and stat from the template variables in the UI?
Otherwise users might try to use them in actions when attaching to Resolved, no?
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 did it in the UI PR
💚 Build SucceededMetrics [docs]Module Count
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
…stic#82799) * Added ability to fire actions when an alert instance is resolved * Fixed due to comments * Fixed merge issue * Fixed tests and added skip for muted resolve * added test for muted alert * Fixed due to comments * Fixed registry error message * Fixed jest test
) (#83381) * Added ability to fire actions when an alert instance is resolved * Fixed due to comments * Fixed merge issue * Fixed tests and added skip for muted resolve * added test for muted alert * Fixed due to comments * Fixed registry error message * Fixed jest test
* master: Migrate `/translations` route to core (elastic#83280) [APM] Ensure APM jest script can run (elastic#83398) [Uptime] Monitor status alert use url as instance (elastic#81736) [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259) TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964) [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139) Skips Vega test skip flaky suite (elastic#79967) [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020) Added ability to fire actions when an alert instance is resolved (elastic#82799) [ML] Adds functional tests for the index data visualizer card contents (elastic#83174)
… into add-logs-to-node-details * 'add-logs-to-node-details' of github.com:phillipb/kibana: fix tall vislib charts in visualize (elastic#83340) [Lens] Avoid unnecessary data fetching on dimension flyout open (elastic#82957) [Security Solution][Case] Change case connector minimum required license to basic (elastic#83401) fix logstash central pipeline management test (elastic#83281) [Search] Send to background UI (elastic#81793) Migrate `/translations` route to core (elastic#83280) [APM] Ensure APM jest script can run (elastic#83398) [Uptime] Monitor status alert use url as instance (elastic#81736) [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259) TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964) [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139) Skips Vega test skip flaky suite (elastic#79967) [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020) Added ability to fire actions when an alert instance is resolved (elastic#82799)
Resolve #49405