Align series by step in *seriesLists functions if steps or value length not equal #99
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.
npazosmendez
reviewed
Sep 7, 2022
| @@ -133,6 +132,7 @@ func (f *seriesList) Do(ctx context.Context, e parser.Expr, from, until int64, v | |||
| } | |||
|
|
|||
| var denominator *types.MetricData | |||
| step, alignStep := helper.GetCommonStep(append(numerators, denominators...)) | |||
There was a problem hiding this comment.
I think the step should be calculated for each pair of series and not for all of them
Comment on lines
154
to
158
| if alignStep || len(numerator.Values) != len(denominator.Values) { | ||
| alignedSeries := helper.ScaleToCommonStep(types.CopyMetricDataSlice([]*types.MetricData{numerator, denominator}), step) | ||
| numerator = alignedSeries[0] | ||
| denominator = alignedSeries[1] |
There was a problem hiding this comment.
Following up on my other comment, I think I would calculate the step and alignStep here:
pair := []*types.MetricData{numerator, denominator})
step, alignStep := helper.GetCommonStep(pair)
if alignStep || len(numerator.Values) != len(denominator.Values) {
....
carrieedwards
changed the title
This comment has been minimized.
This comment has been minimized.
carrieedwards
force-pushed
the
cedwards/fix-divideSeriesList
branch
from
1c62824
to
b18fcce
Compare
|
Go coverage report: Click to expand.
Go lint report: No issues found. 😎 |
carrieedwards
changed the title
carrieedwards
added a commit
that referenced
this pull request
Sep 14, 2022
…th not equal (#99) Align series by step in divideBySeries if steps not equal
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 addresses a bug with the *seriesLists functions, such as divideSeriesList. This function has been returning an error if the step times of the numerator and denominator are not equal, or if the length of the values in each series are not equal. This is not consistent with Graphite web's implementation, which finds the LCM of the step and then consolidates based on that step time. The numerators and denominators should now be aligned by step, and should be aligned when the number of values are different, and no error should be returned for mismatched step times or value lengths between series.