-
Notifications
You must be signed in to change notification settings - Fork 543
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
Streaming PromQL engine: add support for range vector selectors as the top-level expression #8023
Streaming PromQL engine: add support for range vector selectors as the top-level expression #8023
Conversation
…WithTransformation into two types.
…perator range rather than calculating it at each time step.
d6e34a3
to
bd6c902
Compare
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.
Very nice job! 👏 I left few minor comments, but just looking at the code diff I can't see anything wrong.
return m.chunkIterator.Err() | ||
case chunkenc.ValFloat: | ||
t, f := m.chunkIterator.At() | ||
if value.IsStaleNaN(f) || t < rangeStart { |
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.
Is it correct to skip StaleNaN? Aren't they required to implement the correct behaviour for stale series? I'm fine if it's something that will be addressed in future PRs, but here I'm just trying to understand the rationale why they're skipped.
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've added tests for this in a9c6d40. Prometheus' engine ignores staleness markers in range vector selectors (and functions like rate()
), so the streaming engine does too.
I'm not sure this is correct for rate()
... I'd expect a stale marker to be treated like a counter reset.
However, this seems to be expected behaviour based on this comment.
… outside a step's range, or `rate` uses points from outside a step's range
c56a094
to
a9c6d40
Compare
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.
Thanks for addressing my comments, and good job spotting the bug!
Co-authored-by: Marco Pracucci <marco@pracucci.com>
What this PR does
This PR adds support for range vector selectors (eg.
my_metric[5m]
) as the top-level expression when using the streaming PromQL engine.As part of this PR, I've split the existing
RangeVectorSelectorWithTransformation
type that performed arate()
calculation over a range vector selector into two separate types:RangeVectorSelector
andRangeVectorFunction
(which still only supports therate()
function). Despite the addition of an extra level of indirection through an interface, this did not dramatically alter the benchmark results for queries that userate()
.RangeVectorSelector
is the first operator in the streaming engine to return a range vector (rather than an instant vector), so I'm keen to hear any feedback or opinions on the overall design andRangeVectorOperator
interface.Benchmark results:
The slight increase in latency and peak memory utilisation is inline with what we've seen for other test cases that return many series. This can likely be optimised further, but given the absolute difference is small, I'm not too concerned about this and would like to delay any further optimisation to a future PR - I want to keep this PR as simple as possible given the amount of refactoring already required.
Which issue(s) this PR fixes or relates to
(none)
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.