Extract delta from rate function #10353
Conversation
Signed-off-by: Jon Kartago Lamida <[email protected]>
| func delta() RangeVectorStepFunction { | ||
| return func(step *types.RangeVectorStepData, rangeSeconds float64, emitAnnotation types.EmitAnnotationFunc) (float64, bool, *histogram.FloatHistogram, error) { |
There was a problem hiding this comment.
I think this can be simplified to this - there's no need for the wrapper function given we don't need different functionality like we do for rate and increase:
| func delta() RangeVectorStepFunction { | |
| return func(step *types.RangeVectorStepData, rangeSeconds float64, emitAnnotation types.EmitAnnotationFunc) (float64, bool, *histogram.FloatHistogram, error) { | |
| func delta(step *types.RangeVectorStepData, rangeSeconds float64, emitAnnotation types.EmitAnnotationFunc) (float64, bool, *histogram.FloatHistogram, error) { |
|
|
||
| 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.
| desiredSchema := firstPoint.H.Schema | ||
| if lastPoint.H.Schema < desiredSchema { | ||
| desiredSchema = lastPoint.H.Schema | ||
| } |
There was a problem hiding this comment.
[nit] This could be simplified to:
| desiredSchema := firstPoint.H.Schema | |
| if lastPoint.H.Schema < desiredSchema { | |
| desiredSchema = lastPoint.H.Schema | |
| } | |
| desiredSchema := min(firstPoint.H.Schema, lastPoint.H.Schema) |
| 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)
| usingCustomBuckets := firstPoint.H.UsesCustomBuckets() | ||
| if lastPoint.H.UsesCustomBuckets() != usingCustomBuckets { |
There was a problem hiding this comment.
[nit] This could be simplified to:
| usingCustomBuckets := firstPoint.H.UsesCustomBuckets() | |
| if lastPoint.H.UsesCustomBuckets() != usingCustomBuckets { | |
| if firstPoint.H.UsesCustomBuckets() != lastPoint.H.UsesCustomBuckets() { |
| 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.
| 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.
| 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?
| hCount := len(hHead) + len(hTail) | ||
|
|
||
| if fCount > 0 && hCount > 0 { | ||
| // We need either at least two histograms and no floats, or at least two floats and no histograms to calculate a rate. |
There was a problem hiding this comment.
| // We need either at least two histograms and no floats, or at least two floats and no histograms to calculate a rate. | |
| // We need either at least two histograms and no floats, or at least two floats and no histograms to calculate a delta. |
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.