fix: Further trim public API on views (#2989)

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

Author: cijothomas

opentelemetry-sdk/CHANGELOG.md

@@ -44,24 +44,46 @@ also modified to suppress telemetry before invoking exporters.
   behind feature flag "experimental_metrics_custom_reader".
   [#2928](https://github.com/open-telemetry/opentelemetry-rust/pull/2928)
 
-- TODO: Placeholder for View related changelog. Polish this after all changs done
+- **Views improvements**:
   - Core view functionality is now available by default—users can change the
     name, unit, description, and cardinality limit of a metric via views without
     enabling the `spec_unstable_metrics_views` feature flag. Advanced view
     features, such as custom aggregation or attribute filtering, still require
     the `spec_unstable_metrics_views` feature.
-- Introduced a builder pattern for `Stream` creation to use with "Views".
+  - Removed `new_view()` method and `View` trait. Views can now be added by passing
+    a function with signature `Fn(&Instrument) -> Option<Stream>` to the `with_view`
+    method on `MeterProviderBuilder`.
+- Introduced a builder pattern for `Stream` creation to use with views:
   - Added `StreamBuilder` struct with methods to configure stream properties
   - Added `Stream::builder()` method that returns a new `StreamBuilder`
   - `StreamBuilder::build()` returns `Result<Stream, Box<dyn Error>>` enabling
-    proper validation
-  - Removed `new_view()` on `View`. Views can be instead added by passing anything
-    that implements `View` trait to `with_view` method on `MeterProviderBuilder`.
-    `View` is implemented for `Fn(&Instrument) -> Option<Stream>`, so this can be
-    used to add views.
+    proper validation.
+
+Example of using views to rename a metric:
+
+```rust
+let view_rename = |i: &Instrument| {
+    if i.name() == "my_histogram" {
+        Some(
+            Stream::builder()
+                .with_name("my_histogram_renamed")
+                .build()
+                .unwrap(),
+        )
+    } else {
+        None
+    }
+};
+
+let provider = SdkMeterProvider::builder()
+    // add exporters, set resource etc.
+    .with_view(view_rename)
+    .build();
+```
 
 - *Breaking* `Aggregation` enum moved behind feature flag
-  "spec_unstable_metrics_views". This was only required when using Views.
+  "spec_unstable_metrics_views". This was only required when using advanced view
+  capabilities.
   [#2928](https://github.com/open-telemetry/opentelemetry-rust/pull/2928)
 - *Breaking* change, affecting custom `PushMetricExporter` authors:
   - The `export` method on `PushMetricExporter` now accepts `&ResourceMetrics`

opentelemetry-sdk/benches/metric.rs

@@ -7,7 +7,7 @@ use opentelemetry_sdk::{
     error::OTelSdkResult,
     metrics::{
         data::ResourceMetrics, reader::MetricReader, Aggregation, Instrument, InstrumentKind,
-        ManualReader, Pipeline, SdkMeterProvider, Stream, Temporality, View,
+        ManualReader, Pipeline, SdkMeterProvider, Stream, Temporality,
     },
 };
 use rand::Rng;
@@ -107,7 +107,9 @@ impl MetricReader for SharedReader {
 //                         time:   [643.75 ns 649.05 ns 655.14 ns]
 // Histogram/Record10Attrs1000bounds
 //                         time:   [726.87 ns 736.52 ns 747.09 ns]
-fn bench_counter(view: Option<Box<dyn View>>, temporality: &str) -> (SharedReader, Counter<u64>) {
+type ViewFn = Box<dyn Fn(&Instrument) -> Option<Stream> + Send + Sync + 'static>;
+
+fn bench_counter(view: Option<ViewFn>, temporality: &str) -> (SharedReader, Counter<u64>) {
     let rdr = if temporality == "cumulative" {
         SharedReader(Arc::new(ManualReader::builder().build()))
     } else {
@@ -273,7 +275,7 @@ fn bench_histogram(bound_count: usize) -> (SharedReader, Histogram<u64>) {
     let r = SharedReader(Arc::new(ManualReader::default()));
     let builder = SdkMeterProvider::builder()
         .with_reader(r.clone())
-        .with_view(Box::new(move |i: &Instrument| {
+        .with_view(move |i: &Instrument| {
             if i.name().starts_with("histogram_") {
                 Stream::builder()
                     .with_aggregation(Aggregation::ExplicitBucketHistogram {
@@ -285,7 +287,7 @@ fn bench_histogram(bound_count: usize) -> (SharedReader, Histogram<u64>) {
             } else {
                 None
             }
-        }));
+        });
 
     let mtr = builder.build().meter("test_meter");
     let hist = mtr

opentelemetry-sdk/src/metrics/instrument.rs

@@ -64,12 +64,12 @@ impl InstrumentKind {
         }
     }
 }
-
-/// Describes properties an instrument is created with, used for filtering in
-/// [View](crate::metrics::View)s.
+/// Describes the properties of an instrument at creation, used for filtering in
+/// views. This is utilized in the `with_view` methods on `MeterProviderBuilder`
+/// to customize metric output.
 ///
-/// A reference to `Instrument` is provided to the `view` to select the
-/// instrument(s) for which the [Stream] should be applied.
+/// Users can use a reference to `Instrument` to select which instrument(s) a
+/// [Stream] should be applied to.
 ///
 /// # Example
 ///
@@ -78,7 +78,7 @@ impl InstrumentKind {
 ///
 /// let my_view_change_cardinality = |i: &Instrument| {
 ///     if i.name() == "my_second_histogram" {
-///         // Note: If Stream is invalid, build() will return an error. By
+///         // Note: If Stream is invalid, `build()` will return an error. By
 ///         // calling `.ok()`, any such error is ignored and treated as if the
 ///         // view does not match the instrument. If this is not the desired
 ///         // behavior, consider handling the error explicitly.
@@ -185,8 +185,8 @@ impl StreamBuilder {
     /// Set the stream allowed attribute keys.
     ///
     /// Any attribute recorded for the stream with a key not in this set will be
-    /// dropped. If the set is empty, all attributes will be dropped, if `None` all
-    /// attributes will be kept.
+    /// dropped. If the set is empty, all attributes will be dropped.
+    /// If this method is not used, all attributes will be kept.
     pub fn with_allowed_attribute_keys(
         mut self,
         attribute_keys: impl IntoIterator<Item = Key>,
@@ -256,15 +256,17 @@ fn validate_bucket_boundaries(boundaries: &[f64]) -> Result<(), String> {
     // validate that buckets are sorted and non-duplicate
     for i in 1..boundaries.len() {
         if boundaries[i] <= boundaries[i - 1] {
-            return Err("Bucket boundaries must be sorted and non-duplicate".to_string());
+            return Err(
+                "Bucket boundaries must be sorted and not contain any duplicates".to_string(),
+            );
         }
     }
 
     Ok(())
 }
 
-/// Describes the stream of data an instrument produces. Used in
-/// [View](crate::metrics::View)s to customize the metric output.
+/// Describes the stream of data an instrument produces. Used in `with_view`
+/// methods on `MeterProviderBuilder` to customize the metric output.
 #[derive(Default, Debug)]
 pub struct Stream {
     /// The human-readable identifier of the stream.

opentelemetry-sdk/src/metrics/meter_provider.rs

@@ -18,12 +18,13 @@ use crate::Resource;
 use super::{
     exporter::PushMetricExporter, meter::SdkMeter, noop::NoopMeter,
     periodic_reader::PeriodicReader, pipeline::Pipelines, reader::MetricReader, view::View,
+    Instrument, Stream,
 };
 
 /// Handles the creation and coordination of [Meter]s.
 ///
 /// All `Meter`s created by a `MeterProvider` will be associated with the same
-/// [Resource], have the same [View]s applied to them, and have their produced
+/// [Resource], have the same views applied to them, and have their produced
 /// metric telemetry passed to the configured [MetricReader]s. This is a
 /// clonable handle to the MeterProvider implementation itself, and cloning it
 /// will create a new reference, not a new instance of a MeterProvider. Dropping
@@ -287,14 +288,14 @@ impl MeterProviderBuilder {
         self
     }
 
-    /// Adds a [View] to the [MeterProvider].
+    /// Adds a view to the [MeterProvider].
     ///
     /// Views allow you to customize how metrics are aggregated, renamed, or
     /// otherwise transformed before export, without modifying instrument
     /// definitions in your application or library code.
     ///
-    /// You can pass any type implementing the [`View`] trait, including
-    /// closures matching the pattern `Fn(&Instrument) -> Option<Stream>`. The
+    /// You can pass any function or closure matching the signature
+    /// `Fn(&Instrument) -> Option<Stream> + Send + Sync + 'static`. The
     /// function receives a reference to the [`Instrument`] and can return an
     /// [`Option`] of [`Stream`] to specify how matching instruments should be
     /// exported. Returning `None` means the view does not apply to the given
@@ -348,7 +349,7 @@ impl MeterProviderBuilder {
     ///         // By calling `.ok()`, any such error is ignored and treated as if the view does not match
     ///         // the instrument.
     ///         // If this is not the desired behavior, consider handling the error explicitly.
-    ///         Stream::builder().with_cardinality_limit(100).build().ok()
+    ///         Stream::builder().with_cardinality_limit(0).build().ok()
     ///     } else {
     ///         None
     ///     }
@@ -361,10 +362,12 @@ impl MeterProviderBuilder {
     /// If no views are added, the [MeterProvider] uses the default view.
     ///
     /// [`Instrument`]: crate::metrics::Instrument
-    /// [`View`]: crate::metrics::View
     /// [`Stream`]: crate::metrics::Stream
     /// [`Option`]: core::option::Option
-    pub fn with_view<T: View>(mut self, view: T) -> Self {
+    pub fn with_view<T>(mut self, view: T) -> Self
+    where
+        T: Fn(&Instrument) -> Option<Stream> + Send + Sync + 'static,
+    {
         self.views.push(Arc::new(view));
         self
     }

opentelemetry-sdk/src/metrics/mod.rs

@@ -3,7 +3,7 @@
 //! ## Configuration
 //!
 //! The metrics SDK configuration is stored with each [SdkMeterProvider].
-//! Configuration for [Resource]s, [View]s, and [ManualReader] or
+//! Configuration for [Resource]s, views, and [ManualReader] or
 //! [PeriodicReader] instances can be specified.
 //!
 //! ### Example
@@ -81,7 +81,6 @@ pub use periodic_reader::*;
 pub use pipeline::Pipeline;
 
 pub use instrument::{Instrument, InstrumentKind, Stream, StreamBuilder};
-pub use view::View;
 
 use std::hash::Hash;
 
@@ -113,7 +112,6 @@ mod tests {
     use self::data::{HistogramDataPoint, ScopeMetrics, SumDataPoint};
     use super::data::MetricData;
     use super::internal::Number;
-    use super::view::View;
     use super::*;
     use crate::metrics::data::ResourceMetrics;
     use crate::metrics::internal::AggregatedMetricsAccess;
@@ -940,12 +938,10 @@ mod tests {
         // View drops all attributes.
         let view = |i: &Instrument| {
             if i.name == "my_observable_counter" {
-                Some(
-                    Stream::builder()
-                        .with_allowed_attribute_keys(vec![])
-                        .build()
-                        .unwrap(),
-                )
+                Stream::builder()
+                    .with_allowed_attribute_keys(vec![])
+                    .build()
+                    .ok()
             } else {
                 None
             }
@@ -3314,7 +3310,10 @@ mod tests {
             }
         }
 
-        fn new_with_view<T: View>(temporality: Temporality, view: T) -> Self {
+        fn new_with_view<T>(temporality: Temporality, view: T) -> Self
+        where
+            T: Fn(&Instrument) -> Option<Stream> + Send + Sync + 'static,
+        {
             let exporter = InMemoryMetricExporterBuilder::new().with_temporality(temporality);
             let exporter = exporter.build();
             let meter_provider = SdkMeterProvider::builder()

opentelemetry-sdk/src/metrics/view.rs

@@ -36,9 +36,7 @@ use super::instrument::{Instrument, Stream};
 /// let provider = SdkMeterProvider::builder().with_view(my_view).build();
 /// # drop(provider)
 /// ```
-// TODO: This trait need not be public, if we modify MeterProvider to take a
-// Fn(&Instrument) -> Option<Stream> instead of View.
-pub trait View: Send + Sync + 'static {
+pub(crate) trait View: Send + Sync + 'static {
     /// Defines how data should be collected for certain instruments.
     ///
     /// Return [Stream] to use for matching [Instrument]s,
@@ -54,9 +52,3 @@ where
         self(inst)
     }
 }
-
-impl View for Box<dyn View> {
-    fn match_inst(&self, inst: &Instrument) -> Option<Stream> {
-        (**self).match_inst(inst)
-    }
-}