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

[Metrics Alerts] Add support for search query and groupBy on alerts #59388

Merged
merged 12 commits into from
Mar 10, 2020
Merged

[Metrics Alerts] Add support for search query and groupBy on alerts #59388

merged 12 commits into from
Mar 10, 2020

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Mar 4, 2020

Summary

Closes #58290

  • Adds the ability to pass a filterQuery to an alert. This should be a Kuery converted to an Elasticsearch query, e.g. the JSON blob generated by the search bar on the Metrics Explorer
  • Adds support for passing a groupBy parameter to an alert. This will generate an alertInstance for every group found, and should dynamically add new instances if new groups are found after the alert is created.
  • Refactors the getAllCompositeData function so that it no longer needs to be passed a framework and requestContext, and updated the Snapshot library's use of it

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@Zacqary Zacqary added Feature:Alerting Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Mar 4, 2020
@Zacqary Zacqary requested a review from a team March 4, 2020 23:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@Zacqary Zacqary added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 4, 2020
@Zacqary
Copy link
Contributor Author

Zacqary commented Mar 4, 2020

@elastic/kibana-alerting-services This dynamically creates new alert instances but doesn't delete them. Would this create a potential problem? For example, if we have an alert grouped by agent.id on an infrastructure with extraordinarily high agent turnover, that starts and kills agents very frequently. We might have lingering alert instances corresponding to agent.ids that haven't reported data for months that build up over time.

Would that be an issue, or is the alerting system able to handle that?

@pmuellr
Copy link
Member

pmuellr commented Mar 5, 2020

Would that be an issue, or is the alerting system able to handle that?

We optimize on that, actually :-)

Currently, if your alert executor does not schedule actions on an alertInstance, the alertInstance "goes away". That could be a problem if you were expecting it to stick around, and we have an issue open to figure that part out. But we figured the best default was to let them get "garbage collected" over time, if you didn't schedule any actions on them.

So, if I understand correctly, you should be fine.

@phillipb phillipb self-requested a review March 5, 2020 18:50
phillipb
phillipb suggested changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

Looks good. A couple of tiny things.

...k/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts Outdated Show resolved Hide resolved
...k/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts Outdated Show resolved Hide resolved
phillipb
phillipb approved these changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@Zacqary Zacqary merged commit 5ad8337 into elastic:master Mar 10, 2020
@Zacqary Zacqary deleted the 58290-query-alerts branch March 10, 2020 15:46
@Zacqary Zacqary mentioned this pull request Mar 10, 2020
Merged
Zacqary added a commit to Zacqary/kibana that referenced this pull request Mar 10, 2020
@Zacqary
[Metrics Alerts] Add support for search query and groupBy on alerts (e…
8417b98 
…lastic#59388)

* Add filterQuery to metric alert params

* Add groupBy alert support

* Fix typings

* Fix malformed query

* Fix filterQuery merge

* Fix groupBy afterkey insertion, add group name to alert action

* Convert iife to getter

* Fix type check

* Fix type check again

* Remove unnecessary order param
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 10, 2020
@gmmorris
Merge branch 'master' into alerting/view-in-app
dd5ae27 
* master:
  Add a retry to dashboard test for a sometimes slow async operation (elastic#59600)
  [Endpoint] Sample data generator for endpoint app (elastic#58936)
  [Vis Editor] Refactoring metrics axes (elastic#59135)
  [DOCS] Changed Discover app to Discover (elastic#59769)
  [Metrics Alerts] Add support for search query and groupBy on alerts (elastic#59388)
  Enhancement - EUICodeEditor for Visualize JSON  (elastic#58679)
  [ML] Transforms: Data grid fixes. (elastic#59538)
  [SIEM] Fix and consolidate handling of error responses in the client (elastic#59438)
  [Maps] convert tooltip classes to typescript (elastic#59589)
  [ML] Functional tests - re-activate date_nanos test (elastic#59649)
  Move canvas to use NP Expressions service (elastic#58387)
  Update misc dependencies (elastic#59542)
  [Unit Testing] Configure react-testing-library queries to use Kibana's data-test-subj instead of default data-testid (elastic#59445)
  [Console] Remove unused code (elastic#59554)
  [Logs / Metrics UI] Link handling / stop page reloads (elastic#58478)
  Add SavedObject management section registration in core  (elastic#59291)
Zacqary added a commit that referenced this pull request Mar 10, 2020
@Zacqary
[Metrics Alerts] Add support for search query and groupBy on alerts (#…
6bb4d9d 
…59388) (#59773)

* Add filterQuery to metric alert params

* Add groupBy alert support

* Fix typings

* Fix malformed query

* Fix filterQuery merge

* Fix groupBy afterkey insertion, add group name to alert action

* Convert iife to getter

* Fix type check

* Fix type check again

* Remove unnecessary order param
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Mar 12, 2020
@Zacqary @jkelastic
[Metrics Alerts] Add support for search query and groupBy on alerts (e…
25e824a 
…lastic#59388)

* Add filterQuery to metric alert params

* Add groupBy alert support

* Fix typings

* Fix malformed query

* Fix filterQuery merge

* Fix groupBy afterkey insertion, add group name to alert action

* Convert iife to getter

* Fix type check

* Fix type check again

* Remove unnecessary order param
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phillipb phillipb phillipb approved these changes

Assignees
No one assigned
Labels
Feature:Alerting Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Metrics Alerts] Refactor query to allow for search queried and groupBy alerts
5 participants
@Zacqary @elasticmachine @pmuellr @kibanamachine @phillipb