-
Notifications
You must be signed in to change notification settings - Fork 529
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
First iteration of streaming PromQL engine #7693
First iteration of streaming PromQL engine #7693
Conversation
…eries don't interfere with load test results.
…formatting of output
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 guessing we defer docs on experimental features until it is stabilized which seems reasonable to me. In that case, don't consider my review necessary for the docs changes to be merged :)
pkg/querier/engine/streaming/operator/instant_vector_selector.go
Outdated
Show resolved
Hide resolved
pkg/querier/engine/streaming/operator/instant_vector_selector.go
Outdated
Show resolved
Hide resolved
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.
Super cool work! I'm hyper pumped about this! The design is very clean and easy to follow. I only found a potential critical issue with hash collisions (let me know if I'm missing something). Regardless I may be right or wrong with hash collision issue, I'm approving anyway because:
- Changes looks safe to me when using the standard engine (I can't see regressions)
- The new engine is experimental and a WIP. This PR is huge, so I suggest to merge it unless there are significant concerns, and then re-iterate on this through (hopefully) smaller PRs
P.S. I read the code, but I haven't reviewed the tests.
pkg/querier/engine/streaming/operator/instant_vector_selector.go
Outdated
Show resolved
Hide resolved
pkg/querier/engine/streaming/operator/range_vector_selector_with_transformation.go
Outdated
Show resolved
Hide resolved
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 fixing up the nits. I agree with Marco's comment about it not having any regression for the existing engine, and that we should merge and iterate from there.
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.
(FYI I have skimmed the tests and they look good to me. I have not considered what might be missing from them, but they are a great start)
What this PR does
This PR adds the first iteration of a streaming PromQL engine.
It supports a very limited feature set, and is far from production-ready.
It supports the following PromQL features:
my_metric{env="prod"}
andmy_metric{env="prod"} @ 123
)sum
aggregations with no grouping and withby
(eg.sum(my_metric)
orsum by (env) (my_metric)
)rate
(eg.rate(my_metric[5m])
)sum
andrate
(eg.sum by (env) (rate(my_metric[5m]))
)A number of features will be added in subsequent PRs, including support for falling back to Prometheus' PromQL engine if an unsupported PromQL feature is used, and support for native histograms.
Before reviewing this PR, I'd recommend reading this short introduction to the engine I've included in this PR, as it gives an overview of how the engine is structured and how the various parts interact.
There are many improvements I'd like to make beyond what's in this PR, but I believe what is here is good enough to merge, and we can build upon what's here in future PRs.
Benchmark results
These results compare Prometheus' PromQL engine (
standard
below) with the streaming engine added in this PR (streaming
below).Note that while these benchmarks capture latency and memory allocations, they do not capture peak memory utilisation. Reducing peak memory utilisation is one of the main goals behind this engine, and capturing this in benchmarks will be added in a subsequent PR. Initial peak memory utilisation benchmark results can be seen in this presentation.
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.