feat: Cleanup Views

Author: cijothomas

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

opentelemetry-sdk/CHANGELOG.md

@@ -53,6 +53,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.

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"]

opentelemetry-sdk/src/metrics/error.rs

@@ -3,30 +3,8 @@ 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.

opentelemetry-sdk/src/metrics/instrument.rs

@@ -84,91 +84,39 @@ impl InstrumentKind {
 /// 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)
-    }
-
-    pub(crate) fn matches_name(&self, other: &Instrument) -> bool {
-        self.name.is_empty() || self.name.as_ref() == other.name.as_ref()
-    }
-
-    pub(crate) fn matches_description(&self, other: &Instrument) -> bool {
-        self.description.is_empty() || self.description.as_ref() == other.description.as_ref()
+    /// Instrument name.
+    pub fn name(&self) -> &str {
+        self.name.as_ref()
     }
 
-    pub(crate) fn matches_kind(&self, other: &Instrument) -> bool {
-        self.kind.is_none() || self.kind == other.kind
+    /// Instrument kind.
+    pub fn kind(&self) -> InstrumentKind {
+        self.kind
     }
 
-    pub(crate) fn matches_unit(&self, other: &Instrument) -> bool {
-        self.unit.is_empty() || self.unit.as_ref() == other.unit.as_ref()
+    /// Instrument unit.
+    pub fn unit(&self) -> &str {
+        self.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 +136,6 @@ impl Instrument {
 ///     .unwrap();
 /// ```
 #[derive(Default, Debug)]
-#[non_exhaustive]
 pub struct StreamBuilder {
     name: Option<Cow<'static, str>>,
     description: Option<Cow<'static, str>>,
@@ -331,8 +278,6 @@ fn validate_bucket_boundaries(boundaries: &[f64]) -> Result<(), String> {
 /// # drop(view);
 /// ```
 #[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>>,

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: kind,
             scope: self.meter.scope.clone(),
         };
 

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.

opentelemetry-sdk/src/metrics/mod.rs

@@ -73,8 +73,7 @@ pub use in_memory_exporter::{InMemoryMetricExporter, InMemoryMetricExporterBuild
 
 #[cfg(feature = "spec_unstable_metrics_views")]
 pub use aggregation::*;
-#[cfg(feature = "spec_unstable_metrics_views")]
-pub use error::{MetricError, MetricResult};
+
 #[cfg(feature = "experimental_metrics_custom_reader")]
 pub use manual_reader::*;
 pub use meter_provider::*;
@@ -84,9 +83,6 @@ pub use pipeline::Pipeline;
 
 pub use instrument::{Instrument, InstrumentKind, Stream, StreamBuilder};
 
-#[cfg(feature = "spec_unstable_metrics_views")]
-pub use view::new_view;
-
 pub use view::View;
 
 use std::hash::Hash;

opentelemetry-sdk/src/metrics/pipeline.rs

@@ -259,10 +259,7 @@ where
         let mut matched = false;
         let mut measures = vec![];
         let mut errs = vec![];
-        let kind = match inst.kind {
-            Some(kind) => kind,
-            None => return Err(MetricError::Other("instrument must have a kind".into())),
-        };
+        let kind = inst.kind;
 
         // The cache will return the same Aggregator instance. Use stream ids to de duplicate.
         let mut seen = HashSet::new();