From 86dd4863ddc8d1add4089e201dcbc02e50159d95 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Wed, 2 Oct 2024 15:37:37 -0700 Subject: [PATCH] Metrics - Instrument Name Validation fixes (#2166) --- opentelemetry-sdk/src/metrics/meter.rs | 720 +++++++------------ opentelemetry-sdk/src/metrics/mod.rs | 64 ++ opentelemetry-sdk/src/metrics/pipeline.rs | 4 + opentelemetry/src/metrics/instruments/mod.rs | 35 +- 4 files changed, 343 insertions(+), 480 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index 975297a112..f1563ccb1d 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -4,9 +4,10 @@ use std::{borrow::Cow, sync::Arc}; use opentelemetry::{ global, metrics::{ - noop::NoopAsyncInstrument, AsyncInstrumentBuilder, Counter, Gauge, Histogram, - HistogramBuilder, InstrumentBuilder, InstrumentProvider, MetricsError, ObservableCounter, - ObservableGauge, ObservableUpDownCounter, Result, UpDownCounter, + noop::{NoopAsyncInstrument, NoopSyncInstrument}, + AsyncInstrumentBuilder, Counter, Gauge, Histogram, HistogramBuilder, InstrumentBuilder, + InstrumentProvider, MetricsError, ObservableCounter, ObservableGauge, + ObservableUpDownCounter, Result, UpDownCounter, }, }; @@ -48,7 +49,6 @@ pub(crate) struct SdkMeter { u64_resolver: Resolver, i64_resolver: Resolver, f64_resolver: Resolver, - validation_policy: InstrumentValidationPolicy, } impl SdkMeter { @@ -61,91 +61,67 @@ impl SdkMeter { u64_resolver: Resolver::new(Arc::clone(&pipes), Arc::clone(&view_cache)), i64_resolver: Resolver::new(Arc::clone(&pipes), Arc::clone(&view_cache)), f64_resolver: Resolver::new(pipes, view_cache), - validation_policy: InstrumentValidationPolicy::HandleGlobalAndIgnore, } } - #[cfg(test)] - fn with_validation_policy(self, validation_policy: InstrumentValidationPolicy) -> Self { - Self { - validation_policy, - ..self + fn create_counter( + &self, + builder: InstrumentBuilder<'_, Counter>, + resolver: &InstrumentResolver<'_, T>, + ) -> Result> + where + T: Number, + { + let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); + if let Err(err) = validation_result { + global::handle_error(err); + return Ok(Counter::new(Arc::new(NoopSyncInstrument::new()))); } - } -} - -#[doc(hidden)] -impl InstrumentProvider for SdkMeter { - fn u64_counter(&self, builder: InstrumentBuilder<'_, Counter>) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.u64_resolver); - p.lookup( - InstrumentKind::Counter, - builder.name, - builder.description, - builder.unit, - None, - ) - .map(|i| Counter::new(Arc::new(i))) - } - fn f64_counter(&self, builder: InstrumentBuilder<'_, Counter>) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.f64_resolver); - p.lookup( - InstrumentKind::Counter, - builder.name, - builder.description, - builder.unit, - None, - ) - .map(|i| Counter::new(Arc::new(i))) + match resolver + .lookup( + InstrumentKind::Counter, + builder.name, + builder.description, + builder.unit, + None, + ) + .map(|i| Counter::new(Arc::new(i))) + { + Ok(counter) => Ok(counter), + Err(err) => { + global::handle_error(err); + Ok(Counter::new(Arc::new(NoopSyncInstrument::new()))) + } + } } - fn u64_observable_counter( + fn create_observable_counter( &self, - builder: AsyncInstrumentBuilder<'_, ObservableCounter, u64>, - ) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.u64_resolver); - let ms = p.measures( - InstrumentKind::ObservableCounter, - builder.name, - builder.description, - builder.unit, - None, - )?; - if ms.is_empty() { + builder: AsyncInstrumentBuilder<'_, ObservableCounter, T>, + resolver: &InstrumentResolver<'_, T>, + ) -> Result> + where + T: Number, + { + let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); + if let Err(err) = validation_result { + global::handle_error(err); return Ok(ObservableCounter::new(Arc::new(NoopAsyncInstrument::new()))); } - let observable = Arc::new(Observable::new(ms)); - - for callback in builder.callbacks { - let cb_inst = Arc::clone(&observable); - self.pipes - .register_callback(move || callback(cb_inst.as_ref())); - } - - Ok(ObservableCounter::new(observable)) - } - - fn f64_observable_counter( - &self, - builder: AsyncInstrumentBuilder<'_, ObservableCounter, f64>, - ) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.f64_resolver); - let ms = p.measures( + let ms = resolver.measures( InstrumentKind::ObservableCounter, builder.name, builder.description, builder.unit, None, )?; + if ms.is_empty() { return Ok(ObservableCounter::new(Arc::new(NoopAsyncInstrument::new()))); } + let observable = Arc::new(Observable::new(ms)); for callback in builder.callbacks { @@ -157,51 +133,30 @@ impl InstrumentProvider for SdkMeter { Ok(ObservableCounter::new(observable)) } - fn i64_up_down_counter( + fn create_observable_updown_counter( &self, - builder: InstrumentBuilder<'_, UpDownCounter>, - ) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.i64_resolver); - p.lookup( - InstrumentKind::UpDownCounter, - builder.name, - builder.description, - builder.unit, - None, - ) - .map(|i| UpDownCounter::new(Arc::new(i))) - } - - fn f64_up_down_counter( - &self, - builder: InstrumentBuilder<'_, UpDownCounter>, - ) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.f64_resolver); - p.lookup( - InstrumentKind::UpDownCounter, - builder.name, - builder.description, - builder.unit, - None, - ) - .map(|i| UpDownCounter::new(Arc::new(i))) - } + builder: AsyncInstrumentBuilder<'_, ObservableUpDownCounter, T>, + resolver: &InstrumentResolver<'_, T>, + ) -> Result> + where + T: Number, + { + let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); + if let Err(err) = validation_result { + global::handle_error(err); + return Ok(ObservableUpDownCounter::new(Arc::new( + NoopAsyncInstrument::new(), + ))); + } - fn i64_observable_up_down_counter( - &self, - builder: AsyncInstrumentBuilder<'_, ObservableUpDownCounter, i64>, - ) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.i64_resolver); - let ms = p.measures( + let ms = resolver.measures( InstrumentKind::ObservableUpDownCounter, builder.name, builder.description, builder.unit, None, )?; + if ms.is_empty() { return Ok(ObservableUpDownCounter::new(Arc::new( NoopAsyncInstrument::new(), @@ -219,23 +174,30 @@ impl InstrumentProvider for SdkMeter { Ok(ObservableUpDownCounter::new(observable)) } - fn f64_observable_up_down_counter( + fn create_observable_gauge( &self, - builder: AsyncInstrumentBuilder<'_, ObservableUpDownCounter, f64>, - ) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.f64_resolver); - let ms = p.measures( - InstrumentKind::ObservableUpDownCounter, + builder: AsyncInstrumentBuilder<'_, ObservableGauge, T>, + resolver: &InstrumentResolver<'_, T>, + ) -> Result> + where + T: Number, + { + let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); + if let Err(err) = validation_result { + global::handle_error(err); + return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new()))); + } + + let ms = resolver.measures( + InstrumentKind::ObservableGauge, builder.name, builder.description, builder.unit, None, )?; + if ms.is_empty() { - return Ok(ObservableUpDownCounter::new(Arc::new( - NoopAsyncInstrument::new(), - ))); + return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new()))); } let observable = Arc::new(Observable::new(ms)); @@ -246,184 +208,218 @@ impl InstrumentProvider for SdkMeter { .register_callback(move || callback(cb_inst.as_ref())); } - Ok(ObservableUpDownCounter::new(observable)) + Ok(ObservableGauge::new(observable)) + } + + fn create_updown_counter( + &self, + builder: InstrumentBuilder<'_, UpDownCounter>, + resolver: &InstrumentResolver<'_, T>, + ) -> Result> + where + T: Number, + { + let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); + if let Err(err) = validation_result { + global::handle_error(err); + return Ok(UpDownCounter::new(Arc::new(NoopSyncInstrument::new()))); + } + + match resolver + .lookup( + InstrumentKind::UpDownCounter, + builder.name, + builder.description, + builder.unit, + None, + ) + .map(|i| UpDownCounter::new(Arc::new(i))) + { + Ok(updown_counter) => Ok(updown_counter), + Err(err) => { + global::handle_error(err); + Ok(UpDownCounter::new(Arc::new(NoopSyncInstrument::new()))) + } + } + } + + fn create_gauge( + &self, + builder: InstrumentBuilder<'_, Gauge>, + resolver: &InstrumentResolver<'_, T>, + ) -> Result> + where + T: Number, + { + let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); + if let Err(err) = validation_result { + global::handle_error(err); + return Ok(Gauge::new(Arc::new(NoopSyncInstrument::new()))); + } + + match resolver + .lookup( + InstrumentKind::Gauge, + builder.name, + builder.description, + builder.unit, + None, + ) + .map(|i| Gauge::new(Arc::new(i))) + { + Ok(gauge) => Ok(gauge), + Err(err) => { + global::handle_error(err); + Ok(Gauge::new(Arc::new(NoopSyncInstrument::new()))) + } + } + } + + fn create_histogram( + &self, + builder: HistogramBuilder<'_, T>, + resolver: &InstrumentResolver<'_, T>, + ) -> Result> + where + T: Number, + { + let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); + if let Err(err) = validation_result { + global::handle_error(err); + return Ok(Histogram::new(Arc::new(NoopSyncInstrument::new()))); + } + + match resolver + .lookup( + InstrumentKind::Histogram, + builder.name, + builder.description, + builder.unit, + builder.boundaries, + ) + .map(|i| Histogram::new(Arc::new(i))) + { + Ok(histogram) => Ok(histogram), + Err(err) => { + global::handle_error(err); + Ok(Histogram::new(Arc::new(NoopSyncInstrument::new()))) + } + } + } +} + +#[doc(hidden)] +impl InstrumentProvider for SdkMeter { + fn u64_counter(&self, builder: InstrumentBuilder<'_, Counter>) -> Result> { + let resolver = InstrumentResolver::new(self, &self.u64_resolver); + self.create_counter(builder, &resolver) + } + + fn f64_counter(&self, builder: InstrumentBuilder<'_, Counter>) -> Result> { + let resolver = InstrumentResolver::new(self, &self.f64_resolver); + self.create_counter(builder, &resolver) + } + + fn u64_observable_counter( + &self, + builder: AsyncInstrumentBuilder<'_, ObservableCounter, u64>, + ) -> Result> { + let resolver = InstrumentResolver::new(self, &self.u64_resolver); + self.create_observable_counter(builder, &resolver) + } + + fn f64_observable_counter( + &self, + builder: AsyncInstrumentBuilder<'_, ObservableCounter, f64>, + ) -> Result> { + let resolver = InstrumentResolver::new(self, &self.f64_resolver); + self.create_observable_counter(builder, &resolver) + } + + fn i64_up_down_counter( + &self, + builder: InstrumentBuilder<'_, UpDownCounter>, + ) -> Result> { + let resolver = InstrumentResolver::new(self, &self.i64_resolver); + self.create_updown_counter(builder, &resolver) + } + + fn f64_up_down_counter( + &self, + builder: InstrumentBuilder<'_, UpDownCounter>, + ) -> Result> { + let resolver = InstrumentResolver::new(self, &self.f64_resolver); + self.create_updown_counter(builder, &resolver) + } + + fn i64_observable_up_down_counter( + &self, + builder: AsyncInstrumentBuilder<'_, ObservableUpDownCounter, i64>, + ) -> Result> { + let resolver = InstrumentResolver::new(self, &self.i64_resolver); + self.create_observable_updown_counter(builder, &resolver) + } + + fn f64_observable_up_down_counter( + &self, + builder: AsyncInstrumentBuilder<'_, ObservableUpDownCounter, f64>, + ) -> Result> { + let resolver = InstrumentResolver::new(self, &self.f64_resolver); + self.create_observable_updown_counter(builder, &resolver) } fn u64_gauge(&self, builder: InstrumentBuilder<'_, Gauge>) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.u64_resolver); - p.lookup( - InstrumentKind::Gauge, - builder.name, - builder.description, - builder.unit, - None, - ) - .map(|i| Gauge::new(Arc::new(i))) + let resolver = InstrumentResolver::new(self, &self.u64_resolver); + self.create_gauge(builder, &resolver) } fn f64_gauge(&self, builder: InstrumentBuilder<'_, Gauge>) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.f64_resolver); - p.lookup( - InstrumentKind::Gauge, - builder.name, - builder.description, - builder.unit, - None, - ) - .map(|i| Gauge::new(Arc::new(i))) + let resolver = InstrumentResolver::new(self, &self.f64_resolver); + self.create_gauge(builder, &resolver) } fn i64_gauge(&self, builder: InstrumentBuilder<'_, Gauge>) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.i64_resolver); - p.lookup( - InstrumentKind::Gauge, - builder.name, - builder.description, - builder.unit, - None, - ) - .map(|i| Gauge::new(Arc::new(i))) + let resolver = InstrumentResolver::new(self, &self.i64_resolver); + self.create_gauge(builder, &resolver) } fn u64_observable_gauge( &self, builder: AsyncInstrumentBuilder<'_, ObservableGauge, u64>, ) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.u64_resolver); - let ms = p.measures( - InstrumentKind::ObservableGauge, - builder.name, - builder.description, - builder.unit, - None, - )?; - if ms.is_empty() { - return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new()))); - } - - let observable = Arc::new(Observable::new(ms)); - - for callback in builder.callbacks { - let cb_inst = Arc::clone(&observable); - self.pipes - .register_callback(move || callback(cb_inst.as_ref())); - } - - Ok(ObservableGauge::new(observable)) + let resolver = InstrumentResolver::new(self, &self.u64_resolver); + self.create_observable_gauge(builder, &resolver) } fn i64_observable_gauge( &self, builder: AsyncInstrumentBuilder<'_, ObservableGauge, i64>, ) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.i64_resolver); - let ms = p.measures( - InstrumentKind::ObservableGauge, - builder.name, - builder.description, - builder.unit, - None, - )?; - if ms.is_empty() { - return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new()))); - } - - let observable = Arc::new(Observable::new(ms)); - - for callback in builder.callbacks { - let cb_inst = Arc::clone(&observable); - self.pipes - .register_callback(move || callback(cb_inst.as_ref())); - } - - Ok(ObservableGauge::new(observable)) + let resolver = InstrumentResolver::new(self, &self.i64_resolver); + self.create_observable_gauge(builder, &resolver) } fn f64_observable_gauge( &self, builder: AsyncInstrumentBuilder<'_, ObservableGauge, f64>, ) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.f64_resolver); - let ms = p.measures( - InstrumentKind::ObservableGauge, - builder.name, - builder.description, - builder.unit, - None, - )?; - if ms.is_empty() { - return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new()))); - } - - let observable = Arc::new(Observable::new(ms)); - - for callback in builder.callbacks { - let cb_inst = Arc::clone(&observable); - self.pipes - .register_callback(move || callback(cb_inst.as_ref())); - } - - Ok(ObservableGauge::new(observable)) + let resolver = InstrumentResolver::new(self, &self.f64_resolver); + self.create_observable_gauge(builder, &resolver) } fn f64_histogram(&self, builder: HistogramBuilder<'_, f64>) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.f64_resolver); - p.lookup( - InstrumentKind::Histogram, - builder.name, - builder.description, - builder.unit, - builder.boundaries, - ) - .map(|i| Histogram::new(Arc::new(i))) + let resolver = InstrumentResolver::new(self, &self.f64_resolver); + self.create_histogram(builder, &resolver) } fn u64_histogram(&self, builder: HistogramBuilder<'_, u64>) -> Result> { - validate_instrument_config(builder.name.as_ref(), &builder.unit, self.validation_policy)?; - let p = InstrumentResolver::new(self, &self.u64_resolver); - p.lookup( - InstrumentKind::Histogram, - builder.name, - builder.description, - builder.unit, - builder.boundaries, - ) - .map(|i| Histogram::new(Arc::new(i))) + let resolver = InstrumentResolver::new(self, &self.u64_resolver); + self.create_histogram(builder, &resolver) } } -/// Validation policy for instrument -#[derive(Clone, Copy)] -enum InstrumentValidationPolicy { - HandleGlobalAndIgnore, - /// Currently only for test - #[cfg(test)] - Strict, -} - -fn validate_instrument_config( - name: &str, - unit: &Option>, - policy: InstrumentValidationPolicy, -) -> Result<()> { - match validate_instrument_name(name).and_then(|_| validate_instrument_unit(unit)) { - Ok(_) => Ok(()), - Err(err) => match policy { - InstrumentValidationPolicy::HandleGlobalAndIgnore => { - global::handle_error(err); - Ok(()) - } - #[cfg(test)] - InstrumentValidationPolicy::Strict => Err(err), - }, - } +fn validate_instrument_config(name: &str, unit: &Option>) -> Result<()> { + validate_instrument_name(name).and_then(|_| validate_instrument_unit(unit)) } fn validate_instrument_name(name: &str) -> Result<()> { @@ -525,42 +521,18 @@ where #[cfg(test)] mod tests { - use std::{borrow::Cow, sync::Arc}; + use std::borrow::Cow; - use opentelemetry::{ - global, - metrics::{InstrumentProvider, MeterProvider, MetricsError}, - }; + use opentelemetry::metrics::MetricsError; use super::{ - InstrumentValidationPolicy, SdkMeter, INSTRUMENT_NAME_FIRST_ALPHABETIC, + validate_instrument_name, validate_instrument_unit, INSTRUMENT_NAME_FIRST_ALPHABETIC, INSTRUMENT_NAME_INVALID_CHAR, INSTRUMENT_NAME_LENGTH, INSTRUMENT_UNIT_INVALID_CHAR, INSTRUMENT_UNIT_LENGTH, }; - use crate::{ - metrics::{pipeline::Pipelines, SdkMeterProvider}, - Resource, Scope, - }; #[test] - #[ignore = "See issue https://github.com/open-telemetry/opentelemetry-rust/issues/1699"] - fn test_instrument_creation() { - let provider = SdkMeterProvider::builder().build(); - let meter = provider.meter("test"); - assert!(meter.u64_counter("test").try_init().is_ok()); - let result = meter.u64_counter("test with invalid name").try_init(); - // this assert fails, as result is always ok variant. - assert!(result.is_err()); - } - - #[test] - fn test_instrument_config_validation() { - // scope and pipelines are not related to test - let meter = SdkMeter::new( - Scope::default(), - Arc::new(Pipelines::new(Resource::default(), Vec::new(), Vec::new())), - ) - .with_validation_policy(InstrumentValidationPolicy::Strict); + fn instrument_name_validation() { // (name, expected error) let instrument_name_test_cases = vec![ ("validateName", ""), @@ -585,84 +557,12 @@ mod tests { } }; - // Get handle to InstrumentBuilder for testing - let global_meter = global::meter("test"); - let counter_builder_u64 = global_meter.u64_counter(name); - let counter_builder_f64 = global_meter.f64_counter(name); - - // Get handle to AsyncInstrumentBuilder for testing - let observable_counter_u64 = global_meter.u64_observable_counter(name); - let observable_counter_f64 = global_meter.f64_observable_counter(name); - - assert(meter.u64_counter(counter_builder_u64).map(|_| ())); - assert(meter.f64_counter(counter_builder_f64).map(|_| ())); - assert( - meter - .u64_observable_counter(observable_counter_u64) - .map(|_| ()), - ); - assert( - meter - .f64_observable_counter(observable_counter_f64) - .map(|_| ()), - ); - - // Get handle to InstrumentBuilder for testing - let up_down_counter_builder_i64 = global_meter.i64_up_down_counter(name); - let up_down_counter_builder_f64 = global_meter.f64_up_down_counter(name); - - assert( - meter - .i64_up_down_counter(up_down_counter_builder_i64) - .map(|_| ()), - ); - assert( - meter - .f64_up_down_counter(up_down_counter_builder_f64) - .map(|_| ()), - ); - - // Get handle to AsyncInstrumentBuilder for testing - let observable_up_down_counter_i64 = global_meter.i64_observable_up_down_counter(name); - let observable_up_down_counter_f64 = global_meter.f64_observable_up_down_counter(name); - - assert( - meter - .i64_observable_up_down_counter(observable_up_down_counter_i64) - .map(|_| ()), - ); - assert( - meter - .f64_observable_up_down_counter(observable_up_down_counter_f64) - .map(|_| ()), - ); - - // Get handle to InstrumentBuilder for testing - let gauge_builder_u64 = global_meter.u64_gauge(name); - let gauge_builder_f64 = global_meter.f64_gauge(name); - let gauge_builder_i64 = global_meter.i64_gauge(name); - - assert(meter.u64_gauge(gauge_builder_u64).map(|_| ())); - assert(meter.f64_gauge(gauge_builder_f64).map(|_| ())); - assert(meter.i64_gauge(gauge_builder_i64).map(|_| ())); - - // Get handle to AsyncInstrumentBuilder for testing - let observable_gauge_u64 = global_meter.u64_observable_gauge(name); - let observable_gauge_i64 = global_meter.i64_observable_gauge(name); - let observable_gauge_f64 = global_meter.f64_observable_gauge(name); - - assert(meter.u64_observable_gauge(observable_gauge_u64).map(|_| ())); - assert(meter.i64_observable_gauge(observable_gauge_i64).map(|_| ())); - assert(meter.f64_observable_gauge(observable_gauge_f64).map(|_| ())); - - // Get handle to HistogramBuilder for testing - 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(|_| ())); + assert(validate_instrument_name(name).map(|_| ())); } + } + #[test] + fn instrument_unit_validation() { // (unit, expected error) let instrument_unit_test_cases = vec![ ( @@ -689,101 +589,7 @@ mod tests { }; let unit: Option> = Some(unit.into()); - // Get handle to InstrumentBuilder for testing - let global_meter = global::meter("test"); - let counter_builder_u64 = global_meter - .u64_counter("test") - .with_unit(unit.clone().unwrap()); - let counter_builder_f64 = global_meter - .f64_counter("test") - .with_unit(unit.clone().unwrap()); - - assert(meter.u64_counter(counter_builder_u64).map(|_| ())); - assert(meter.f64_counter(counter_builder_f64).map(|_| ())); - - // Get handle to AsyncInstrumentBuilder for testing - let observable_counter_u64 = global_meter - .u64_observable_counter("test") - .with_unit(unit.clone().unwrap()); - let observable_counter_f64 = global_meter - .f64_observable_counter("test") - .with_unit(unit.clone().unwrap()); - - assert( - meter - .u64_observable_counter(observable_counter_u64) - .map(|_| ()), - ); - assert( - meter - .f64_observable_counter(observable_counter_f64) - .map(|_| ()), - ); - - // Get handle to InstrumentBuilder for testing - let up_down_counter_builder_i64 = global_meter - .i64_up_down_counter("test") - .with_unit(unit.clone().unwrap()); - let up_down_counter_builder_f64 = global_meter - .f64_up_down_counter("test") - .with_unit(unit.clone().unwrap()); - - assert( - meter - .i64_up_down_counter(up_down_counter_builder_i64) - .map(|_| ()), - ); - - assert( - meter - .f64_up_down_counter(up_down_counter_builder_f64) - .map(|_| ()), - ); - - // Get handle to AsyncInstrumentBuilder for testing - let observable_up_down_counter_i64 = global_meter - .i64_observable_up_down_counter("test") - .with_unit(unit.clone().unwrap()); - let observable_up_down_counter_f64 = global_meter - .f64_observable_up_down_counter("test") - .with_unit(unit.clone().unwrap()); - - assert( - meter - .i64_observable_up_down_counter(observable_up_down_counter_i64) - .map(|_| ()), - ); - assert( - meter - .f64_observable_up_down_counter(observable_up_down_counter_f64) - .map(|_| ()), - ); - - // Get handle to AsyncInstrumentBuilder for testing - let observable_gauge_u64 = global_meter - .u64_observable_gauge("test") - .with_unit(unit.clone().unwrap()); - let observable_gauge_i64 = global_meter - .i64_observable_gauge("test") - .with_unit(unit.clone().unwrap()); - let observable_gauge_f64 = global_meter - .f64_observable_gauge("test") - .with_unit(unit.clone().unwrap()); - - assert(meter.u64_observable_gauge(observable_gauge_u64).map(|_| ())); - assert(meter.i64_observable_gauge(observable_gauge_i64).map(|_| ())); - assert(meter.f64_observable_gauge(observable_gauge_f64).map(|_| ())); - - // Get handle to HistogramBuilder for testing - 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(|_| ())); + assert(validate_instrument_unit(&unit).map(|_| ())); } } } diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index a9b61622e3..8b1f33370a 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -149,6 +149,61 @@ mod tests { // "multi_thread" tokio flavor must be used else flush won't // be able to make progress! + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + async fn invalid_instrument_config_noops() { + // Run this test with stdout enabled to see output. + // cargo test invalid_instrument_config_noops --features=testing -- --nocapture + let invalid_instrument_names = vec![ + "_startWithNoneAlphabet", + "utf8char锈", + "a".repeat(256).leak(), + "invalid name", + ]; + for name in invalid_instrument_names { + let test_context = TestContext::new(Temporality::Cumulative); + let counter = test_context.meter().u64_counter(name).init(); + counter.add(1, &[]); + + let up_down_counter = test_context.meter().i64_up_down_counter(name).init(); + up_down_counter.add(1, &[]); + + let gauge = test_context.meter().f64_gauge(name).init(); + gauge.record(1.9, &[]); + + let histogram = test_context.meter().f64_histogram(name).init(); + histogram.record(1.0, &[]); + + let _observable_counter = test_context + .meter() + .u64_observable_counter(name) + .with_callback(move |observer| { + observer.observe(1, &[]); + }) + .init(); + + let _observable_gauge = test_context + .meter() + .f64_observable_gauge(name) + .with_callback(move |observer| { + observer.observe(1.0, &[]); + }) + .init(); + + let _observable_up_down_counter = test_context + .meter() + .i64_observable_up_down_counter(name) + .with_callback(move |observer| { + observer.observe(1, &[]); + }) + .init(); + + test_context.flush_metrics(); + + // As instrument name is invalid, no metrics should be exported + test_context.check_no_metrics(); + } + } + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn counter_aggregation_delta() { // Run this test with stdout enabled to see output. @@ -2368,6 +2423,15 @@ mod tests { self.exporter.reset(); } + fn check_no_metrics(&self) { + let resource_metrics = self + .exporter + .get_finished_metrics() + .expect("metrics expected to be exported"); // TODO: Need to fix InMemoryMetricsExporter to return None. + + assert!(resource_metrics.is_empty(), "no metrics should be exported"); + } + fn get_aggregation( &mut self, counter_name: &str, diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs index e3b2b43729..a140f65dec 100644 --- a/opentelemetry-sdk/src/metrics/pipeline.rs +++ b/opentelemetry-sdk/src/metrics/pipeline.rs @@ -709,6 +709,10 @@ where } if errs.is_empty() { + if measures.is_empty() { + // TODO: Emit internal log that measurements from the instrument + // are being dropped due to view configuration + } Ok(measures) } else { Err(MetricsError::Other(format!("{errs:?}"))) diff --git a/opentelemetry/src/metrics/instruments/mod.rs b/opentelemetry/src/metrics/instruments/mod.rs index 44c4c72f0b..3922353ab4 100644 --- a/opentelemetry/src/metrics/instruments/mod.rs +++ b/opentelemetry/src/metrics/instruments/mod.rs @@ -96,12 +96,9 @@ impl<'a> HistogramBuilder<'a, f64> { /// 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. + /// Validates the instrument configuration and creates a new instrument. In + /// case of invalid configuration, an instrument that is no-op is returned + /// and an error is logged using internal logging. pub fn init(self) -> Histogram { self.try_init().unwrap() } @@ -115,12 +112,9 @@ impl<'a> HistogramBuilder<'a, u64> { /// 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. + /// Validates the instrument configuration and creates a new instrument. In + /// case of invalid configuration, an instrument that is no-op is returned + /// and an error is logged using internal logging. pub fn init(self) -> Histogram { self.try_init().unwrap() } @@ -184,12 +178,9 @@ where /// 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. + /// Validates the instrument configuration and creates a new instrument. In + /// case of invalid configuration, an instrument that is no-op is returned + /// and an error is logged using internal logging. pub fn init(self) -> T { T::try_from(self).unwrap() } @@ -305,12 +296,10 @@ where /// Creates a new instrument. /// - /// Validate the instrument configuration and creates a new instrument. - /// - /// # Panics /// - /// Panics if the instrument cannot be created. Use - /// [`try_init`](InstrumentBuilder::try_init) if you want to handle errors. + /// Validates the instrument configuration and creates a new instrument. In + /// case of invalid configuration, an instrument that is no-op is returned + /// and an error is logged using internal logging. pub fn init(self) -> I { I::try_from(self).unwrap() }