Fix smartSummarize to properly handle alignTo start time and intervals smaller than data StepTime #137
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
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -62,28 +86,6 @@ func (f *smartSummarize) Do(ctx context.Context, e parser.Expr, from, until int6 | |||
| return nil, err | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I'd add a check after line 74 to make sure bucketSizeInt32 > 0. I tried passing a negative interval '-5m' and it panicked.
| } | ||
| } | ||
|
|
||
| func TestSmartSummarizeAlignTo1Year(t *testing.T) { |
There was a problem hiding this comment.
I don't think our current code can align past days, it's doing:
for _, v := range []int64{86400, 3600, 60} {
if int64(interval) >= v {
start -= start % v
break
}
}I think these tests are passing because their start time is 0, which is already aligned to years (and is also aligned to days). Same thing applies to TestSmartSummarizeAlignToMonths
There was a problem hiding this comment.
Working on adding support the other interval formats as well
| } | ||
| } | ||
|
|
||
| func TestSmartSummarizeAlignToWeeksThursday(t *testing.T) { |
There was a problem hiding this comment.
These GraphiteWeb tests are actually passing weeks4 to align it to a Thursday: https://github.com/graphite-project/graphite-web/blob/dca59dc72ae28ffdae659232c10c60aa598536eb/webapp/tests/test_functions.py#L5646
I think these translated tests pass here because the start times of the inputs are 0, and the epoch January 1st 1970 was actually a Thursday. This is a funny coincidence 😆
There was a problem hiding this comment.
Yeah, for now I just used 'weeks' because IntervalString fails to parse weeks4. But I will work on fixing that in IntervalString
| } | ||
| } | ||
|
|
||
| func TestSmartSummarizeAlignToDays(t *testing.T) { |
There was a problem hiding this comment.
For these tests, I would use start times that are not multiple of 86400. Otherwise it's not aligning much
| } | ||
| } | ||
|
|
||
| func TestSmartSummarizeAlignToHours(t *testing.T) { |
There was a problem hiding this comment.
For these I would use start times that are not multiple of 3600
| } | ||
| } | ||
|
|
||
| func TestSmartSummarizeAlignToMinutes(t *testing.T) { |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
carrieedwards
force-pushed
the
cedwards/fix-smartsummarize
branch
from
37a53da
to
e5ac035
Compare
This comment has been minimized.
This comment has been minimized.
|
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
npazosmendez
approved these changes
Apr 24, 2023
I will do that and update the tests in a separate PR, since this PR is already a pretty large one. |
leizor
pushed a commit
that referenced
this pull request
May 24, 2023
…s smaller than data StepTime (#137) * Enable parser to adjust start time for smartSummarize based on interval * Fix smartSummarize handling intervals that are smaller than the step time of the data * Add additional tests for smartSummarize * Optimize algorithm for smartSummarize * Calculate bucket limits with integer division * Add additional tests to cover Graphite web test cases * Add support for alignTo values of seconds, weeks, months and years * Error with negative intervals --------- Co-authored-by: Nicolás Pazos <npazosmendez@gmail.com>
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 rewrites the smartSummarize function in order to correct two issues that were discovered (similar to the recent fix to hitcount, see #132).
When the interval parameter 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:
smartSummarize(metric1,'6m','sum', 'minutes')
[]*types.MetricData{{"metric1", 0, 1}: {types.MakeMetricData("metric1", []float64{2, 4, 6}, 600, 0)},} // 10m step
The result will be []float64{2, 4, 6}. Issuing the same query with the same data in Graphite-web yielded results of [2, 4, None, 6, None]. This is due to the interval being smaller than the step, and therefore some resulting values will be NaN.
Additionally, in Graphite-web's implement of smartSummarize, the start time of the request is adjusted based on the alignTo value, and the data is re-fetched. In CarbonAPI, the start time was adjusted, but data was not re-fetched.
Major changes in this PR:
The smartSummarize function was completely rewritten to match the behavior of Graphite web, as well as adding some optimizations in the algorithm. This helps address the issue with intervals smaller than the data's step, as well as fixes discrepancies in results that were occurring with CarbonAPI tests that were tested in Graphite web.
The Metrics() function in pkg/parser.go was updated to adjust the start time of a metric request with a target of smartSummarize, so that the start time reflects the alignTo parameter's value. This is intended to prevent needing to re-fetch the data during smartSummarize function execution as is done in Graphite web. Additionally, previously, only 'minutes', 'hours', and 'days' was supported, so support was extended for seconds, weeks, months and years.
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 smartSummarize.