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.

Sign up for GitHub

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

Jump to bottom

Onboard "Custom Threshold" rule type with FAAD #179284

Merged
merged 8 commits into from
Mar 26, 2024
Merged

Onboard "Custom Threshold" rule type with FAAD #179284

merged 8 commits into from
Mar 26, 2024

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Mar 22, 2024 •
edited
Loading

Towards: #169867

This PR onboards "Custom Threshold" rule type with FAAD.

To verify

Create a Custom Threshold rule by using a test index and DW. Set the Role visibility metrics.
When the rule runs, it generates an alert and saves it under .internal.alerts-observability.threshold.alerts-default.
The alert should be visible on Observability > alerts page as well.

@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 22, 2024
@ersin-erdal ersin-erdal self-assigned this Mar 22, 2024
@ersin-erdal
Copy link
Contributor Author

/ci

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

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

@botelastic botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Mar 25, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

pmuellr
pmuellr approved these changes Mar 25, 2024
View reviewed changes
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; alert doc looks good in the expected index; was able to cause an alert to recover and see it updated in the alert index

@maryam-saeidi maryam-saeidi self-requested a review March 26, 2024 07:46
maryam-saeidi
maryam-saeidi approved these changes Mar 26, 2024
View reviewed changes
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.

Tested locally and worked as expected! 💪🏻

Added one question about tests and a nit.

...bility_solution/observability/server/lib/rules/custom_threshold/custom_threshold_executor.ts Outdated
@@ -58,24 +57,37 @@ export interface CustomThresholdLocators {
logsExplorerLocator?: LocatorPublic<LogsExplorerLocatorParams>;
}

export type CustomThresholdAlert = Omit<
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe we can move this type to the types.ts file in this directory.

...y_solution/observability/server/lib/rules/custom_threshold/custom_threshold_executor.test.ts Outdated
@@ -184,63 +271,63 @@ describe('The custom threshold alert type', () => {
test('alerts as expected with the > comparator', async () => {
setResults(Comparator.GT, [0.75], true);
await execute(Comparator.GT, [0.75]);
expect(mostRecentAction(instanceID)).toBeAlertAction();
expect(getLastReportedAlert(instanceID)).toBeAlertAction();
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit confused now, is this checking an alert or action? It seems we are getting an alert (getLastReportedAlert) but checking an action (toBeAlertAction).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old implementation causes this confusion.
We used to create an alert and then schedule actions for each actionGroups (alert, warning, recovered etc.)
Now we just report an alert with action group, no separate action scheduling.

Maybe we can rename toBeAlertAction to toHaveAlertAction?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rename toBeAlertAction to toHaveAlertAction?

I like this suggestion 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
observability 2 0 -2

History

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

cc @ersin-erdal

@ersin-erdal ersin-erdal merged commit ef0f0e0 into elastic:main Mar 26, 2024
17 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 26, 2024
@ersin-erdal ersin-erdal deleted the 169867-custom-threshold branch March 26, 2024 15:20
@ersin-erdal ersin-erdal mentioned this pull request Mar 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maryam-saeidi maryam-saeidi maryam-saeidi approved these changes

@pmuellr pmuellr pmuellr approved these changes

@umbopepato umbopepato Awaiting requested review from umbopepato

Assignees

@ersin-erdal ersin-erdal

Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.14.0
Projects
No open projects
AppEx: ResponseOps - Execution & Connectors
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ersin-erdal @elasticmachine @kibana-ci @pmuellr @maryam-saeidi @kibanamachine