Extract delta from rate function #10353
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
Signed-off-by: Jon Kartago Lamida <[email protected]>
charleskorn
reviewed
Jan 6, 2025
|
|
||
| func floatDelta(fCount int, fHead []promql.FPoint, fTail []promql.FPoint, rangeStart int64, rangeEnd int64, rangeSeconds float64) float64 { | ||
| firstPoint := fHead[0] | ||
| fHead = fHead[1:] |
There was a problem hiding this comment.
[nit] We don't need to do this for delta.
|
|
||
| func histogramDelta(hCount int, hHead []promql.HPoint, hTail []promql.HPoint, rangeStart int64, rangeEnd int64, rangeSeconds float64, emitAnnotation types.EmitAnnotationFunc) (*histogram.FloatHistogram, error) { | ||
| firstPoint := hHead[0] | ||
| hHead = hHead[1:] |
There was a problem hiding this comment.
[nit] We don't need to do this for delta.
Comment on lines
344
to
346
| if firstPoint.H.CounterResetHint == histogram.GaugeType || lastPoint.H.CounterResetHint == histogram.GaugeType { | ||
| emitAnnotation(annotations.NewNativeHistogramNotCounterWarning) | ||
| } |
There was a problem hiding this comment.
Is this correct? We emit a "not a gauge" warning further down if either point is not a gauge.
(we might need to add more test cases to TestAnnotations in engine_test.go)
There was a problem hiding this comment.
This should apply only for rate and increase. Removed in 7ba501f
There was a problem hiding this comment.
Added one test case in 56f67de. Still figuring out how to set CounterResetHint from promql test.
Comment on lines
367
to
369
| if delta.Schema != desiredSchema { | ||
| delta = delta.CopyToSchema(desiredSchema) | ||
| } |
There was a problem hiding this comment.
I don't think this is necessary - the Sub and CopyToSchema calls above should cover this.
Comment on lines
358
to
359
| delta := lastPoint.H.CopyToSchema(desiredSchema) | ||
| _, err := delta.Sub(firstPoint.H) |
There was a problem hiding this comment.
Couldn't we do this? Sub will automatically adjust the schema if necessary.
Suggested change
| delta := lastPoint.H.CopyToSchema(desiredSchema) | |
| _, err := delta.Sub(firstPoint.H) | |
| delta, err := lastPoint.H.Copy().Sub(firstPoint.H) |
If so, then we don't need desiredSchema at all.
| } | ||
|
|
||
| if fCount >= 2 { | ||
| // TODO: just pass step here? (and below) |
There was a problem hiding this comment.
Is this TODO still relevant?
Co-authored-by: Charles Korn <[email protected]>
Co-authored-by: Charles Korn <[email protected]>
Co-authored-by: Charles Korn <[email protected]>
Co-authored-by: Charles Korn <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
|
@charleskorn I have addressed almost all of your suggestion. I will merge this to #9795 and we can continue the discussion there. |
lamida
added a commit
that referenced
this pull request
Jan 9, 2025
* Extract delta from rate function Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/streamingpromql/operators/functions/rate_increase.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/streamingpromql/operators/functions/rate_increase.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/streamingpromql/operators/functions/rate_increase.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/streamingpromql/operators/functions/rate_increase.go Co-authored-by: Charles Korn <[email protected]> * Tidy up after applying PR suggestion Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove unnecessary head subslice Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove wrong placed annotation Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove extra copySchema Signed-off-by: Jon Kartago Lamida <[email protected]> * Simplify native histogram sub schema Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove TODO Signed-off-by: Jon Kartago Lamida <[email protected]> * The lastPoint should be last index of the head Signed-off-by: Jon Kartago Lamida <[email protected]> * Add delta counterResetHint test Signed-off-by: Jon Kartago Lamida <[email protected]> --------- Signed-off-by: Jon Kartago Lamida <[email protected]> Co-authored-by: Charles Korn <[email protected]>
lamida
added a commit
that referenced
this pull request
Jan 10, 2025
* Extract delta from rate function Signed-off-by: Jon Kartago Lamida <[email protected]> * Update pkg/streamingpromql/operators/functions/rate_increase.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/streamingpromql/operators/functions/rate_increase.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/streamingpromql/operators/functions/rate_increase.go Co-authored-by: Charles Korn <[email protected]> * Update pkg/streamingpromql/operators/functions/rate_increase.go Co-authored-by: Charles Korn <[email protected]> * Tidy up after applying PR suggestion Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove unnecessary head subslice Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove wrong placed annotation Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove extra copySchema Signed-off-by: Jon Kartago Lamida <[email protected]> * Simplify native histogram sub schema Signed-off-by: Jon Kartago Lamida <[email protected]> * Remove TODO Signed-off-by: Jon Kartago Lamida <[email protected]> * The lastPoint should be last index of the head Signed-off-by: Jon Kartago Lamida <[email protected]> * Add delta counterResetHint test Signed-off-by: Jon Kartago Lamida <[email protected]> --------- Signed-off-by: Jon Kartago Lamida <[email protected]> Co-authored-by: Charles Korn <[email protected]>
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 trying to address suggestion in #9795 to extract delta function from rate. The diff will show how we have many more line additions if we extract the logic from rate.
How this different with #9795
deltacalculateFloatRateandcalculateHistogramRateto do extrapolationMy concern with extracting delta from rate is we create many line duplications, such as most of lines in delta(), floatDelta() and histogramDelta(). This is reflected from 118 line additions and 36 line removal in this PR diffs.
What this PR does
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.