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

CloudWatch: Calculate period based on time range #21471

Merged
merged 8 commits into from
Jan 17, 2020
Merged

Conversation

sunker
Copy link
Contributor

@sunker sunker commented Jan 13, 2020

This PR will add logic that will set a period interval based on the time range in case the period field in the query editor is left blank or has the value auto. If the period is actively defined by the user in the query editor, that value will be passed to the GMD api unconditionally. Previously the auto period calculations were made in the frontend, and therefore it didn't work for alerting. This PR moves the period calculation to the backend.

If the period field is not defined by the user, the period will be calculated the following way:
periods = [60, 300, 900, 3600, 21600]
x = timeRangeDeltaInSeconds / 2000
x will then snap to the closest number in the periods array

This is an example with a time range of 30 days
2,592,000 / 2000 = 1296 --> 900 is the closest number

fixes #20652
fixes #18493

@sunker sunker requested a review from nkogit January 13, 2020 15:23
@sunker sunker force-pushed the 20652-auto-min-interval branch 2 times, most recently from 234d21c to 5e517c7 Compare January 14, 2020 15:38
@sunker sunker changed the title CloudWatch: Calculate min period based on time range and no of queries CloudWatch: Calculate period based on time range Jan 15, 2020
@sunker sunker requested a review from mtanda January 16, 2020 09:04
@sunker
Copy link
Contributor Author

sunker commented Jan 16, 2020

@mtanda could you review this, please?

@sunker sunker marked this pull request as ready for review January 16, 2020 12:06
mtanda
mtanda previously requested changes Jan 17, 2020

Convey("closest works as expected", func() {
periods := []int{60, 300, 900, 3600, 21600}
Convey("and input is lower than 60", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sunker Would you add test which confirm the period isn't shorter than CloudWatch retension?

https://aws.amazon.com/about-aws/whats-new/2016/11/cloudwatch-extends-metrics-retention-and-new-user-interface/

Sample test case.

So(closest(periods, (60 * 60 * 24 * 15) + 1), ShouldEqual, 300)
So(closest(periods, (60 * 60 * 24 * 63) + 1), ShouldEqual, 3600)

@sunker sunker requested a review from mtanda January 17, 2020 11:47
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 dismissed mtanda’s stale review January 17, 2020 11:59

Requested changes fixed

@sunker sunker merged commit d40b66f into master Jan 17, 2020
@sunker sunker deleted the 20652-auto-min-interval branch January 17, 2020 12:22
@marefr marefr added this to the 6.6.0-beta1 milestone Jan 17, 2020
@mtanda
Copy link
Collaborator

mtanda commented Jan 17, 2020

@sunker When I tested 8616f1b, cloudwatch query is randomly failed.

I get error in my console. Would you test in your environment?

EROR[01-17|22:54:13] Metric request error                     logger=context userId=1 orgId=1 uname=admin error="Error parsing query A, time: invalid duration undefined"

@mtanda
Copy link
Collaborator

mtanda commented Jan 17, 2020

image
When I get query error, period is undefined.

image
When success, the period is empty string.

@mtanda
Copy link
Collaborator

mtanda commented Jan 17, 2020

I guess it can be reproduced, by creating new panel which use CloudWatch datasource.

@sunker
Copy link
Contributor Author

sunker commented Jan 17, 2020

Oh, must have been a merge problem. Fix coming right up @mtanda..

@sunker
Copy link
Contributor Author

sunker commented Jan 17, 2020

@mtanda see #21574

johntdyer pushed a commit to johntdyer/grafana that referenced this pull request Jan 21, 2020
* Calculate min period based on time range and no of queries

* Use hardcoded array of periods if period is not defined actively by the user

* Fix broken tests

* Use a smaller max period for auto interval

* Fix broken tests

* Test period calculation

* Test min retention period

* Fix broken test
@crxpandion
Copy link
Contributor

I think this commit introduced a behavior where when you zoom in (to say start-end < 15 days) on an event that occurred more than 15 or 63 days prior and period is set to auto the query will use period as 60 resulting in Cloudwatch returning no data.

To fix we will need to have a concept of now in this function to understand which period Cloudwatch has retained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants