Skip to content
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

[Alerting] Skip writing alerts when rule execution times out #114518

Merged
merged 25 commits into from
Dec 3, 2021

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Oct 11, 2021

Resolves #113804

Summary

Followup to Alerting cancel implementation PR. In that PR, we added a cancelAlertsOnRuleTimeout flag that can be set at a kibana level (in kibana.yml) or on a rule type level, where, if the flag is set to true for both, the alerting task runner will not schedule actions for a rule that has exceeded its timeout value.

This PR extends that logic to alerts as data by passing in a shouldWriteAlerts service function into the rule executor that the rule registry executors (currently lifecycle and persistence) check before writing out alerts as data. In the future, I believe we can remove the need for this service function if/when we absorb the rule registry into the framework.

If, as a rule type producer, you want to always write out alerts as data, regardless of whether rule execution times out, you will need to set the cancelAlertsOnRuleTimeout flag on the rule type to false.

Checklist

@ymao1 ymao1 force-pushed the alerting/skip-writing-aad-on-timeout-2 branch from 423695a to 56fcff0 Compare October 11, 2021 17:45
@ymao1 ymao1 force-pushed the alerting/skip-writing-aad-on-timeout-2 branch from 56fcff0 to 7fac6a2 Compare October 11, 2021 19:01
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 249 251 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 762.7KB 763.0KB +312.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
alerting 36.9KB 36.9KB +20.0B
Unknown metric groups

API count

id before after diff
alerting 257 259 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ymao1 ymao1 changed the title Alerting/skip writing aad on timeout 2 [Alerting] Skip writing alerts when rule execution times out Nov 30, 2021
@ymao1 ymao1 self-assigned this Nov 30, 2021
@ymao1
Copy link
Contributor Author

ymao1 commented Nov 30, 2021

@elasticmachine merge upstream

@ymao1 ymao1 added the Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework label Nov 30, 2021
@ymao1 ymao1 added Feature:RAC label obsolete Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:skip Skip the PR/issue when compiling release notes v8.1.0 labels Nov 30, 2021
@ymao1 ymao1 marked this pull request as ready for review November 30, 2021 19:35
@ymao1 ymao1 requested review from a team as code owners November 30, 2021 19:35
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Nov 30, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ymao1
Copy link
Contributor Author

ymao1 commented Dec 3, 2021

@elasticmachine merge upstream

Copy link
Contributor

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Persistence rule type changes LGTM

@ymao1
Copy link
Contributor Author

ymao1 commented Dec 3, 2021

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 257 258 +1
Unknown metric groups

API count

id before after diff
alerting 265 266 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @ymao1

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APM changes LGTM

@ymao1 ymao1 merged commit b5895ec into elastic:main Dec 3, 2021
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 3, 2021
@ymao1 ymao1 deleted the alerting/skip-writing-aad-on-timeout-2 branch December 3, 2021 20:05
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
…#114518)

* Added cancel() to alerting task runner and writing event log document

* Updating rule saved object with timeout execution status

* Skip scheduling actions and logging event log for alerts if rule execution is cancelled

* Adding config for disabling skipping actions

* Fixing types

* Adding flag for rule types to opt out of skipping acitons

* Passing function into rule executor to determine whether to write AAD

* Using task runner uuid to differentiate between task instances

* Adding functional test

* Fixing types

* Updating service function name and adding logic to persistence rule type

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework Feature:RAC label obsolete release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RAC][Rule Registry] Rule registry executors skip writing AAD when execution is cancelled
8 participants