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

[ResponseOps][Alerting] Rule run history displays success with a message when the rule status is warning #142645

Merged

Conversation

doakalexi
Copy link
Contributor

@doakalexi doakalexi commented Oct 4, 2022

Resolves #129120

Summary

Adds a new field to the event log kibana.alerting.outcome that will allow us to set the warning status.

Checklist

To verify

To verify that filtering still works correctly:

  • Run es using yarn es snapshot --license trial -E path.data=<data_path> on a different branch
  • Create a rule and let it run
  • Run es on this branch with the previous command
  • Verify that the rule run history status still shows up and that you can filter on it on the rule details page and in the log tab.

To verify that warning shows up:

  • Create a rule that will generate a warning status in the rule run history and verify that the status in the table matches the last response
  • I usually generate a 'warning status' by forcing it here x-pack/plugins/alerting/server/lib/rule_execution_status.ts

@doakalexi doakalexi changed the title Adding new field to the event log [ResponseOps][Alerting] Rule run history displays success with a message when the rule status is warning Oct 4, 2022
@doakalexi doakalexi marked this pull request as ready for review October 5, 2022 22:55
@doakalexi doakalexi requested review from a team as code owners October 5, 2022 22:55
@doakalexi doakalexi requested a review from xcrzx October 5, 2022 22:55
@doakalexi doakalexi added the Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) label Oct 5, 2022
@elasticmachine
Copy link
Contributor

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

@doakalexi doakalexi added release_note:fix Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework labels Oct 5, 2022
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.

Left some initial comments but overall looks good! I noticed that when I filter on the Warning outcome and then sort by the Active Alerts column, I get empty data:

With warning filter, no sort:
Screen Shot 2022-10-06 at 10 40 46 AM

With warning filter, sorted Active Alerts A-Z
Screen Shot 2022-10-06 at 10 41 10 AM

Also noticed that when I search for "reported" in the message, I get warning + success, even though I have the warning filter applied. Looks like it is doing an OR and not an AND? @JiaweiWu Is this a known issue?
Screen Shot 2022-10-06 at 10 42 38 AM

@JiaweiWu
Copy link
Contributor

JiaweiWu commented Oct 6, 2022

@ymao1 thanks for the heads up, it's not been reported AFAIK but I will take a look

edit: Looks like we forgot a parentheses, will create an issue (#142889)

i.e. the query is message: "test" OR error.message: "test" and event.outcome: failure but should be (message: "test" OR error.message: "test") and event.outcome: failure

Copy link
Contributor

@xcrzx xcrzx 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 locally the rule execution results table (the one on the detection rule details page). Everything works as expected.

@doakalexi
Copy link
Contributor Author

doakalexi commented Oct 10, 2022

Left some initial comments but overall looks good! I noticed that when I filter on the Warning outcome and then sort by the Active Alerts column, I get empty data:

@ymao1 This should be resolved in b4766c0

Copy link
Contributor

@ersin-erdal ersin-erdal 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 locally, works as expected.

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 unit tests but everything looks great and works as expected!

@doakalexi doakalexi enabled auto-merge (squash) October 12, 2022 14:04
@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 370 372 +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 672.6KB 672.6KB +28.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 38.7KB 38.7KB +10.0B
triggersActionsUi 99.1KB 99.3KB +212.0B
total +222.0B
Unknown metric groups

API count

id before after diff
alerting 379 381 +2

History

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

@doakalexi doakalexi merged commit 11d4922 into elastic:main Oct 12, 2022
@kibanamachine kibanamachine added v8.6.0 backport:skip This commit does not require backporting labels Oct 12, 2022
@doakalexi doakalexi deleted the alerting/rule-run-history-matches-status branch December 6, 2022 19:18
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 release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule run history displays success with a message when the rule status is warning
8 participants