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

Elasticsearch: Fix date histogram auto interval handling for alert queries #30049

Merged
merged 3 commits into from
Jan 14, 2021

Conversation

simianhacker
Copy link
Contributor

@simianhacker simianhacker commented Jan 4, 2021

What this PR does / why we need it:

The current backend Golang code doesn't honor the minimum interval set in the UI for alerts. This means that the data the alerts are triggering against will never match the data in the visualization. For example, if you're monitoring AWS Cloudwatch EC2 data that only reports every 5 minutes and you set the minimum interval to 5m, the alerts will not trigger correctly because it will use the auto interval instead which for 5m of data defaults to 200ms.

This PR fixes that by setting the minimum interval to the same value in the UI.

Which issue(s) this PR fixes:

Fixes #22082

Special notes for your reviewer:

@simianhacker simianhacker marked this pull request as ready for review January 7, 2021 21:22
@simianhacker simianhacker requested a review from a team as a code owner January 7, 2021 21:22
@simianhacker simianhacker requested review from joshhunt and wbrowne and removed request for a team January 7, 2021 21:22
@Elfo404 Elfo404 self-requested a review January 8, 2021 10:40
@marefr marefr self-requested a review January 12, 2021 18:03
@marefr
Copy link
Member

marefr commented Jan 12, 2021

Thanks. This change seemed wrong to me at first sight. I was the one implementing the first version of backend support for elasticsearch. I wonder if something did change in Grafana alerting since last 3 years :) Cannot remember whether this has never worked as expected or not. Could be that Grafana alerting needs an update to solve this problem for "all" data sources I don't know right now. I need to run, but will continue reviewing this tomorrow.

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Great catch. Added a suggestion, but other than that this looks good.

pkg/tsdb/elasticsearch/time_series_query.go Outdated Show resolved Hide resolved
@marefr marefr added this to the 7.4.0 milestone Jan 13, 2021
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@marefr marefr merged commit 3d7748d into grafana:master Jan 14, 2021
@marefr
Copy link
Member

marefr commented Jan 14, 2021

Thank you for contributing to Grafana!

@marefr marefr changed the title Elasticsearch: Use minimum interval for alerts Elasticsearch: Fix date histogram auto interval handling for alert queries Jan 14, 2021
@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alert sampling interval does not respect minimum time interval
4 participants