[Metrics Alerts] Create Metric Threshold Alert Type and Executor #57606

Zacqary · 2020-02-13T18:45:26Z

Summary

This is a merge and update of some of the components of #47165

Adds an alert type and executor function for metric threshold alerts. Only supports the Fired action group (not Recovered), as multiple action groups won't be available until 7.8.

The new Alerting APIs support renotify intervals, retrieving current alert states, and other features that were previously missing in the POC, so those are now removed. The executor function is now a relatively simple check of the current metric value.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~[ ] Documentation was added for features that require explanation or tutorials~~
~~[ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~[ ] This was checked for keyboard-only and screenreader accessibility~~
~~[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-13T18:45:29Z

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

Zacqary · 2020-02-13T19:06:05Z

@elasticmachine merge upstream

…nto 56427-metric-alert-type

pmuellr

Did a quick review after @mikecote pointed this PR to me.

Looks good so far, made some comments, obviously hit us up for any questions.

Two relevant PRs ...

Alerting plugin migrate to Kibana platform - PR Alerting plugin migrate to Kibana platform #57635
[alerting] initial index threshold alertType and supporting APIs - PR [alerting] initial index threshold alertType and supporting APIs #57030

The first is the alerting plugin migration from legacy to new platform. Your plugin setup will change a bit. This will likely get merged to master by next week (if not sooner).

The second is the initial step in our first alertType that we will provide built-in with the alerting plugin. It's fairly similar to this one. I've been expecting that other consumers of the alerting plugin will have similar kinds of alertTypes, perhaps over time we can share some implementation code. For now, you might want to take a peek to get some additional ideas, like providing TONS of context properties when scheduling actions. :-)

pmuellr · 2020-02-14T19:17:20Z

...y/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts

+        indexPattern: schema.string(),
+      }),
+    },
+    actionGroups: [FIRED_ACTIONS],


The actionGroups parameter is currently defined as a string[], so I'm surprised this type checks, since FIRED_ACTIONS is an object and not a string. Perhaps I'm missing something here tho.

This failed to typecheck before because, according to the docs, actionGroups is typed as Array<{id:string, name:string}>

scheduleActions is still expecting a string though which is why this one's failing:
https://github.com/elastic/kibana/pull/57606/checks?check_run_id=446684113

Need to fix that. I guess scheduleActions is expecting FIRED_ACTIONS.id? Might want to clear up this ambiguity

woops, yeah, sorry! We did change the types on actionGroups last week or so, I forgot about that. scheduleActions() should just take the id value, and not the {id, name} structure. The {id, name} structure lets us associate an i18n "name" for a particular id, but that name is not used during the execution of alerts, only the id.

I've opened up issue #57853 to clean up the README. Please feel free to add more to it!

pmuellr · 2020-02-14T19:17:23Z

...y/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts

+  });
+
+  const { buckets } = result.aggregations.aggregatedIntervals;
+  const { value } = buckets[buckets.length - 1].aggregatedValue;


Since you're only using the last bucket, the date histogram doesn't seem to be that useful here. But this function could be repurposed to deal with a "history" of what the alert would have done with old data, eg, for usage in a preview UI. We're doing something similar with the not-yet-merged built-in alerting index threshold alertType.

I'm still an ES query newbie, what approach would you recommend for getting this data without using histogram buckets?

I believe you can use a simple "metrics" aggregation, which doesn't do any bucketing, eg https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-metrics-avg-aggregation.html . You've already got the set of documents to use to generate the metric value you want via the filter, and you don't need those documents further bucketed. That's what I'd try, but I'd also have someone else review it, as it wouldn't surprise me if there was something even easier - because I am also an ES query n00b :-). I actually kinda wonder if an esSQL query would be the easiest thing here (and thinking about it a little for the index threshold as well).

pmuellr · 2020-02-14T19:23:40Z

...y/plugins/infra/server/lib/alerting/metric_threshold/register_metric_threshold_alert_type.ts

