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] Replace date_histogram with date_range aggregation in threshold alert #100004

Merged
merged 3 commits into from
May 14, 2021

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented May 12, 2021

Summary

Closes #100003

This switches Metric Threshold alerts from a date_histogram to a date_range in order to aggregate their data. This is in line with how Index Threshold alerts aggregate their data, and fixes an accuracy issue with gaps in document indexing.

It may also improve performance across all aggregation types.

Because this changes the way alerts evaluate, we should test this extensively to make sure it doesn't misfire existing alerts

Checklist

@Zacqary Zacqary added 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.14.0 Epic: Metrics UI Performance labels May 12, 2021
@Zacqary Zacqary requested a review from a team as a code owner May 12, 2021 22:19
@elasticmachine
Copy link
Contributor

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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
crossClusterReplication 8 6 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 5 3 -2
licensing 18 15 -3
monitoring 109 56 -53
total -67

History

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

Copy link
Contributor

@estermv estermv left a comment

Choose a reason for hiding this comment

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

I've tested this and everything seems to work.

I created some alarms, with different aggregations (avg, rate, min, max), some with warning thresholds, and the alarms were triggered correctly.

Do you think there is something else that should be tested specifically? Otherwise, I think this can be approved.

@Zacqary Zacqary enabled auto-merge (squash) May 14, 2021 15:59
@Zacqary
Copy link
Contributor Author

Zacqary commented May 14, 2021

@estermv That's probably good, I just get anxious about changes like these

We can merge if you wanna approve

@neptunian
Copy link
Contributor

neptunian commented May 14, 2021

Is having the rate agg still use date_histogram a temporary fix or do we need to find a non derivative value at some point? Could the rate agg still possibly cause the inaccuracy issue until then?

@Zacqary
Copy link
Contributor Author

Zacqary commented May 14, 2021

I think the inaccuracy issue is exclusive to doc_count. Changing the aggregation for the rest was more of for the possible performance boost.

@Zacqary Zacqary merged commit 2f3e175 into elastic:master May 14, 2021
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 18, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100004 or prevent reminders by adding the backport:skip label.

@jasonrhodes
Copy link
Member

@Zacqary I assume we just missed this by accident, which is no big deal -- let me know if you have questions/concerns or if you ran into any problems with the backport, though.

Zacqary added a commit to Zacqary/kibana that referenced this pull request May 19, 2021
…reshold alert (elastic#100004)

* [Metrics UI] Replace date_histogram with date_range aggregation in threshold alert

* Remove console.log

* Fix rate aggregation and offset
Zacqary added a commit to Zacqary/kibana that referenced this pull request May 19, 2021
…reshold alert (elastic#100004)

* [Metrics UI] Replace date_histogram with date_range aggregation in threshold alert

* Remove console.log

* Fix rate aggregation and offset
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

Zacqary added a commit that referenced this pull request May 19, 2021
…reshold alert (#100004) (#100350)

* [Metrics UI] Replace date_histogram with date_range aggregation in threshold alert

* Remove console.log

* Fix rate aggregation and offset
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 19, 2021
Zacqary added a commit that referenced this pull request May 19, 2021
…reshold alert (#100004) (#100349)

* [Metrics UI] Replace date_histogram with date_range aggregation in threshold alert

* Remove console.log

* Fix rate aggregation and offset
yctercero pushed a commit to yctercero/kibana that referenced this pull request May 25, 2021
…reshold alert (elastic#100004)

* [Metrics UI] Replace date_histogram with date_range aggregation in threshold alert

* Remove console.log

* Fix rate aggregation and offset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic: Metrics UI Performance Feature:Metrics UI Metrics UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.13.0 v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metrics UI] Doc count on Metric Threshold alerts are prone to data gaps
6 participants