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

[Response Ops][Alerting] Optimize alerting task runner for persistent (non-lifecycle rule types) #149043

Merged
merged 11 commits into from
Jan 23, 2023

Conversation

doakalexi
Copy link
Contributor

@doakalexi doakalexi commented Jan 17, 2023

Resolves #148573

Summary

To help prepare for the framework to handle persistent (non-lifecycle rule types) that do not need the auto-recovery functionality performed by the framework, we added a flag for the rule type so they can opt in or out

Checklist

To verify

  • Create a rule and verify that it is working as expected.
  • Set autoRecoverAlerts: true for a rule type and create a rule using that rule type. Verify that the rule does not recover.

@doakalexi doakalexi changed the title Updating task runner [Response Ops][Alerting] Optimize alerting task runner for persistent (non-lifecycle rule types) Jan 17, 2023
@doakalexi doakalexi added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:fix Feature:Alerting labels Jan 17, 2023
@doakalexi doakalexi marked this pull request as ready for review January 18, 2023 16:34
@doakalexi doakalexi requested review from a team as code owners January 18, 2023 16:34
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Can we add a functional test for a rule type that sets autoRecoverAlerts to false?

@doakalexi doakalexi removed the request for review from a team January 19, 2023 17:17
@doakalexi
Copy link
Contributor Author

Can we add a functional test for a rule type that sets autoRecoverAlerts to false?

Resolved in this commit 2a2be5b

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM! Left one comment about maybe checking for autoRecover in a different spot.

@@ -140,6 +142,7 @@ export function createAlertFactory<
previouslyRecoveredAlerts: {},
hasReachedAlertLimit,
alertLimit: maxAlerts,
autoRecoverAlerts,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this again, we could probably short circuit this logic even more by adding a check before processAlerts:

if (!autoRecoverAlerts) {
  logger.debug(`Set autoRecoverAlerts to true on rule type to get access to recovered alerts.`);
  return [];
}

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can do that!

Copy link
Contributor Author

@doakalexi doakalexi Jan 23, 2023

Choose a reason for hiding this comment

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

Resolved in this commit 5306a12

@doakalexi doakalexi enabled auto-merge (squash) January 23, 2023 14:41
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id before after diff
alerting 465 466 +1

History

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

@doakalexi doakalexi merged commit 164f629 into elastic:main Jan 23, 2023
@kibanamachine kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Jan 23, 2023
This pull request was closed.
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 release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Response Ops][Alerting] Optimize alerting task runner for persistent (non-lifecycle rule types)
5 participants