metrics: Improve flexibility of H2Histogram Configuration #6963
+135
−33
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
Based on feedback tokio-rs#6960, customers want more flexibility in histogram configuration. The main change here is allowing precision for an H2 Histgoram to go all the way down to zero—there is not fundamental reason this wasn't possible before, but typically H2 histograms stop at P=2. I've also added another optimization that truncates the number of buckets based on the requested max_value. This can save space by truncating in the middle of a bucket group. Along the way and in the process of adding more tests, I found a couple of bugs which are fixed here: 1. The `max_value` in LogHistogramBuilder wasn't precisely respected 2. The `max_value` computed by `LogHistogram` was actually incorrect for some n/p combinations. The new version precisely considers bucket layout instead.
Darksonn
added
A-tokio
Darksonn
reviewed
Nov 8, 2024
tokio/tests/rt_unstable_metrics.rs
Outdated
| for b in 0..num_buckets { | ||
| let range = metrics.poll_time_histogram_bucket_range(b); | ||
| let size = range.end - range.start; | ||
| println!("bucket {b}: {range:?} (size: {size:?})"); |
There was a problem hiding this comment.
Can we assert their values here instead of printing?
| panic!("precision must be >= 2"); | ||
| }; | ||
| if p > 10 { | ||
| if p > MAX_PRECISION { | ||
| panic!("precision must be <= 10"); |
There was a problem hiding this comment.
Suggested change
| panic!("precision must be <= 10"); | |
| panic!("precision must be <= {}", MAX_PRECISION); |
Comment on lines
167
to
169
| /// To match the behavior of the previous log histogram implementation, use | ||
| /// `builder.precision_exact(0)`. This creates a histogram where each bucket is twice the size | ||
| /// of the previous value. |
There was a problem hiding this comment.
I'd revert the order of this sentence so you first say that passing 0 results in buckets with doubling being used for the size, and then you can comment that this matches the behavior used in the legacy implementation.
| @@ -1245,8 +1245,30 @@ impl Builder { | |||
| /// .unwrap(); | |||
| /// ``` | |||
| /// | |||
| /// Configure a `LogHistogram` with similar behavior to [`HistogramScale::Log`] | |||
There was a problem hiding this comment.
This sentence is confusing. I'm guessing HistogramScale::Log is the old implementation?
Comment on lines
+1249
to
+1250
| /// ```rust | ||
| /// |
There was a problem hiding this comment.
Suggested change
| /// ```rust | |
| /// | |
| /// ```rust |
hds
reviewed
Nov 8, 2024
| prop_assert!(last_bucket_start > max_value, "last bucket start ({last_bucket_start:?} must be at least as big as `max_value` ({max_value:?})"); | ||
|
|
||
| // We should have the exact right number of buckets. The second to last bucket should be strictly less than max value. | ||
| prop_assert!(second_last_bucket_start < max_value, "second last bucket end ({second_last_bucket_start:?} must be at least as big as `max_value` ({max_value:?})"); |
There was a problem hiding this comment.
nit: long lines, perhaps this could be manually split up?
Comment on lines
205
to
206
| if p > MAX_PRECISION { | ||
| panic!("precision must be <= 10"); |
There was a problem hiding this comment.
I know this is from the previous commit, but perhaps this could be an assert instead, that's how pre-conditions are mostly checked in tokio.
|
Any status update on this? |
|
Sorry I went on parental leave, I will have someone from my team follow up
…On Thu, Nov 21, 2024, 11:29 AM Alice Ryhl ***@***.***> wrote:
Any status update on this?
—
Reply to this email directly, view it on GitHub
<#6963 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADYKZ73MH7JJPJLKHMEDML2BYC7VAVCNFSM6AAAAABRNWIB2OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJRG4YDSMZRGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Congratulations!! |
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.
Motivation
Based on feedback #6960, customers want more flexibility in histogram configuration.
The main change here is allowing precision for an H2 Histgoram to go all the way down to zero—there is not fundamental reason this wasn't possible before, but typically H2 histograms stop at P=2.
Solution
I've also added another optimization that truncates the number of buckets based on the requested max_value. This can save space by truncating in the middle of a bucket group.
Along the way and in the process of adding more tests, I found a couple of bugs which are fixed here:
max_valuein LogHistogramBuilder wasn't precisely respectedmax_valuecomputed byLogHistogramwas actually incorrect for some n/p combinations. The new version precisely considers bucket layout instead.