Update Histogram Builder (#2130)

Author: utpilla

opentelemetry-sdk/src/metrics/meter.rs

@@ -4,9 +4,9 @@ use std::{borrow::Cow, sync::Arc};
 use opentelemetry::{
     global,
     metrics::{
-        noop::NoopAsyncInstrument, Callback, Counter, Gauge, Histogram, InstrumentProvider,
-        MetricsError, ObservableCounter, ObservableGauge, ObservableUpDownCounter, Result,
-        UpDownCounter,
+        noop::NoopAsyncInstrument, Callback, Counter, Gauge, Histogram, HistogramBuilder,
+        InstrumentProvider, MetricsError, ObservableCounter, ObservableGauge,
+        ObservableUpDownCounter, Result, UpDownCounter,
     },
 };
 
@@ -373,28 +373,28 @@ impl InstrumentProvider for SdkMeter {
         Ok(ObservableGauge::new(observable))
     }
 
-    fn f64_histogram(
-        &self,
-        name: Cow<'static, str>,
-        description: Option<Cow<'static, str>>,
-        unit: Option<Cow<'static, str>>,
-    ) -> Result<Histogram<f64>> {
-        validate_instrument_config(name.as_ref(), &unit, self.validation_policy)?;
+    fn f64_histogram(&self, builder: HistogramBuilder<'_, f64>) -> Result<Histogram<f64>> {
+        validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?;
         let p = InstrumentResolver::new(self, &self.f64_resolver);
-        p.lookup(InstrumentKind::Histogram, name, description, unit)
-            .map(|i| Histogram::new(Arc::new(i)))
+        p.lookup(
+            InstrumentKind::Histogram,
+            builder.name,
+            builder.description,
+            builder.unit,
+        )
+        .map(|i| Histogram::new(Arc::new(i)))
     }
 
-    fn u64_histogram(
-        &self,
-        name: Cow<'static, str>,
-        description: Option<Cow<'static, str>>,
-        unit: Option<Cow<'static, str>>,
-    ) -> Result<Histogram<u64>> {
-        validate_instrument_config(name.as_ref(), &unit, self.validation_policy)?;
+    fn u64_histogram(&self, builder: HistogramBuilder<'_, u64>) -> Result<Histogram<u64>> {
+        validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?;
         let p = InstrumentResolver::new(self, &self.u64_resolver);
-        p.lookup(InstrumentKind::Histogram, name, description, unit)
-            .map(|i| Histogram::new(Arc::new(i)))
+        p.lookup(
+            InstrumentKind::Histogram,
+            builder.name,
+            builder.description,
+            builder.unit,
+        )
+        .map(|i| Histogram::new(Arc::new(i)))
     }
 }
 
@@ -524,7 +524,10 @@ where
 mod tests {
     use std::sync::Arc;
 
-    use opentelemetry::metrics::{InstrumentProvider, MeterProvider, MetricsError};
+    use opentelemetry::{
+        global,
+        metrics::{InstrumentProvider, MeterProvider, MetricsError},
+    };
 
     use super::{
         InstrumentValidationPolicy, SdkMeter, INSTRUMENT_NAME_FIRST_ALPHABETIC,
@@ -629,8 +632,14 @@ mod tests {
                     .f64_observable_gauge(name.into(), None, None, Vec::new())
                     .map(|_| ()),
             );
-            assert(meter.f64_histogram(name.into(), None, None).map(|_| ()));
-            assert(meter.u64_histogram(name.into(), None, None).map(|_| ()));
+
+            // Get handle to HistogramBuilder for testing
+            let global_meter = global::meter("test");
+            let histogram_builder_f64 = global_meter.f64_histogram(name);
+            let histogram_builder_u64 = global_meter.u64_histogram(name);
+
+            assert(meter.f64_histogram(histogram_builder_f64).map(|_| ()));
+            assert(meter.u64_histogram(histogram_builder_u64).map(|_| ()));
         }
 
         // (unit, expected error)
@@ -713,16 +722,18 @@ mod tests {
                     .f64_observable_gauge("test".into(), None, unit.clone(), Vec::new())
                     .map(|_| ()),
             );
-            assert(
-                meter
-                    .f64_histogram("test".into(), None, unit.clone())
-                    .map(|_| ()),
-            );
-            assert(
-                meter
-                    .u64_histogram("test".into(), None, unit.clone())
-                    .map(|_| ()),
-            );
+
+            // Get handle to HistogramBuilder for testing
+            let global_meter = global::meter("test");
+            let histogram_builder_f64 = global_meter
+                .f64_histogram("test")
+                .with_unit(unit.clone().unwrap());
+            let histogram_builder_u64 = global_meter
+                .u64_histogram("test")
+                .with_unit(unit.clone().unwrap());
+
+            assert(meter.f64_histogram(histogram_builder_f64).map(|_| ()));
+            assert(meter.u64_histogram(histogram_builder_u64).map(|_| ()));
         }
     }
 }

