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

[Metrics UI] Alerting for metrics explorer and inventory #58779

Merged
merged 46 commits into from
Mar 23, 2020

Conversation

phillipb
Copy link
Contributor

@phillipb phillipb commented Feb 27, 2020

Summary

Add the alert flyout to the metric explorer and snapshot.

Metric Explorer

This PR allows creating alerts from a metrics chart, and from the header. When creating an alert from the chart, the flyout is pre-filled with conditions that correspond to the chart.

There is also the ability to create an alert per each distinct metric. For example, you can create an alert per hostname. This will allow you to create one alert for all hosts without needing to specify each exact hostname.

Inventory

On the inventory page, you can create alerts from the header and from a specific node. If you select a node, the flyout is pre-filled with a filter that targets that node. The inventory also supports the alert per functionality.

Flyout

The alert flyout uses the alerting teams AlertFlyout component. This comes with built-in actions, naming and frequency checks. We've extended it to support our need for multiple alert conditions, filtering and grouped alerts.

Fixes #56431, #56430, #56429

Screen Shot 2020-03-10 at 11 07 55 AM

Screen Shot 2020-03-17 at 6 19 17 PM

Screen Shot 2020-03-17 at 6 19 25 PM

Screen Shot 2020-03-17 at 6 19 47 PM

Checklist

Delete any items that are not applicable to this PR.

@phillipb phillipb marked this pull request as ready for review March 11, 2020 15:26
@phillipb phillipb requested review from a team as code owners March 11, 2020 15:26
@phillipb phillipb added 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 v7.7.0 v8.0.0 labels Mar 11, 2020
@elasticmachine
Copy link
Contributor

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

@phillipb phillipb changed the title [DRAFT] Metrics alerting UI for metrics explorer [Metrics UI] Alerting for metrics explorer Mar 11, 2020
@YulNaumenko YulNaumenko self-requested a review March 11, 2020 16:37
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM overal! But have a comment regarding toastNotifications.

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for a change about toast messages.

@simianhacker
Copy link
Member

I tried to create an alert from Metrics Explorer but I keep stubbing my toe on things:

image

results in this error from the server

image

@phillipb
Copy link
Contributor Author

@elasticmachine merge upstream

@phillipb
Copy link
Contributor Author

I tried to create an alert from Metrics Explorer but I keep stubbing my toe on things:

Was a small type error, and is now fixed.

@Zacqary
Copy link
Contributor

Zacqary commented Mar 19, 2020

Couple issues with pre-populating the flyout. Here's what I've configured the Metrics Explorer to fetch:
Screen Shot 2020-03-19 at 11 28 41 AM

When I use the Alerts dropdown in the upper-right corner of the screen, it doesn't seem to populate the flyout with anything:
Screen Shot 2020-03-19 at 11 28 54 AM

I'd expect it to look more like what I get when I use the Actions dropdown next to the graph:
Screen Shot 2020-03-19 at 11 27 45 AM

However, the Actions-populated alert uses both groupBy: agent.id and filtering down to a single agent.id, which is redundant. Maybe it's okay to leave it in there? Like from a UX standpoint it might be a good thing if the user just deletes the filter query, but I'm not sure.

But ultimately what I want to see is:

  • When I create an alert from the Alerts menu, populate the flyout with everything present in the toolbar: the aggregator, metric, groupBy, and filterQuery

@Zacqary
Copy link
Contributor

Zacqary commented Mar 19, 2020

Screen Shot 2020-03-19 at 11 37 42 AM

rate and cardinality are missing from here.

Also note that when we select Document Count in the metrics explorer it removes the metric field entirely, but the alert flyout still requires us to select a metric. @simianhacker maybe we need to have a full discussion of how Document Count alerts should work? They're a stranger case than I think we originally anticipated.

@Zacqary
Copy link
Contributor

Zacqary commented Mar 19, 2020

Getting this when I graph by Rate or Cardinality and then try to create an alert through the Actions menu:
Screen Shot 2020-03-19 at 11 42 37 AM

I see sum in the flyout's menu but I'm not sure if it's possible to graph by sum in the Metrics Explorer? Unless that's an alias for cardinality?

@Zacqary
Copy link
Contributor

Zacqary commented Mar 19, 2020

Screen Shot 2020-03-19 at 11 44 36 AM

Can this be Select a metric instead?

Also maybe give IS ABOVE a placeholder next to it so the user knows they have to click on it to add a number

@phillipb
Copy link
Contributor Author

When I use the Alerts dropdown in the upper-right corner of the screen, it doesn't seem to populate the flyout with anything:
Screen Shot 2020-03-19 at 11 28 54 AM

@Zacqary When you click the alerts action menu on the top of the screen, you're outside of the context of the graph. If you want to prepopulate the alert form, you should click create alert from the chart actions menu

@Zacqary
Copy link
Contributor

Zacqary commented Mar 19, 2020

