From c228fd35f631bf32bdd868728f20f246be7e104c Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Wed, 16 Oct 2024 10:09:29 -0400 Subject: [PATCH] CR feedback: improve docs, cleanup implementation, a few renames --- tokio/src/runtime/builder.rs | 30 ++++++++++++++- tokio/src/runtime/metrics/histogram.rs | 38 ++++++++----------- .../runtime/metrics/histogram/h2_histogram.rs | 6 +-- tokio/tests/rt_unstable_metrics.rs | 2 +- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/tokio/src/runtime/builder.rs b/tokio/src/runtime/builder.rs index 502ef832175..802bcb51de9 100644 --- a/tokio/src/runtime/builder.rs +++ b/tokio/src/runtime/builder.rs @@ -1169,7 +1169,7 @@ impl Builder { /// better granularity with low memory usage, use [`LogHistogram`] instead. /// /// # Examples - /// Configure a `LogHistogram` with default configuration: + /// Configure a [`LogHistogram`] with [default configuration]: /// ``` /// use tokio::runtime; /// use tokio::runtime::{HistogramConfiguration, LogHistogram}; @@ -1183,7 +1183,7 @@ impl Builder { /// .unwrap(); /// ``` /// - /// Configure a linear histogram + /// Configure a linear histogram with 100 buckets, each 10μs wide /// ``` /// use tokio::runtime; /// use std::time::Duration; @@ -1198,7 +1198,33 @@ impl Builder { /// .unwrap(); /// ``` /// + /// Configure a [`LogHistogram`] with the following settings: + /// - Measure times from 100ns to 120s + /// - Max error of 0.1 + /// - No more than 1024 buckets + /// ``` + /// use std::time::Duration; + /// use tokio::runtime; + /// use tokio::runtime::{HistogramConfiguration, LogHistogram}; + /// + /// let rt = runtime::Builder::new_multi_thread() + /// .enable_metrics_poll_count_histogram() + /// .metrics_poll_count_histogram_configuration( + /// HistogramConfiguration::log(LogHistogram::builder() + /// .max_value(Duration::from_secs(120)) + /// .min_value(Duration::from_nanos(100)) + /// .max_error(0.1) + /// .max_buckets(1024) + /// .expect("configuration uses 488 buckets") + /// ) + /// ) + /// .build() + /// .unwrap(); + /// ``` + /// + /// /// [`LogHistogram`]: crate::runtime::LogHistogram + /// [default configuration]: crate::runtime::LogHistogramBuilder pub fn metrics_poll_count_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.rs b/tokio/src/runtime/metrics/histogram.rs index d3fa1d5d951..5e612492418 100644 --- a/tokio/src/runtime/metrics/histogram.rs +++ b/tokio/src/runtime/metrics/histogram.rs @@ -15,8 +15,10 @@ pub(crate) struct Histogram { /// The histogram buckets buckets: Box<[MetricAtomicU64]>, - /// Bucket scale, linear or log - configuration: HistogramType, + /// The type of the histogram + /// + /// This handles `fn(bucket) -> Range` and `fn(value) -> bucket` + histogram_type: HistogramType, } #[derive(Debug, Clone)] @@ -98,6 +100,7 @@ cfg_unstable! { pub(crate) enum HistogramType { /// Linear histogram with fixed width buckets Linear(LinearHistogram), + /// Old log histogram where each bucket doubles in size LogLegacy(LegacyLogHistogram), @@ -196,7 +199,7 @@ impl Histogram { } pub(crate) fn bucket_range(&self, bucket: usize) -> Range { - self.configuration.bucket_range(bucket) + self.histogram_type.bucket_range(bucket) } } @@ -206,7 +209,7 @@ impl HistogramBatch { HistogramBatch { buckets, - configuration: histogram.configuration, + configuration: histogram.histogram_type, } } @@ -215,7 +218,7 @@ impl HistogramBatch { } pub(crate) fn submit(&self, histogram: &Histogram) { - debug_assert_eq!(self.configuration, histogram.configuration); + debug_assert_eq!(self.configuration, histogram.histogram_type); debug_assert_eq!(self.buckets.len(), histogram.buckets.len()); for i in 0..self.buckets.len() { @@ -240,33 +243,22 @@ impl HistogramBuilder { } pub(crate) fn legacy_mut(&mut self, f: impl Fn(&mut LegacyBuilder)) { - if let Some(legacy) = &mut self.legacy { - f(legacy) - } else { - let mut legacy = LegacyBuilder::default(); - f(&mut legacy); - self.legacy = Some(legacy) - } + let legacy = self.legacy.get_or_insert_with(|| LegacyBuilder::default()); + f(legacy); } pub(crate) fn build(&self) -> Histogram { - let configuration = match &self.legacy { + let histogram_type = match &self.legacy { Some(legacy) => { - let mut resolution = legacy.resolution; - - assert!(resolution > 0); - - if matches!(legacy.scale, HistogramScale::Log) { - resolution = resolution.next_power_of_two(); - } + assert!(legacy.resolution > 0); match legacy.scale { HistogramScale::Linear => HistogramType::Linear(LinearHistogram { num_buckets: legacy.num_buckets, - bucket_width: resolution, + bucket_width: legacy.resolution, }), HistogramScale::Log => HistogramType::LogLegacy(LegacyLogHistogram { num_buckets: legacy.num_buckets, - first_bucket_width: resolution, + first_bucket_width: legacy.resolution.next_power_of_two(), }), } } @@ -279,7 +271,7 @@ impl HistogramBuilder { .map(|_| MetricAtomicU64::new(0)) .collect::>() .into_boxed_slice(), - configuration, + histogram_type: histogram_type, } } } diff --git a/tokio/src/runtime/metrics/histogram/h2_histogram.rs b/tokio/src/runtime/metrics/histogram/h2_histogram.rs index 9901bb60f29..99a5e429925 100644 --- a/tokio/src/runtime/metrics/histogram/h2_histogram.rs +++ b/tokio/src/runtime/metrics/histogram/h2_histogram.rs @@ -120,7 +120,7 @@ impl LogHistogram { /// The log-scaled histogram implements an H2 histogram where the first bucket covers /// the range from 0 to [`LogHistogramBuilder::min_value`] and the final bucket covers /// [`LogHistogramBuilder::max_value`] to infinity. The precision is bounded to the specified -/// [`LogHistogramBuilder::precision`]. Specifically, the precision is the next smallest value +/// [`LogHistogramBuilder::max_error`]. Specifically, the precision is the next smallest value /// of `2^-p` such that it is smaller than the requested precision. /// /// Depending on the selected parameters, the number of buckets required is variable. To ensure @@ -162,7 +162,7 @@ impl LogHistogramBuilder { /// # Panics /// - `precision` < 0 /// - `precision` > 1 - pub fn precision(mut self, max_error: f64) -> Self { + pub fn max_error(mut self, max_error: f64) -> Self { if max_error < 0.0 { panic!("precision must be >= 0"); }; @@ -460,7 +460,7 @@ mod test { #[test] fn max_buckets_enforcement() { let error = LogHistogram::builder() - .precision(0.001) + .max_error(0.001) .max_buckets(5) .expect_err("this produces way more than 5 buckets"); let num_buckets = match error { diff --git a/tokio/tests/rt_unstable_metrics.rs b/tokio/tests/rt_unstable_metrics.rs index 7b10aab2521..b7dcceef85c 100644 --- a/tokio/tests/rt_unstable_metrics.rs +++ b/tokio/tests/rt_unstable_metrics.rs @@ -400,7 +400,7 @@ fn log_histogram() { LogHistogram::builder() .max_value(Duration::from_secs(60)) .min_value(Duration::from_nanos(100)) - .precision(0.25), + .max_error(0.25), )) .build() .unwrap();