-
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: native histograms #8121
Conversation
Add native histogram support to instant vectors.
I've marked this as ready-for-review if we want to put this in in its current state. I'm also happy to keep adding commits to this branch as we support more histogram operations. |
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.
Overall looks good, thanks for working on this!
Only other thing I'd like to see is some benchmarking:
- It'd be good to add a benchmark scenario or two that include native histograms
- It'd be good to compare the performance of the existing non-native histogram benchmarks before and after this change to confirm it doesn't have a significant impact on performance or memory utilisation - I can't imagine it would, but it'd be good to confirm
We can't use OOO samples with native histograms, so instead of batching by series we'll batch by samples This reverts commit 91ba5c1.
Benchmark of this branch in details:
|
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.
Changes overall look good to me 🚀
Could you please add this PR to the existing changelog entry for the new PromQL engine?
I'm not expecting any dramatic differences, but could you also please compare benchmark results for the non-native histogram selector cases for the new PromQL engine before and after this change? (eg. a_2000,_range_query_with_1000_steps
) Just want to make sure there are no unexpected significant regressions.
Benchmarking against main shows no difference within a margin of error:
|
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.
🚀
* Streaming PromQL engine: native histograms Add native histogram support to instant vectors. * Use seriesPerResultBucketFactor for histogram pool * Don't defer returning slices * Refactor slice returning logic * Move returnSlices helper * rename val for consistency * Update comment from feedback * Use returnSeriesDataSlices where missed * Comment out unsupported tests for now * Enable upstream native_histogram test file * Remove duplicate test * Fix FloatHistogram selector * Add range test * Add test for mixed floats+histograms * Re-enable supported upstream tests * Load benchmark samples in batches to avoid OOM'ing * Revert "Load benchmark samples in batches to avoid OOM'ing" We can't use OOO samples with native histograms, so instead of batching by series we'll batch by samples This reverts commit 91ba5c1. * Batch by samples instead of series * Generate native histogram benchmark test data * Add native histogram benchmark test * Move compare.sh next to other benchmark scripts * Fix comparison tests for when there are no floats * Add extra samples to the histogram tests * Add note on batch size * Add quick check that histograms are loaded * Use clearer variables for indexes * Update changelog * Fix typo
Add native histogram support to instant vectors.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.