Native histograms custom buckets storage #13662
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* add custom buckets to native histogram model * simple copy for custom bounds * return errors for unsupported add/sub operations * add test cases for string and update appendhistogram in scrape to account for new schema * check fields which are supposed to be unused but may affect results in equals * allow appending custom buckets histograms regardless of max schema Signed-off-by: Jeanette Tan <[email protected]>
|
/prombench main |
|
@krajorama is not a org member nor a collaborator and cannot execute benchmarks. |
|
/prombench main |
|
I don't see any difference. Also, prombench data doesn't have any native histograms. |
|
Benchmark tests are running for 3 days! If this is intended ignore this message otherwise you can cancel it by commenting: |
|
/prombench cancel |
|
Benchmark cancel is in progress. |
We made modifications to the engine, was wondering about any regression on the non-experimental part in floats. |
* add custom bounds to chunks encoding * change custom buckets schema number * rename custom bounds to custom values Signed-off-by: Jeanette Tan <[email protected]>
The size of histogram points are now bigger by 24 bytes due to the custom values slice. When histograms are loaded into partial results in vector selectors we use HPoint type where the size is calculated as (size of histogram + 8 for timestamp)/16. https://github.com/prometheus/prometheus/blob/a3d1a46eda682590a80fb1f15959457dad1e5d91/promql/value.go#L176 When histograms are put into Sample type in range evaluations, the Sample has more overhead and the size is calculated differently: (size of histogram / 16) + 1 for time stamp. https://github.com/prometheus/prometheus/blob/a3d1a46eda682590a80fb1f15959457dad1e5d91/promql/engine.go#L1928 When the size of the histogram is 16k, then the first calculation gives k but the second gives k+1 for the sample count. If the histogram size is 16k+8, then both would give k+1. Signed-off-by: György Krajcsovits <[email protected]>
…s and floats (#13828) * Use single bit to differentiate between optimized bounds and floats Use one bit to decide what kind of data to read/write. This reduces storage need of floats from 72 bits to 65 bits and makes the integers store in 5 to 32 bits instead of 16. Signed-off-by: György Krajcsovits <[email protected]> Signed-off-by: Jeanette Tan <[email protected]> Signed-off-by: György Krajcsovits <[email protected]> Signed-off-by: Jeanette Tan <[email protected]> Signed-off-by: George Krajcsovits <[email protected]> Co-authored-by: Jeanette Tan <[email protected]>
…buckets converted from classic histograms (#13794) * modify unit test framework to automatically generate native histograms with custom buckets from classic histogram series * add very basic tests for classic histogram converted into native histogram with custom bounds * fix histogram_quantile for native histograms with custom buckets * make loading with nhcb explicit * evaluate native histograms with custom buckets on queries with explicit keyword * use regex replacer * use temp histogram struct for automatically loading converted nhcb Signed-off-by: Jeanette Tan <[email protected]> Signed-off-by: George Krajcsovits <[email protected]>
[nhcb] Merge main at bd18787
Signed-off-by: Jeanette Tan <[email protected]>
[nhcb branch] Merge main
* process custom values in histogram unit test framework * check for warnings when evaluating in unit test framework * add test cases for custom buckets in test framework Signed-off-by: Jeanette Tan <[email protected]>
krajorama
changed the title
Signed-off-by: Jeanette Tan <[email protected]>
Signed-off-by: Jeanette Tan <[email protected]>
Signed-off-by: Jeanette Tan <[email protected]>
Signed-off-by: Jeanette Tan <[email protected]>
[nhcb branch] address review comments and merge with main
Currently we can only reduce the resolution of exponential native histograms, so checking the schema for that is slightly more precise than checking against max schema. Signed-off-by: György Krajcsovits <[email protected]>
This was referenced Jun 12, 2024
|
Let me know when you want me to have another look here. |
Signed-off-by: György Krajcsovits <[email protected]>
Signed-off-by: György Krajcsovits <[email protected]>
native histograms: only reduce resolution for exponential histograms
Signed-off-by: Jeanette Tan <[email protected]>
Signed-off-by: Jeanette Tan <[email protected]>
[nhcb branch] update missed suggested change from code review
|
@beorn7 PTAL |
krajorama
commented
Jun 21, 2024
Signed-off-by: George Krajcsovits <[email protected]>
Some weird formatting issue in using comment suggestion Signed-off-by: György Krajcsovits <[email protected]>
|
Will give it another round of review ASAP. |
beorn7
approved these changes
Jun 27, 2024
There was a problem hiding this comment.
Great epic stuff, thank you very much.
Would you prefer to keep the current commit structure, or would you rather like to squash everything into one commit? I would tend to do the former.
Or maybe you want to restructure the commits.
I'll wait for your pick before merging.
|
Thank you! Keeping the current structure sounds good. |
|
With the wild commit history, the DCO test got confused, but I think all commits are signed alright. |
jan--f
added a commit
to jan--f/prometheus
that referenced
this pull request
Jul 1, 2024
Conflicts:
promql/engine_test.go
Resolved by picking main changes but adjusting total_samples for
query "max_over_time(metricWith1HistogramEvery10Seconds[60s])[20s:5s]"
to 312. Via prometheus#13662 this
histogram now stores 13 values per timestamp, but via
prometheus#13904 the range query
is now left-open.
promql/promqltest/testdata/functions.test
Resolved by picking changes in main. See also
prometheus#13662.
promql/promqltest/testdata/histograms.test
Resolved by picking changes in main. See also
prometheus#13662, but adjust some
range selectors (`s/5m/10m/`) to account for
prometheus#13904
jan--f
added a commit
to jan--f/prometheus
that referenced
this pull request
Jul 3, 2024
Conflicts:
promql/engine_test.go
Resolved by picking main changes but adjusting total_samples for
query "max_over_time(metricWith1HistogramEvery10Seconds[60s])[20s:5s]"
to 312. Via prometheus#13662 this
histogram now stores 13 values per timestamp, but via
prometheus#13904 the range query
is now left-open.
promql/promqltest/testdata/functions.test
Resolved by picking changes in main. See also
prometheus#13662, but adjust some
range selectors (`s/1m/2m/`) to account for
prometheus#13904.
promql/promqltest/testdata/histograms.test
Resolved by picking changes in main. See also
prometheus#13662, but adjust some
range selectors (`s/5m/10m/`) to account for
prometheus#13904.
Signed-off-by: Jan Fajerski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements parts of #13485
#13486
#13487
#13508
In short: implements the internal model, storage (TSDB) and basic unit tests for the storage and query.
No user visible impact yet. No scraping/remote write/configuration change.