-
Notifications
You must be signed in to change notification settings - Fork 534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MQE: Add support for delta function #9795
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add some tests for delta
over native histograms?
It'd be good include native histograms with custom buckets, as well as scenarios that trigger the various annotations. (Have a look at our tests for rate
and increase
for some inspiration, and look at TestAnnotations
in engine_test.go
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also add some tests for the case where the value (float or histogram) increases and decreases during the range, to ensure that we're correctly ignoring resets?
val := calculateHistogramRate(isRate, step.RangeStart, step.RangeEnd, rangeSeconds, firstPoint, lastPoint, delta, hCount) | ||
return val, err | ||
} | ||
return delta, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is quite right - the docs for delta
say:
delta(v range-vector)
calculates the difference between the first and last value of each time series element in a range vectorv
, returning an instant vector with the given deltas and equivalent labels. The delta is extrapolated to cover the full time range as specified in the range vector selector...
I see two issues with the implementation here:
- we're not doing extrapolation
- we're considering resets in
delta
, but we want to consider just the first and last value
I wonder if it would be clearer and simpler for delta
to not use rate
above, and instead be implemented separately and call calculateFloatRate
and calculateHistogramRate
to perform extrapolation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was looking at how prometheus engine was implementing delta. But let me check at this again and try to apply your suggestion.
9ef52f2
to
b4c6a45
Compare
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]>
Signed-off-by: Jon Kartago Lamida <[email protected]>
b4c6a45
to
840c870
Compare
What this PR does
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.