CR feedback: improve docs, cleanup implementation, a few renames

Author: rcoh

spellcheck.dic

@@ -14,14 +14,16 @@
 100ms
 100ns
 10ms
-12.5%
+10μs
 ~12
+120s
+12.5%
 ±1m
 ±1ms
 1ms
 1s
-250ms
 25%
+250ms
 2x
 ~4
 443
@@ -119,11 +121,11 @@ GID
 goroutines
 Growable
 gzip
-hashmaps
 H2
+hashmaps
 HashMaps
-HdrHistogram
 hashsets
+HdrHistogram
 ie
 Illumos
 impl

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

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<u64> {
-        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::<Vec<_>>()
                 .into_boxed_slice(),
-            configuration,
+            histogram_type: histogram_type,
         }
     }
 }

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 {

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();