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

Onboard "SLO burn rate" rule type with FAAD #178922

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Mar 18, 2024

Towards: #169867

This PR onboards "SLO burn rate" rule type with FAAD.

To verify

Create an SLO by using a test index (create a dataview for it), use very low budget consumed %
The rule bound to the SLO should create an alert and save it under .internal.alerts-observability.slo.alerts-default-000001

@ersin-erdal ersin-erdal added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.14.0 labels Mar 18, 2024
@ersin-erdal ersin-erdal self-assigned this Mar 18, 2024
@ersin-erdal ersin-erdal force-pushed the 169867-slo-burn-rate-faad branch from 9a57685 to a241edc Compare March 18, 2024 23:06
@ersin-erdal
Copy link
Contributor Author

/ci

# Conflicts:
#	x-pack/plugins/observability_solution/observability/server/lib/rules/register_rule_types.ts
#	x-pack/plugins/observability_solution/slo/server/lib/rules/slo_burn_rate/executor.ts
@ersin-erdal
Copy link
Contributor Author

/ci

@ersin-erdal ersin-erdal marked this pull request as ready for review March 19, 2024 21:44
@ersin-erdal ersin-erdal requested a review from a team as a code owner March 19, 2024 21:44
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM; ran test suggested in PR top comment, saw documents written to
.internal.alerts-observability.slo.alerts-default-000001 as expected

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #75 / discover/group2 discover data grid row height should allow to change row height

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
slo 4 3 -1

History

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

cc @ersin-erdal

@maryam-saeidi maryam-saeidi self-requested a review March 26, 2024 08:33
Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

LGTM! Tested firing an alert and recover it + sending saving action variables in an index connector and worked as expected.

@@ -256,7 +238,8 @@ describe('BurnRateRuleExecutor', () => {
getTimeRange,
});

expect(alertWithLifecycleMock).not.toBeCalled();
expect(servicesMock.alertsClient?.report).not.toBeCalled();
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, what is the difference between servicesMock.alertsClient! and servicesMock.alertsClient?? Is there any difference in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No difference indeed, alertsClient! could be used everywhere as we are sure that it exists.

fields: {
actionGroup: windowDef.actionGroup,
state: {
alertState: AlertStates.ALERT,
Copy link
Member

Choose a reason for hiding this comment

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

Is this instead of alert.replaceState({ alertState: AlertStates.ALERT });? What was the reason to have replaceState previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had different methods to set different fields in the alert such as state and context, but realised that they have never been used separately, so combined them in .report

@ersin-erdal ersin-erdal merged commit 5a14d2e into elastic:main Mar 26, 2024
20 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 26, 2024
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 release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.14.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

6 participants