feat: View cleanups (#2988)

cijothomas · web-flow · commit e123996d8074 · 2025-05-21T16:40:32.000-07:00
diff --git a/examples/metrics-advanced/src/main.rs b/examples/metrics-advanced/src/main.rs
@@ -7,7 +7,7 @@ use std::error::Error;
 fn init_meter_provider() -> opentelemetry_sdk::metrics::SdkMeterProvider {
     // for example 1
     let my_view_rename_and_unit = |i: &Instrument| {
-        if i.name == "my_histogram" {
+        if i.name() == "my_histogram" {
             Some(
                 Stream::builder()
                     .with_name("my_histogram_renamed")
@@ -22,7 +22,7 @@ fn init_meter_provider() -> opentelemetry_sdk::metrics::SdkMeterProvider {
 
     // for example 2
     let my_view_change_cardinality = |i: &Instrument| {
-        if i.name == "my_second_histogram" {
+        if i.name() == "my_second_histogram" {
             // 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
diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md
@@ -55,6 +55,10 @@ also modified to suppress telemetry before invoking exporters.
   - 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.
 
 - *Breaking* `Aggregation` enum moved behind feature flag
   "spec_unstable_metrics_views". This was only required when using Views.
diff --git a/opentelemetry-sdk/Cargo.toml b/opentelemetry-sdk/Cargo.toml
@@ -18,7 +18,6 @@ futures-executor = { workspace = true }
 futures-util = { workspace = true, features = ["std", "sink", "async-await-macro"] }
 percent-encoding = { workspace = true, optional = true }
 rand = { workspace = true, features = ["std", "std_rng", "small_rng", "os_rng", "thread_rng"], optional = true }
-glob = { workspace = true, optional = true }
 serde = { workspace = true, features = ["derive", "rc"], optional = true }
 serde_json = { workspace = true, optional = true }
 thiserror = { workspace = true }
@@ -45,7 +44,7 @@ trace = ["opentelemetry/trace", "rand", "percent-encoding"]
 jaeger_remote_sampler = ["trace", "opentelemetry-http", "http", "serde", "serde_json", "url", "experimental_async_runtime"]
 logs = ["opentelemetry/logs", "serde_json"]
 spec_unstable_logs_enabled = ["logs", "opentelemetry/spec_unstable_logs_enabled"]
-metrics = ["opentelemetry/metrics", "glob"]
+metrics = ["opentelemetry/metrics"]
 testing = ["opentelemetry/testing", "trace", "metrics", "logs", "rt-tokio", "rt-tokio-current-thread", "tokio/macros", "tokio/rt-multi-thread"]
 experimental_async_runtime = []
 rt-tokio = ["tokio", "tokio-stream", "experimental_async_runtime"]
diff --git a/opentelemetry-sdk/benches/metric.rs b/opentelemetry-sdk/benches/metric.rs
@@ -6,8 +6,8 @@ use opentelemetry::{
 use opentelemetry_sdk::{
     error::OTelSdkResult,
     metrics::{
-        data::ResourceMetrics, new_view, reader::MetricReader, Aggregation, Instrument,
-        InstrumentKind, ManualReader, Pipeline, SdkMeterProvider, Stream, Temporality, View,
+        data::ResourceMetrics, reader::MetricReader, Aggregation, Instrument, InstrumentKind,
+        ManualReader, Pipeline, SdkMeterProvider, Stream, Temporality, View,
     },
 };
 use rand::Rng;
@@ -220,16 +220,12 @@ fn counters(c: &mut Criterion) {
     });
 
     let (_, cntr) = bench_counter(
-        Some(
-            new_view(
-                Instrument::new().name("*"),
-                Stream::builder()
-                    .with_allowed_attribute_keys([Key::new("K")])
-                    .build()
-                    .unwrap(),
-            )
-            .unwrap(),
-        ),
+        Some(Box::new(|_i: &Instrument| {
+            Stream::builder()
+                .with_allowed_attribute_keys([Key::new("K")])
+                .build()
+                .ok()
+        })),
         "cumulative",
     );
 
@@ -273,25 +269,24 @@ fn bench_histogram(bound_count: usize) -> (SharedReader, Histogram<u64>) {
     for i in 0..bounds.len() {
         bounds[i] = i * MAX_BOUND / bound_count
     }
-    let view = Some(
-        new_view(
-            Instrument::new().name("histogram_*"),
-            Stream::builder()
-                .with_aggregation(Aggregation::ExplicitBucketHistogram {
-                    boundaries: bounds.iter().map(|&x| x as f64).collect(),
-                    record_min_max: true,
-                })
-                .build()
-                .unwrap(),
-        )
-        .unwrap(),
-    );
 
     let r = SharedReader(Arc::new(ManualReader::default()));
-    let mut builder = SdkMeterProvider::builder().with_reader(r.clone());
-    if let Some(view) = view {
-        builder = builder.with_view(view);
-    }
+    let builder = SdkMeterProvider::builder()
+        .with_reader(r.clone())
+        .with_view(Box::new(move |i: &Instrument| {
+            if i.name().starts_with("histogram_") {
+                Stream::builder()
+                    .with_aggregation(Aggregation::ExplicitBucketHistogram {
+                        boundaries: bounds.iter().map(|&x| x as f64).collect(),
+                        record_min_max: true,
+                    })
+                    .build()
+                    .ok()
+            } else {
+                None
+            }
+        }));
+
     let mtr = builder.build().meter("test_meter");
     let hist = mtr
         .u64_histogram(format!("histogram_{}", bound_count))
diff --git a/opentelemetry-sdk/src/metrics/error.rs b/opentelemetry-sdk/src/metrics/error.rs
@@ -3,30 +3,9 @@ use std::sync::PoisonError;
 use thiserror::Error;
 
 /// A specialized `Result` type for metric operations.
-#[cfg(feature = "spec_unstable_metrics_views")]
-pub type MetricResult<T> = result::Result<T, MetricError>;
-#[cfg(not(feature = "spec_unstable_metrics_views"))]
 pub(crate) type MetricResult<T> = result::Result<T, MetricError>;
 
 /// Errors returned by the metrics API.
-#[cfg(feature = "spec_unstable_metrics_views")]
-#[derive(Error, Debug)]
-#[non_exhaustive]
-pub enum MetricError {
-    /// Other errors not covered by specific cases.
-    #[error("Metrics error: {0}")]
-    Other(String),
-    /// Invalid configuration
-    #[error("Config error {0}")]
-    Config(String),
-    /// Invalid instrument configuration such invalid instrument name, invalid instrument description, invalid instrument unit, etc.
-    /// See [spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#general-characteristics)
-    /// for full list of requirements.
-    #[error("Invalid instrument configuration: {0}")]
-    InvalidInstrumentConfiguration(&'static str),
-}
-
-#[cfg(not(feature = "spec_unstable_metrics_views"))]
 #[derive(Error, Debug)]
 pub(crate) enum MetricError {
     /// Other errors not covered by specific cases.
diff --git a/opentelemetry-sdk/src/metrics/instrument.rs b/opentelemetry-sdk/src/metrics/instrument.rs
@@ -65,110 +65,62 @@ impl InstrumentKind {
     }
 }
 
-/// Describes properties an instrument is created with, also used for filtering
-/// in [View](crate::metrics::View)s.
+/// Describes properties an instrument is created with, used for filtering in
+/// [View](crate::metrics::View)s.
+///
+/// A reference to `Instrument` is provided to the `view` to select the
+/// instrument(s) for which the [Stream] should be applied.
 ///
 /// # Example
 ///
-/// Instruments can be used as criteria for views.
+/// ```rust
+/// use opentelemetry_sdk::metrics::{Instrument, Stream};
 ///
+/// let my_view_change_cardinality = |i: &Instrument| {
+///     if i.name() == "my_second_histogram" {
+///         // 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.
+///         Stream::builder().with_cardinality_limit(2).build().ok()
+///     } else {
+///         None
+///     }
+/// };
 /// ```
-/// use opentelemetry_sdk::metrics::{new_view, Aggregation, Instrument, Stream, StreamBuilder};
-///
-/// let criteria = Instrument::new().name("counter_*");
-/// let mask = Stream::builder()
-///     .with_aggregation(Aggregation::Sum)
-///     .build()
-///     .unwrap();
-///
-/// let view = new_view(criteria, mask);
-/// # drop(view);
-/// ```
-#[derive(Clone, Default, Debug, PartialEq)]
-#[non_exhaustive]
+#[derive(Clone, Debug, PartialEq)]
 pub struct Instrument {
     /// The human-readable identifier of the instrument.
-    pub name: Cow<'static, str>,
+    pub(crate) name: Cow<'static, str>,
     /// describes the purpose of the instrument.
-    pub description: Cow<'static, str>,
+    pub(crate) description: Cow<'static, str>,
     /// The functional group of the instrument.
-    pub kind: Option<InstrumentKind>,
+    pub(crate) kind: InstrumentKind,
     /// Unit is the unit of measurement recorded by the instrument.
-    pub unit: Cow<'static, str>,
+    pub(crate) unit: Cow<'static, str>,
     /// The instrumentation that created the instrument.
-    pub scope: InstrumentationScope,
+    pub(crate) scope: InstrumentationScope,
 }
 
-#[cfg(feature = "spec_unstable_metrics_views")]
 impl Instrument {
-    /// Create a new instrument with default values
-    pub fn new() -> Self {
-        Instrument::default()
-    }
-
-    /// Set the instrument name.
-    pub fn name(mut self, name: impl Into<Cow<'static, str>>) -> Self {
-        self.name = name.into();
-        self
-    }
-
-    /// Set the instrument description.
-    pub fn description(mut self, description: impl Into<Cow<'static, str>>) -> Self {
-        self.description = description.into();
-        self
-    }
-
-    /// Set the instrument unit.
-    pub fn unit(mut self, unit: impl Into<Cow<'static, str>>) -> Self {
-        self.unit = unit.into();
-        self
-    }
-
-    /// Set the instrument scope.
-    pub fn scope(mut self, scope: InstrumentationScope) -> Self {
-        self.scope = scope;
-        self
-    }
-
-    /// empty returns if all fields of i are their default-value.
-    pub(crate) fn is_empty(&self) -> bool {
-        self.name.is_empty()
-            && self.description.is_empty()
-            && self.kind.is_none()
-            && self.unit.is_empty()
-            && self.scope == InstrumentationScope::default()
-    }
-
-    pub(crate) fn matches(&self, other: &Instrument) -> bool {
-        self.matches_name(other)
-            && self.matches_description(other)
-            && self.matches_kind(other)
-            && self.matches_unit(other)
-            && self.matches_scope(other)
+    /// Instrument name.
+    pub fn name(&self) -> &str {
+        self.name.as_ref()
     }
 
-    pub(crate) fn matches_name(&self, other: &Instrument) -> bool {
-        self.name.is_empty() || self.name.as_ref() == other.name.as_ref()
+    /// Instrument kind.
+    pub fn kind(&self) -> InstrumentKind {
+        self.kind
     }
 
-    pub(crate) fn matches_description(&self, other: &Instrument) -> bool {
-        self.description.is_empty() || self.description.as_ref() == other.description.as_ref()
+    /// Instrument unit.
+    pub fn unit(&self) -> &str {
+        self.unit.as_ref()
     }
 
-    pub(crate) fn matches_kind(&self, other: &Instrument) -> bool {
-        self.kind.is_none() || self.kind == other.kind
-    }
-
-    pub(crate) fn matches_unit(&self, other: &Instrument) -> bool {
-        self.unit.is_empty() || self.unit.as_ref() == other.unit.as_ref()
-    }
-
-    pub(crate) fn matches_scope(&self, other: &Instrument) -> bool {
-        (self.scope.name().is_empty() || self.scope.name() == other.scope.name())
-            && (self.scope.version().is_none()
-                || self.scope.version().as_ref() == other.scope.version().as_ref())
-            && (self.scope.schema_url().is_none()
-                || self.scope.schema_url().as_ref() == other.scope.schema_url().as_ref())
+    /// Instrument scope.
+    pub fn scope(&self) -> &InstrumentationScope {
+        &self.scope
     }
 }
 
@@ -188,7 +140,6 @@ impl Instrument {
 ///     .unwrap();
 /// ```
 #[derive(Default, Debug)]
-#[non_exhaustive]
 pub struct StreamBuilder {
     name: Option<Cow<'static, str>>,
     description: Option<Cow<'static, str>>,
@@ -312,27 +263,9 @@ fn validate_bucket_boundaries(boundaries: &[f64]) -> Result<(), String> {
     Ok(())
 }
 
-/// Describes the stream of data an instrument produces.
-///
-/// # Example
-///
-/// Streams can be used as masks in views.
-///
-/// ```
-/// use opentelemetry_sdk::metrics::{new_view, Aggregation, Instrument, Stream};
-///
-/// let criteria = Instrument::new().name("counter_*");
-/// let mask = Stream::builder()
-///     .with_aggregation(Aggregation::Sum)
-///     .build()
-///     .unwrap();
-///
-/// let view = new_view(criteria, mask);
-/// # drop(view);
-/// ```
+/// Describes the stream of data an instrument produces. Used in
+/// [View](crate::metrics::View)s to customize the metric output.
 #[derive(Default, Debug)]
-#[non_exhaustive]
-#[allow(unreachable_pub)]
 pub struct Stream {
     /// The human-readable identifier of the stream.
     pub(crate) name: Option<Cow<'static, str>>,
diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs
@@ -683,7 +683,7 @@ where
             name,
             description: description.unwrap_or_default(),
             unit: unit.unwrap_or_default(),
-            kind: Some(kind),
+            kind,
             scope: self.meter.scope.clone(),
         };
 
diff --git a/opentelemetry-sdk/src/metrics/meter_provider.rs b/opentelemetry-sdk/src/metrics/meter_provider.rs
@@ -308,7 +308,7 @@ impl MeterProviderBuilder {
     /// ```
     /// # use opentelemetry_sdk::metrics::{Stream, Instrument};
     /// let view_rename = |i: &Instrument| {
-    ///     if i.name == "my_counter" {
+    ///     if i.name() == "my_counter" {
     ///         Some(Stream::builder().with_name("my_counter_renamed").build().expect("Stream should be valid"))
     ///     } else {
     ///         None
@@ -324,7 +324,7 @@ impl MeterProviderBuilder {
     /// ```
     /// # use opentelemetry_sdk::metrics::{Stream, Instrument};
     /// let view_change_cardinality = |i: &Instrument| {
-    ///     if i.name == "my_counter" {
+    ///     if i.name() == "my_counter" {
     ///         Some(
     ///             Stream::builder()
     ///                 .with_cardinality_limit(100).build().expect("Stream should be valid"),
@@ -343,7 +343,7 @@ impl MeterProviderBuilder {
     /// ```
     /// # use opentelemetry_sdk::metrics::{Stream, Instrument};
     /// let my_view_change_cardinality = |i: &Instrument| {
-    ///     if i.name == "my_second_histogram" {
+    ///     if i.name() == "my_second_histogram" {
     ///         // Note: If Stream is invalid, build() will return `Error` variant.
     ///         // By calling `.ok()`, any such error is ignored and treated as if the view does not match
     ///         // the instrument.
diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs
diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs
diff --git a/opentelemetry-sdk/src/metrics/view.rs b/opentelemetry-sdk/src/metrics/view.rs
