Skip to content
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

metrics: add H2 Histogram option to improve histogram granularity #6897

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

rcoh
Copy link
Contributor

@rcoh rcoh commented Oct 10, 2024

Motivation

Closes: #6896

Solution

This PR introduces a configurable new histogram for the poll-time histogram based on H2 Histograms. As part of this, three APIs are deprecated:

  • metrics_poll_count_histogram_scale
  • metrics_poll_count_histogram_resolution
  • metrics_poll_count_histogram_buckets

Since I suspect these are commonly used, I thought it was worth a little added complexity to provide a graceful path to removing them.

They are replaced with metrics_poll_count_histogram_configuration that accepts a histogram configuration object. The intention of this change is to provide more knobs when configuring histograms (as customers will want with the H2 histogram).

The default histogram remains the linear histogram with 10 buckets. The default H2 histogram provides a 25% error bound from 100ns to 60s of poll time and uses about 1KB of storage. This is configurable by customers as well.

The h2_histogram code is fairly complex compared to the code for the other histogram modes, so I isolated it into its own module. I brought back proptest to test invariants across a wide range of possible parameters–since there isn't any actual branching in this code, we don't get any value from making them fuzz tests (other than that they are a little more annoying to run).

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-tracing Tracing support in Tokio labels Oct 11, 2024
@Darksonn Darksonn requested a review from hds October 11, 2024 02:33
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine w/ this. Thanks.

Copy link
Contributor

@hds hds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple of suggestions on code style and some formatting nits, but this look good to me.

There is one place I mentioned where I think some more description on what the examples are doing would help people understand the configuration options.

tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
tokio/src/runtime/metrics/histogram.rs Show resolved Hide resolved
tokio/src/runtime/metrics/histogram.rs Outdated Show resolved Hide resolved
tokio/src/runtime/metrics/histogram.rs Outdated Show resolved Hide resolved
@rcoh rcoh force-pushed the histogram-precision branch 2 times, most recently from c228fd3 to d3d8532 Compare October 16, 2024 14:30
Copy link
Contributor

@hds hds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple more small things.

tokio/src/runtime/metrics/histogram/h2_histogram.rs Outdated Show resolved Hide resolved
tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
@rcoh rcoh force-pushed the histogram-precision branch 4 times, most recently from a0677a1 to f888b85 Compare October 17, 2024 20:08
@hds hds merged commit da745ff into tokio-rs:master Oct 21, 2024
82 checks passed
mox692 added a commit to mox692/tokio that referenced this pull request Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-tracing Tracing support in Tokio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need better histogram granularity
4 participants