@phillipb Right but that UX doesn't make sense for creating a groupBy alert, for example. If I groupBy something that gives me 8 different graphs and I want to create an alert on all of them, I can't do that by clicking on an Actions menu.

@Zacqary
Copy link
Contributor

Zacqary commented Mar 19, 2020

I want to be able to pre-populate this (note the empty query and present groupBy param):
Screen Shot 2020-03-19 at 11 48 12 AM

@phillipb
Copy link
Contributor Author

I want to be able to pre-populate this (note the empty query and present groupBy param):

Hmm, I can see the utility in that. It would require a pretty big refactor to support that use though. Could be something for a follow up PR.

@Zacqary
Copy link
Contributor

Zacqary commented Mar 19, 2020

@phillipb Okay, tracking that here: #60659

Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

LGTM

@phillipb
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@phillipb phillipb merged commit a790877 into elastic:master Mar 23, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 23, 2020
* master:
  Fix formatter on range aggregation (elastic#58651)
  Goodbye, legacy data plugin 👋 (elastic#60449)
  [Metrics UI] Alerting for metrics explorer and inventory (elastic#58779)
phillipb added a commit to phillipb/kibana that referenced this pull request Mar 23, 2020
* Add flyout with expressions

* Integrate frontend with backend

* Extended AlertContextValue with metadata optional property

* Progress

* Pre-fill criteria with current page filters

* Better validation. Naming for clarity

* Fix types for flyout

* Respect the groupby property in metric explorer

* Fix lint errors

* Fix text, add toast notifications

* Fix tests. Make sure update handles predefined expressions

* Dynamically load source from alert flyout

* Remove unused import

* Simplify and add group by functionality

* Remove unecessary useEffect

* disable exhastive deps

* Remove unecessary useEffect

* change language

* Implement design feedback

* Add alert dropdown to the header and snapshot screen

* Remove icon

* Remove unused props. Code cleanup

* Remove unused values

* Fix formatted message id

* Remove create alert option for now.

* Fix type issue

* Add rate, card and count as aggs

* Fix types

Co-authored-by: Yuliia Naumenko <yuliia.naumenko@elastic.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Henry Harding <henry.harding@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 23, 2020
* master: (26 commits)
  [Alerting] Fixes flaky test in Alert Instances Details page (elastic#60893)
  cleanup visualizations api (elastic#59958)
  Inline timezoneProvider function, remove ui/vis/lib/timezone  (elastic#60475)
  [SIEM] Adds 'Open one signal' Cypress test (elastic#60484)
  [UA] Upgrade assistant migration meta data can become stale (elastic#60789)
  [Metrics Alerts] Remove metric field from doc count on backend (elastic#60679)
  [Uptime] Skip failing location test temporarily (elastic#60938)
  [ML] Disabling datafeed editing when job is running (elastic#60751)
  Adding `authc.invalidateAPIKeyAsInternalUser` (elastic#60717)
  [SIEM] Add license check to ML Rule form (elastic#60691)
  Adding `authc.grantAPIKeyAsInternalUser`  (elastic#60423)
  Support Histogram Data Type (elastic#59387)
  [Upgrade Assistant] Fix edge case where reindex op can falsely be seen as stale (elastic#60770)
  [SIEM] [Cases] Update case icons (elastic#60812)
  [TSVB] Fix percentiles band mode (elastic#60741)
  Fix formatter on range aggregation (elastic#58651)
  Goodbye, legacy data plugin 👋 (elastic#60449)
  [Metrics UI] Alerting for metrics explorer and inventory (elastic#58779)
  [Remote clustersadopt changes to remote info API (elastic#60795)
  Only run xpack siem cypress in PRs when there are siem changes (elastic#60661)
  ...
phillipb added a commit that referenced this pull request Mar 23, 2020
…0948)

* Add flyout with expressions

* Integrate frontend with backend

* Extended AlertContextValue with metadata optional property

* Progress

* Pre-fill criteria with current page filters

* Better validation. Naming for clarity

* Fix types for flyout

* Respect the groupby property in metric explorer

* Fix lint errors

* Fix text, add toast notifications

* Fix tests. Make sure update handles predefined expressions

* Dynamically load source from alert flyout

* Remove unused import

* Simplify and add group by functionality

* Remove unecessary useEffect

* disable exhastive deps

* Remove unecessary useEffect

* change language

* Implement design feedback

* Add alert dropdown to the header and snapshot screen

* Remove icon

* Remove unused props. Code cleanup

* Remove unused values

* Fix formatted message id

* Remove create alert option for now.

* Fix type issue

* Add rate, card and count as aggs

* Fix types

Co-authored-by: Yuliia Naumenko <yuliia.naumenko@elastic.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Henry Harding <henry.harding@elastic.co>

Co-authored-by: Yuliia Naumenko <yuliia.naumenko@elastic.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Henry Harding <henry.harding@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Metrics UI Metrics UI feature release_note:enhancement 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
Development

Successfully merging this pull request may close these issues.

[Metrics Alerts] Add Create Alert menu option to Inventory Page
6 participants