+        });
+      }
+
+      alertInstance.replaceState({


You don't seem to be using this alertInstance state being stored at this point, anywhere else (at least in the alert), so you technically don't need this. Perhaps it's for a future use ...

I figured that we'd need this for a UI that displays alerts and their current states/historical states. I know that's not implemented yet but I assume it'll use the api/alert/<id>/state API? If it's still up in the air I can remove this and we'll handle that later.

The alertInstance state is typed as a Record<string, unknown>, and as such, the alerting plugin itself is unlikely to expose any generic UI that could display it. A consumer-specific plugin certainly could. I think it's fine to leave this in, perhaps add a comment that this could be retrieved by a metrics plugin to provide alert-instance specific data in a UI. ie, "future use"

Zacqary · 2020-02-14T20:58:51Z

@pmuellr From a quick glance at the index threshold type it looks like we can definitely extend that, or at least import common stuff such as comparators. I'll give it a more thorough review, thanks!

simianhacker · 2020-02-18T16:11:04Z

For the checks, instead of creating a query (with aggregations) and sending it directly to Elasticsearch, I would prefer we try and use the same backend API's for the UI. That way the number we display is exactly the same number we check against. That might require us to refactor our API's since alerts comes with it's own "search context" and that will need to be passed in some how.

With that in mind... How does the Alerts system work with Spaces? This is important because if a user creates an alert in a custom space where they've modified the index pattern in our settings, we need to know to honor that pattern.

…t-type # Conflicts: # x-pack/legacy/plugins/infra/index.ts # x-pack/legacy/plugins/infra/server/lib/metrics/get_groupings.ts # x-pack/legacy/plugins/infra/server/routes/metrics_explorer/lib/get_groupings.ts # x-pack/plugins/infra/server/routes/metrics_explorer/lib/get_groupings.ts

Zacqary · 2020-02-19T19:11:31Z

Update from Zoom talk with @simianhacker and @phillipb: We want to explore using getTSVBData for this alert function so that we know it's alerting on the same data that the Metrics Explorer receives. This will require hacking a requestContext to send from the executor function (which doesn't use requests), as the underlying get_vis_data is still relying on some legacy platform stuff. We'll write an integration test to notify us if the underlying function changes and breaks our hack.

If this ends up being too fragile or complicated we can use the Snapshot API instead, or just stick with our own ES query if that's also too difficult.

Zacqary · 2020-02-19T19:28:13Z

Another update: We're going to test and make sure this PR works as-is and open up a second issue to switch the engine for getting the current metric value.

Zacqary · 2020-02-20T20:32:59Z

@elasticmachine merge upstream

…nto 56427-metric-alert-type

Zacqary · 2020-02-26T17:09:20Z

@elasticmachine merge upstream

…structure alert

phillipb

LGTM!

kibanamachine · 2020-02-26T22:49:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7990079

History

💔 Build #29201 failed a88d0cf
💚 Build #28178 succeeded ae0bd9d
💚 Build #27997 succeeded 4264930
💚 Build #27767 succeeded 58b6489
💚 Build #27473 succeeded d4693d5

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

simianhacker

LGTM (code wise)

Are you planning on change the aggregation in a follow up PR to support rate?

Zacqary · 2020-02-27T16:47:48Z

@simianhacker Tracking rate in #58621

…stic#57606) * [Infra] Add basic backend for metric threshold alerts * Define separate fired/recovered action groups * Allow alerting on arbitrary search fields besides host.name * Add list and delete endpoioints * Add groupBy alerts * Remove extraneous routes and SavedObject logic * Remove additional SavedObject code * Remove renotify logic from executor * Fix action group type * Fix scheduledActions typecheck * Fix i18n * Migrate alerting to new platform * Add alerting to infra dependencies * Add comment about future use * Adjust alert params tm names to sync with UI; default to Entire Infrastructure alert * Add support for between comparator Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kibanamachine · 2020-02-28T16:48:47Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

) (#58742) * [Infra] Add basic backend for metric threshold alerts * Define separate fired/recovered action groups * Allow alerting on arbitrary search fields besides host.name * Add list and delete endpoioints * Add groupBy alerts * Remove extraneous routes and SavedObject logic * Remove additional SavedObject code * Remove renotify logic from executor * Fix action group type * Fix scheduledActions typecheck * Fix i18n * Migrate alerting to new platform * Add alerting to infra dependencies * Add comment about future use * Adjust alert params tm names to sync with UI; default to Entire Infrastructure alert * Add support for between comparator Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Zacqary added 10 commits October 2, 2019 16:25

[Infra] Add basic backend for metric threshold alerts

f33df05

Merge remote-tracking branch 'upstream/master' into 46511-alerting-poc

96ef0bd

Define separate fired/recovered action groups

4e30bb6

Allow alerting on arbitrary search fields besides host.name

cb27ef3

Add list and delete endpoioints

d64d0df

Add groupBy alerts

5eb056a

Merge in Alerting POC

0de90d8

Remove extraneous routes and SavedObject logic

1a55e7f

Remove additional SavedObject code

78bc666

Remove renotify logic from executor

0fe4c0c

Zacqary added release_note:enhancement Feature:Alerting Feature:Metrics UI Metrics UI feature v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.7.0 labels Feb 13, 2020

Zacqary requested a review from a team February 13, 2020 18:45

Zacqary marked this pull request as ready for review February 13, 2020 18:46

elasticmachine and others added 3 commits February 13, 2020 13:06

Merge branch 'master' into 56427-metric-alert-type

30babc2

Fix action group type

65de9eb

Merge branch '56427-metric-alert-type' of github.com:Zacqary/kibana i…

6384f23

…nto 56427-metric-alert-type

pmuellr reviewed Feb 14, 2020

View reviewed changes

Fix scheduledActions typecheck

24d6fa6

Fix i18n

44082e1

pmuellr mentioned this pull request Feb 18, 2020

[alerting] update README #57853

Closed

simianhacker requested review from phillipb and simianhacker February 18, 2020 15:58

Zacqary added 2 commits February 18, 2020 13:55

Migrate alerting to new platform

d4693d5

Add alerting to infra dependencies

58b6489

Merge branch 'master' into 56427-metric-alert-type

4264930

Zacqary mentioned this pull request Feb 20, 2020

[Metrics Alerts] Use TSVB data for threshold alerting instead of bespoke ES query #58171

Closed

Zacqary added 2 commits February 21, 2020 10:34

Add comment about future use

09ac117

Merge branch '56427-metric-alert-type' of github.com:Zacqary/kibana i…

ae0bd9d

…nto 56427-metric-alert-type

Merge branch 'master' into 56427-metric-alert-type

a88d0cf

Zacqary mentioned this pull request Feb 26, 2020

[Metrics Alerts] Fix alerting on a rate aggregation #58621

Closed

Zacqary added 2 commits February 26, 2020 14:52

Adjust alert params tm names to sync with UI; default to Entire Infra…

54441b5

…structure alert

Add support for between comparator

7990079

phillipb approved these changes Feb 26, 2020

View reviewed changes

Zacqary mentioned this pull request Feb 26, 2020

[Metric Alerts] Add backend support for multiple expressions per alert #58672

Merged

2 tasks

simianhacker approved these changes Feb 27, 2020

View reviewed changes

Zacqary merged commit 390165b into elastic:master Feb 27, 2020

Zacqary mentioned this pull request Feb 27, 2020

[7.x] [Metrics Alerts] Create Metric Threshold Alert Type and Executor (#57606) #58742

Merged

Zacqary deleted the 56427-metric-alert-type branch February 27, 2020 17:57

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 28, 2020

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Metrics Alerts] Create Metric Threshold Alert Type and Executor #57606

[Metrics Alerts] Create Metric Threshold Alert Type and Executor #57606

Zacqary commented Feb 13, 2020

elasticmachine commented Feb 13, 2020

Zacqary commented Feb 13, 2020

pmuellr left a comment

pmuellr Feb 14, 2020

Zacqary Feb 14, 2020

pmuellr Feb 18, 2020

pmuellr Feb 14, 2020

Zacqary Feb 14, 2020

pmuellr Feb 18, 2020

pmuellr Feb 14, 2020

Zacqary Feb 14, 2020

pmuellr Feb 18, 2020

Zacqary commented Feb 14, 2020

simianhacker commented Feb 18, 2020

Zacqary commented Feb 19, 2020

Zacqary commented Feb 19, 2020

Zacqary commented Feb 20, 2020

Zacqary commented Feb 26, 2020

phillipb left a comment

kibanamachine commented Feb 26, 2020

simianhacker left a comment

Zacqary commented Feb 27, 2020

kibanamachine commented Feb 28, 2020

[Metrics Alerts] Create Metric Threshold Alert Type and Executor #57606

[Metrics Alerts] Create Metric Threshold Alert Type and Executor #57606

Conversation

Zacqary commented Feb 13, 2020

Summary

Checklist

For maintainers

elasticmachine commented Feb 13, 2020

Zacqary commented Feb 13, 2020

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zacqary commented Feb 14, 2020

simianhacker commented Feb 18, 2020

Zacqary commented Feb 19, 2020

Zacqary commented Feb 19, 2020

Zacqary commented Feb 20, 2020

Zacqary commented Feb 26, 2020

phillipb left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 26, 2020

💚 Build Succeeded

History

simianhacker left a comment

Choose a reason for hiding this comment

Zacqary commented Feb 27, 2020

kibanamachine commented Feb 28, 2020