opentelemetry/src/metrics/instruments/histogram.rs

@@ -1,7 +1,4 @@
-use crate::{
-    metrics::{InstrumentBuilder, MetricsError},
-    KeyValue,
-};
+use crate::KeyValue;
 use core::fmt;
 use std::sync::Arc;
 
@@ -35,23 +32,3 @@ impl<T> Histogram<T> {
         self.0.record(value, attributes)
     }
 }
-
-impl TryFrom<InstrumentBuilder<'_, Histogram<f64>>> for Histogram<f64> {
-    type Error = MetricsError;
-
-    fn try_from(builder: InstrumentBuilder<'_, Histogram<f64>>) -> Result<Self, Self::Error> {
-        builder
-            .instrument_provider
-            .f64_histogram(builder.name, builder.description, builder.unit)
-    }
-}
-
-impl TryFrom<InstrumentBuilder<'_, Histogram<u64>>> for Histogram<u64> {
-    type Error = MetricsError;
-
-    fn try_from(builder: InstrumentBuilder<'_, Histogram<u64>>) -> Result<Self, Self::Error> {
-        builder
-            .instrument_provider
-            .u64_histogram(builder.name, builder.description, builder.unit)
-    }
-}

opentelemetry/src/metrics/instruments/mod.rs

@@ -6,7 +6,7 @@ use std::borrow::Cow;
 use std::marker;
 use std::sync::Arc;
 
-use super::InstrumentProvider;
+use super::{Histogram, InstrumentProvider};
 
 pub(super) mod counter;
 pub(super) mod gauge;
@@ -24,6 +24,94 @@ pub trait AsyncInstrument<T>: Send + Sync {
     fn as_any(&self) -> Arc<dyn Any>;
 }
 
+/// Configuration for building a Histogram.
+#[non_exhaustive]
+pub struct HistogramBuilder<'a, T> {
+    /// Instrument provider is used to create the instrument.
+    pub instrument_provider: &'a dyn InstrumentProvider,
+
+    /// Name of the Histogram.
+    pub name: Cow<'static, str>,
+
+    /// Description of the Histogram.
+    pub description: Option<Cow<'static, str>>,
+
+    /// Unit of the Histogram.
+    pub unit: Option<Cow<'static, str>>,
+
+    // boundaries: Vec<T>,
+    _marker: marker::PhantomData<T>,
+}
+
+impl<'a, T> HistogramBuilder<'a, T> {
+    /// Create a new instrument builder
+    pub(crate) fn new(meter: &'a Meter, name: Cow<'static, str>) -> Self {
+        HistogramBuilder {
+            instrument_provider: meter.instrument_provider.as_ref(),
+            name,
+            description: None,
+            unit: None,
+            _marker: marker::PhantomData,
+        }
+    }
+
+    /// Set the description for this instrument
+    pub fn with_description<S: Into<Cow<'static, str>>>(mut self, description: S) -> Self {
+        self.description = Some(description.into());
+        self
+    }
+
+    /// Set the unit for this instrument.
+    ///
+    /// Unit is case sensitive(`kb` is not the same as `kB`).
+    ///
+    /// Unit must be:
+    /// - ASCII string
+    /// - No longer than 63 characters
+    pub fn with_unit<S: Into<Cow<'static, str>>>(mut self, unit: S) -> Self {
+        self.unit = Some(unit.into());
+        self
+    }
+}
+
+impl<'a> HistogramBuilder<'a, f64> {
+    /// Validate the instrument configuration and creates a new instrument.
+    pub fn try_init(self) -> Result<Histogram<f64>> {
+        self.instrument_provider.f64_histogram(self)
+    }
+
+    /// Creates a new instrument.
+    ///
+    /// Validate the instrument configuration and crates a new instrument.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the instrument cannot be created. Use
+    /// [`try_init`](InstrumentBuilder::try_init) if you want to handle errors.
+    pub fn init(self) -> Histogram<f64> {
+        self.try_init().unwrap()
+    }
+}
+
+impl<'a> HistogramBuilder<'a, u64> {
+    /// Validate the instrument configuration and creates a new instrument.
+    pub fn try_init(self) -> Result<Histogram<u64>> {
+        self.instrument_provider.u64_histogram(self)
+    }
+
+    /// Creates a new instrument.
+    ///
+    /// Validate the instrument configuration and crates a new instrument.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the instrument cannot be created. Use
+    /// [`try_init`](InstrumentBuilder::try_init) if you want to handle errors.
+    pub fn init(self) -> Histogram<u64> {
+        self.try_init().unwrap()
+    }
+}
+
 /// Configuration for building a sync instrument.
 pub struct InstrumentBuilder<'a, T> {
     instrument_provider: &'a dyn InstrumentProvider,
@@ -95,6 +183,20 @@ impl<T> fmt::Debug for InstrumentBuilder<'_, T> {
     }
 }
 
+impl<T> fmt::Debug for HistogramBuilder<'_, T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("HistogramBuilder")
+            .field("name", &self.name)
+            .field("description", &self.description)
+            .field("unit", &self.unit)
+            .field(
+                "kind",
+                &format!("Histogram<{}>", &std::any::type_name::<T>()),
+            )
+            .finish()
+    }
+}
+
 /// A function registered with a [Meter] that makes observations for the
 /// instruments it is registered with.
 ///

opentelemetry/src/metrics/meter.rs

@@ -3,11 +3,13 @@ use std::borrow::Cow;
 use std::sync::Arc;
 
 use crate::metrics::{
-    AsyncInstrumentBuilder, Counter, Gauge, Histogram, InstrumentBuilder, InstrumentProvider,
+    AsyncInstrumentBuilder, Counter, Gauge, InstrumentBuilder, InstrumentProvider,
     ObservableCounter, ObservableGauge, ObservableUpDownCounter, UpDownCounter,
 };
 use crate::KeyValue;
 
+use super::HistogramBuilder;
+
 /// Provides access to named [Meter] instances, for instrumenting an application
 /// or crate.
 pub trait MeterProvider {
@@ -385,19 +387,13 @@ impl Meter {
     }
 
     /// creates an instrument builder for recording a distribution of values.
-    pub fn f64_histogram(
-        &self,
-        name: impl Into<Cow<'static, str>>,
-    ) -> InstrumentBuilder<'_, Histogram<f64>> {
-        InstrumentBuilder::new(self, name.into())
+    pub fn f64_histogram(&self, name: impl Into<Cow<'static, str>>) -> HistogramBuilder<'_, f64> {
+        HistogramBuilder::new(self, name.into())
     }
 
     /// creates an instrument builder for recording a distribution of values.
-    pub fn u64_histogram(
-        &self,
-        name: impl Into<Cow<'static, str>>,
-    ) -> InstrumentBuilder<'_, Histogram<u64>> {
-        InstrumentBuilder::new(self, name.into())
+    pub fn u64_histogram(&self, name: impl Into<Cow<'static, str>>) -> HistogramBuilder<'_, u64> {
+        HistogramBuilder::new(self, name.into())
     }
 }
 

opentelemetry/src/metrics/mod.rs

@@ -17,7 +17,7 @@ pub use instruments::{
     gauge::{Gauge, ObservableGauge, SyncGauge},
     histogram::{Histogram, SyncHistogram},
     up_down_counter::{ObservableUpDownCounter, SyncUpDownCounter, UpDownCounter},
-    AsyncInstrument, AsyncInstrumentBuilder, Callback, InstrumentBuilder,
+    AsyncInstrument, AsyncInstrumentBuilder, Callback, HistogramBuilder, InstrumentBuilder,
 };
 pub use meter::{Meter, MeterProvider};
 
@@ -273,22 +273,12 @@ pub trait InstrumentProvider {
     }
 
     /// creates an instrument for recording a distribution of values.
-    fn f64_histogram(
-        &self,
-        _name: Cow<'static, str>,
-        _description: Option<Cow<'static, str>>,
-        _unit: Option<Cow<'static, str>>,
-    ) -> Result<Histogram<f64>> {
+    fn f64_histogram(&self, _builder: HistogramBuilder<'_, f64>) -> Result<Histogram<f64>> {
         Ok(Histogram::new(Arc::new(noop::NoopSyncInstrument::new())))
     }
 
     /// creates an instrument for recording a distribution of values.
-    fn u64_histogram(
-        &self,
-        _name: Cow<'static, str>,
-        _description: Option<Cow<'static, str>>,
-        _unit: Option<Cow<'static, str>>,
-    ) -> Result<Histogram<u64>> {
+    fn u64_histogram(&self, _builder: HistogramBuilder<'_, u64>) -> Result<Histogram<u64>> {
         Ok(Histogram::new(Arc::new(noop::NoopSyncInstrument::new())))
     }
 }