From bf62038648c7376f579bb33ac10eb5690e3d6c1d Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Fri, 8 Nov 2024 08:17:49 -0500 Subject: [PATCH 1/3] metrics: Improve flexibility of H2Histogram Configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. 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. --- tokio/src/runtime/builder.rs | 22 ++++++ .../runtime/metrics/histogram/h2_histogram.rs | 72 +++++++++++++++---- tokio/tests/rt_unstable_metrics.rs | 23 ++++++ 3 files changed, 104 insertions(+), 13 deletions(-) diff --git a/tokio/src/runtime/builder.rs b/tokio/src/runtime/builder.rs index 53edc048208..b910da74058 100644 --- a/tokio/src/runtime/builder.rs +++ b/tokio/src/runtime/builder.rs @@ -1245,8 +1245,30 @@ impl Builder { /// .unwrap(); /// ``` /// + /// Configure a `LogHistogram` with similar behavior to [`HistogramScale::Log`] + /// ```rust + /// + /// use std::time::Duration; + /// use tokio::runtime::{HistogramConfiguration, LogHistogram}; + /// let rt = tokio::runtime::Builder::new_current_thread() + /// .enable_all() + /// .enable_metrics_poll_time_histogram() + /// .metrics_poll_time_histogram_configuration(HistogramConfiguration::log( + /// LogHistogram::builder() + /// .max_value(Duration::from_millis(5)) + /// .min_value(Duration::from_micros(20)) + /// // Set `precision_exact` to `0` to match `HistogramScale::Log` + /// .precision_exact(0) + /// .max_buckets(10) + /// .unwrap(), + /// )) + /// .build() + /// .unwrap(); + /// ``` + /// /// [`LogHistogram`]: crate::runtime::LogHistogram /// [default configuration]: crate::runtime::LogHistogramBuilder + /// [`HistogramScale::Log`]: crate::runtime::HistogramScale::Log pub fn metrics_poll_time_histogram_configuration(&mut self, configuration: HistogramConfiguration) -> &mut Self { self.metrics_poll_count_histogram.histogram_type = configuration.inner; self diff --git a/tokio/src/runtime/metrics/histogram/h2_histogram.rs b/tokio/src/runtime/metrics/histogram/h2_histogram.rs index 09e554440f4..cdeb6d5ba52 100644 --- a/tokio/src/runtime/metrics/histogram/h2_histogram.rs +++ b/tokio/src/runtime/metrics/histogram/h2_histogram.rs @@ -9,6 +9,7 @@ const DEFAULT_MAX_VALUE: Duration = Duration::from_secs(60); /// Default precision is 2^-2 = 25% max error const DEFAULT_PRECISION: u32 = 2; +const MAX_PRECISION: u32 = 10; /// Log Histogram /// @@ -57,6 +58,15 @@ impl LogHistogram { } } + fn truncate_to_max_value(&self, max_value: u64) -> LogHistogram { + let mut hist = self.clone(); + while hist.max_value() >= max_value { + hist.num_buckets -= 1; + } + hist.num_buckets += 1; + hist + } + /// Creates a builder for [`LogHistogram`] pub fn builder() -> LogHistogramBuilder { LogHistogramBuilder::default() @@ -64,8 +74,7 @@ impl LogHistogram { /// The maximum value that can be stored before truncation in this histogram pub fn max_value(&self) -> u64 { - let n = (self.num_buckets / (1 << self.p)) - 1 + self.p as usize; - (1_u64 << n) - 1 + self.bucket_range(self.num_buckets - 2).end } pub(crate) fn value_to_bucket(&self, value: u64) -> usize { @@ -155,6 +164,10 @@ impl LogHistogramBuilder { /// such that `2^-p` is less than `precision`. To set `p` directly, use /// [`LogHistogramBuilder::precision_exact`]. /// + /// 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. + /// /// The default value is 25% (2^-2) /// /// The highest supported precision is `0.0977%` `(2^-10)`. Provided values @@ -171,7 +184,7 @@ impl LogHistogramBuilder { panic!("precision must be > 1"); }; let mut p = 2; - while 2_f64.powf(-1.0 * p as f64) > max_error && p <= 10 { + while 2_f64.powf(-1.0 * p as f64) > max_error && p <= MAX_PRECISION { p += 1; } self.precision = Some(p); @@ -180,14 +193,16 @@ impl LogHistogramBuilder { /// Sets the precision of this histogram directly. /// + /// The precision (meaning: the ratio `n/bucket_range(n)` for some given `n`) will be `2^-p`. + /// + /// 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. + /// /// # Panics - /// - `p` < 2 /// - `p` > 10 pub fn precision_exact(mut self, p: u32) -> Self { - if p < 2 { - panic!("precision must be >= 2"); - }; - if p > 10 { + if p > MAX_PRECISION { panic!("precision must be <= 10"); }; self.precision = Some(p); @@ -234,16 +249,17 @@ impl LogHistogramBuilder { /// Builds the log histogram pub fn build(&self) -> LogHistogram { - let max_value = duration_as_u64(self.max_value.unwrap_or(DEFAULT_MAX_VALUE)); - let max_value = max_value.next_power_of_two(); + let requested_max_value = duration_as_u64(self.max_value.unwrap_or(DEFAULT_MAX_VALUE)); + let max_value = requested_max_value.next_power_of_two(); let min_value = duration_as_u64(self.min_value.unwrap_or(DEFAULT_MIN_VALUE)); let p = self.precision.unwrap_or(DEFAULT_PRECISION); // determine the bucket offset by finding the bucket for the minimum value. We need to lower // this by one to ensure we are at least as granular as requested. let bucket_offset = cmp::max(bucket_index(min_value, p), 1) - 1; // n must be at least as large as p - let n = max_value.ilog2().max(p); + let n = max_value.ilog2().max(p) + 1; LogHistogram::from_n_p(n, p, bucket_offset as usize) + .truncate_to_max_value(requested_max_value) } } @@ -291,21 +307,51 @@ mod test { use super::InvalidHistogramConfiguration; use crate::runtime::metrics::histogram::h2_histogram::LogHistogram; use crate::runtime::metrics::histogram::HistogramType; + use std::time::Duration; #[cfg(not(target_family = "wasm"))] mod proptests { use super::*; + use crate::runtime::metrics::batch::duration_as_u64; + use crate::runtime::metrics::histogram::h2_histogram::MAX_PRECISION; use proptest::prelude::*; + use std::time::Duration; + fn valid_log_histogram_strategy() -> impl Strategy { - (2..=50u32, 2..=16u32, 0..100usize).prop_map(|(n, p, bucket_offset)| { + (1..=50u32, 0..=MAX_PRECISION, 0..100usize).prop_map(|(n, p, bucket_offset)| { let p = p.min(n); let base = LogHistogram::from_n_p(n, p, 0); LogHistogram::from_n_p(n, p, bucket_offset.min(base.num_buckets - 1)) }) } + fn log_histogram_settings() -> impl Strategy { + ( + duration_as_u64(Duration::from_nanos(1))..duration_as_u64(Duration::from_secs(20)), + duration_as_u64(Duration::from_secs(1))..duration_as_u64(Duration::from_secs(1000)), + 0..MAX_PRECISION, + ) + } + // test against a wide assortment of different histogram configurations to ensure invariants hold proptest! { + #[test] + fn log_histogram_settings_maintain_invariants((min_value, max_value, p) in log_histogram_settings()) { + if max_value < min_value { + return Ok(()) + } + let (min_value, max_value) = (Duration::from_nanos(min_value), Duration::from_nanos(max_value)); + let histogram = LogHistogram::builder().min_value(min_value).max_value(max_value).precision_exact(p).build(); + let first_bucket_end = Duration::from_nanos(histogram.bucket_range(0).end); + let last_bucket_start = Duration::from_nanos(histogram.bucket_range(histogram.num_buckets - 1).start); + let second_last_bucket_start = Duration::from_nanos(histogram.bucket_range(histogram.num_buckets - 2).start); + prop_assert!(first_bucket_end <= min_value, "first bucket {first_bucket_end:?} must be less than {min_value:?}"); + 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:?})"); + } + #[test] fn proptest_log_histogram_invariants(histogram in valid_log_histogram_strategy()) { // 1. Assert that the first bucket always starts at 0 @@ -469,7 +515,7 @@ mod test { required_bucket_count, } => required_bucket_count, }; - assert_eq!(num_buckets, 27549); + assert_eq!(num_buckets, 27291); } #[test] diff --git a/tokio/tests/rt_unstable_metrics.rs b/tokio/tests/rt_unstable_metrics.rs index df05bfbf7ce..9149b0e7c69 100644 --- a/tokio/tests/rt_unstable_metrics.rs +++ b/tokio/tests/rt_unstable_metrics.rs @@ -424,6 +424,29 @@ fn log_histogram() { assert_eq!(N, n); } +#[test] +fn minimal_log_histogram() { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .enable_metrics_poll_time_histogram() + .metrics_poll_time_histogram_configuration(HistogramConfiguration::log( + LogHistogram::builder() + .max_value(Duration::from_millis(4)) + .min_value(Duration::from_micros(20)) + .precision_exact(0), + )) + .build() + .unwrap(); + let metrics = rt.metrics(); + let num_buckets = rt.metrics().poll_time_histogram_num_buckets(); + 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:?})"); + } + assert_eq!(num_buckets, 10); +} + #[test] #[allow(deprecated)] fn legacy_log_histogram() { From ddbdbc32b8145c10b8eeb768ce62bcaaf1799ddb Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Sun, 24 Nov 2024 15:56:36 -0500 Subject: [PATCH 2/3] Misc cleanups of docs & tests --- tokio/src/runtime/builder.rs | 2 +- .../runtime/metrics/histogram/h2_histogram.rs | 48 +++++++++++-------- tokio/tests/rt_unstable_metrics.rs | 9 +++- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/tokio/src/runtime/builder.rs b/tokio/src/runtime/builder.rs index b910da74058..4e0f7b9769b 100644 --- a/tokio/src/runtime/builder.rs +++ b/tokio/src/runtime/builder.rs @@ -1255,7 +1255,7 @@ impl Builder { /// .enable_metrics_poll_time_histogram() /// .metrics_poll_time_histogram_configuration(HistogramConfiguration::log( /// LogHistogram::builder() - /// .max_value(Duration::from_millis(5)) + /// .max_value(Duration::from_millis(4)) /// .min_value(Duration::from_micros(20)) /// // Set `precision_exact` to `0` to match `HistogramScale::Log` /// .precision_exact(0) diff --git a/tokio/src/runtime/metrics/histogram/h2_histogram.rs b/tokio/src/runtime/metrics/histogram/h2_histogram.rs index cdeb6d5ba52..fa0f2dc5f34 100644 --- a/tokio/src/runtime/metrics/histogram/h2_histogram.rs +++ b/tokio/src/runtime/metrics/histogram/h2_histogram.rs @@ -164,9 +164,9 @@ impl LogHistogramBuilder { /// such that `2^-p` is less than `precision`. To set `p` directly, use /// [`LogHistogramBuilder::precision_exact`]. /// - /// 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. + /// Precision controls the size of the "bucket groups" (consecutive buckets with identical + /// ranges). When `p` is 0, each bucket will be twice the size of the previous bucket. To match + /// the behavior of the legacy log histogram implementation, use `builder.precision_exact(0)`. /// /// The default value is 25% (2^-2) /// @@ -174,15 +174,11 @@ impl LogHistogramBuilder { /// less than this will be truncated. /// /// # Panics - /// - `precision` < 0 - /// - `precision` > 1 + /// - `max_error` < 0 + /// - `max_error` > 1 pub fn max_error(mut self, max_error: f64) -> Self { - if max_error < 0.0 { - panic!("precision must be >= 0"); - }; - if max_error > 1.0 { - panic!("precision must be > 1"); - }; + assert!(max_error > 0.0, "max_error must be greater than 0"); + assert!(max_error < 1.0, "max_error must be less than 1"); let mut p = 2; while 2_f64.powf(-1.0 * p as f64) > max_error && p <= MAX_PRECISION { p += 1; @@ -195,16 +191,18 @@ impl LogHistogramBuilder { /// /// The precision (meaning: the ratio `n/bucket_range(n)` for some given `n`) will be `2^-p`. /// - /// 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. + /// Precision controls the number consecutive buckets with identically sized ranges. + /// When `p` is 0, each bucket will be twice the size of the previous bucket (bucket groups are + /// only a single bucket wide). + /// + /// To match the behavior of the legacy implementation ([`HistogramScale::Log`]), use `builder.precision_exact(0)`. /// /// # Panics /// - `p` > 10 + /// + /// [`HistogramScale::Log`]: [crate::runtime::HistogramScale] pub fn precision_exact(mut self, p: u32) -> Self { - if p > MAX_PRECISION { - panic!("precision must be <= 10"); - }; + assert!(p <= MAX_PRECISION, "precision must be <= {MAX_PRECISION}"); self.precision = Some(p); self } @@ -307,7 +305,6 @@ mod test { use super::InvalidHistogramConfiguration; use crate::runtime::metrics::histogram::h2_histogram::LogHistogram; use crate::runtime::metrics::histogram::HistogramType; - use std::time::Duration; #[cfg(not(target_family = "wasm"))] mod proptests { @@ -345,11 +342,20 @@ mod test { let first_bucket_end = Duration::from_nanos(histogram.bucket_range(0).end); let last_bucket_start = Duration::from_nanos(histogram.bucket_range(histogram.num_buckets - 1).start); let second_last_bucket_start = Duration::from_nanos(histogram.bucket_range(histogram.num_buckets - 2).start); - prop_assert!(first_bucket_end <= min_value, "first bucket {first_bucket_end:?} must be less than {min_value:?}"); - prop_assert!(last_bucket_start > max_value, "last bucket start ({last_bucket_start:?} must be at least as big as `max_value` ({max_value:?})"); + prop_assert!( + first_bucket_end <= min_value, + "first bucket {first_bucket_end:?} must be less than {min_value:?}" + ); + 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:?})"); + 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:?})" + ); } #[test] diff --git a/tokio/tests/rt_unstable_metrics.rs b/tokio/tests/rt_unstable_metrics.rs index 9149b0e7c69..85eba07d389 100644 --- a/tokio/tests/rt_unstable_metrics.rs +++ b/tokio/tests/rt_unstable_metrics.rs @@ -439,10 +439,15 @@ fn minimal_log_histogram() { .unwrap(); let metrics = rt.metrics(); let num_buckets = rt.metrics().poll_time_histogram_num_buckets(); - for b in 0..num_buckets { + for b in 1..num_buckets - 1 { let range = metrics.poll_time_histogram_bucket_range(b); let size = range.end - range.start; - println!("bucket {b}: {range:?} (size: {size:?})"); + // Assert the buckets continue doubling in size + assert_eq!( + size, + Duration::from_nanos((1 << (b - 1)) * 16384), + "incorrect range for {b}" + ); } assert_eq!(num_buckets, 10); } From 5f8172c5b863a1644e7fa3070e566ff9bc970d61 Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Thu, 26 Dec 2024 15:24:47 -0500 Subject: [PATCH 3/3] Clarify doc comment --- tokio/src/runtime/builder.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tokio/src/runtime/builder.rs b/tokio/src/runtime/builder.rs index 4e0f7b9769b..c9a47c3862c 100644 --- a/tokio/src/runtime/builder.rs +++ b/tokio/src/runtime/builder.rs @@ -1245,9 +1245,10 @@ impl Builder { /// .unwrap(); /// ``` /// - /// Configure a `LogHistogram` with similar behavior to [`HistogramScale::Log`] + /// When migrating from the legacy histogram ([`HistogramScale::Log`]) and wanting + /// to match the previous behavior, use `precision_exact(0)`. This creates a histogram + /// where each bucket is twice the size of the previous bucket. /// ```rust - /// /// use std::time::Duration; /// use tokio::runtime::{HistogramConfiguration, LogHistogram}; /// let rt = tokio::runtime::Builder::new_current_thread() @@ -1255,8 +1256,8 @@ impl Builder { /// .enable_metrics_poll_time_histogram() /// .metrics_poll_time_histogram_configuration(HistogramConfiguration::log( /// LogHistogram::builder() - /// .max_value(Duration::from_millis(4)) /// .min_value(Duration::from_micros(20)) + /// .max_value(Duration::from_millis(4)) /// // Set `precision_exact` to `0` to match `HistogramScale::Log` /// .precision_exact(0) /// .max_buckets(10)