metrics: Improve flexibility of H2Histogram Configuration #6963
+126
−23
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
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.
tokio/src/runtime/builder.rs
Outdated
| @@ -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?
tokio/src/runtime/builder.rs
Outdated
Comment on lines
1249
to
1250
| /// ```rust | ||
| /// |
There was a problem hiding this comment.
Suggested change
| /// ```rust | |
| /// | |
| /// ```rust |
hds
reviewed
Nov 8, 2024
|
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!! |
rcoh
force-pushed
the
allow-p-0
branch
2 times, most recently
from
009d531 to
eaffd4b
Compare
|
This is ready btw @Darksonn |
|
@hds or @Noah-Kennedy can you review? |
|
I'll review today |
|
I'd like @hds to take a look actually here, as I have not been involved with the histogram too much. |
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.
|
I'll review this once I'm back from vacation next week. :) |
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.