Fix summarize() handling of intervals #143
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carrieedwards
force-pushed
the
cedwards/fix-summarize
branch
from
e1478ce
to
3763070
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
carrieedwards
force-pushed
the
cedwards/fix-summarize
branch
from
1ce8946
to
766932c
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
carrieedwards
force-pushed
the
cedwards/fix-summarize
branch
from
766932c
to
3c0b788
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
…r than data start time
carrieedwards
force-pushed
the
cedwards/fix-summarize
branch
from
eb253a7
to
2c694fe
Compare
|
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a rewrite of the summarize function in order to match Graphite web's behavior.
The main issue that was discovered is that when the interval parameter passed into summarize() was set to a value smaller than the fetched data's step between data points, it results were incorrect. The results were a copy of the data points for each series in the list. For example:
summarize(metric1,'5s')
[]*types.MetricData("metric1", []float64{1, 2, 3, 4, 5}, 10, now32)} // 10s step
The result was []float64{1, 2, 3, 4 5}. Issuing the same query with the same data in Graphite-web yielded results of [1, None, 2, Non, 3, None, 4, None, 5, None, None]. This is due to the interval being smaller than the step, and therefore some resulting values will be NaN.
This issue was corrected with a re-write of the summarize function, which now more closely matches Graphite web's implementation (see here and here.
Additionally, the stop time can be changed depending on how the intervals and stop time line up, so this was adjusted in the new version of the function.
More testcases were added, including ones that cover the situation where the specific interval is smaller than the fetched data's step, as well as test cases translated from Graphite web for summarize().