From 704b848c313a6496d4d033e1a5f34761ab970e02 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 8 Oct 2024 16:53:25 -0700 Subject: [PATCH 01/17] initial commit --- .../metrics/internal/exponential_histogram.rs | 14 ++++-- opentelemetry-sdk/src/metrics/internal/mod.rs | 7 ++- .../src/metrics/manual_reader.rs | 6 +-- opentelemetry-sdk/src/metrics/meter.rs | 27 ++++++----- .../src/metrics/meter_provider.rs | 5 +-- .../src/metrics/periodic_reader.rs | 21 +++++---- opentelemetry-sdk/src/metrics/pipeline.rs | 19 ++++---- opentelemetry-sdk/src/metrics/view.rs | 28 +++++++----- opentelemetry/src/global/internal_logging.rs | 45 ++++++++++++++++++- 9 files changed, 112 insertions(+), 60 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs index c23b441663..675a224348 100644 --- a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs +++ b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs @@ -1,7 +1,7 @@ use std::{collections::HashMap, f64::consts::LOG2_E, sync::Mutex, time::SystemTime}; use once_cell::sync::Lazy; -use opentelemetry::{metrics::MetricsError, KeyValue}; +use opentelemetry::{otel_error, KeyValue}; use crate::{ metrics::data::{self, Aggregation, Temporality}, @@ -100,9 +100,15 @@ impl ExpoHistogramDataPoint { if (self.scale - scale_delta as i8) < EXPO_MIN_SCALE { // With a scale of -10 there is only two buckets for the whole range of f64 values. // This can only happen if there is a max size of 1. - opentelemetry::global::handle_error(MetricsError::Other( - "exponential histogram scale underflow".into(), - )); + otel_error!( + name: "ExpoHistogramDataPoint.Scale.Underflow", + current_scale = self.scale, + scale_delta = scale_delta, + max_size = self.max_size, + min_scale = EXPO_MIN_SCALE, + record_min_max = self.record_min_max, + record_sum = self.record_sum + ); return; } // Downscale diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index 7a79fe4653..b7c8fed68f 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -17,7 +17,7 @@ pub(crate) use aggregate::{AggregateBuilder, ComputeAggregation, Measure}; pub(crate) use exponential_histogram::{EXPO_MAX_SCALE, EXPO_MIN_SCALE}; use once_cell::sync::Lazy; use opentelemetry::metrics::MetricsError; -use opentelemetry::{global, otel_warn, KeyValue}; +use opentelemetry::{otel_error, KeyValue}; use crate::metrics::AttributeSet; @@ -146,9 +146,8 @@ impl, T: Number, O: Operation> ValueMap { let new_tracker = AU::new_atomic_tracker(self.buckets_count); O::update_tracker(&new_tracker, measurement, index); trackers.insert(STREAM_OVERFLOW_ATTRIBUTES.clone(), Arc::new(new_tracker)); - global::handle_error(MetricsError::Other("Warning: Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into())); - otel_warn!( name: "ValueMap.measure", - message = "Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged." + otel_error!( name: "ValueMap.Measure.Overflow", + error = MetricsError::Other("Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into()) ); } } diff --git a/opentelemetry-sdk/src/metrics/manual_reader.rs b/opentelemetry-sdk/src/metrics/manual_reader.rs index c3e7860e64..4d880a0f14 100644 --- a/opentelemetry-sdk/src/metrics/manual_reader.rs +++ b/opentelemetry-sdk/src/metrics/manual_reader.rs @@ -4,8 +4,8 @@ use std::{ }; use opentelemetry::{ - global, metrics::{MetricsError, Result}, + otel_error, }; use super::{ @@ -84,9 +84,7 @@ impl MetricReader for ManualReader { if inner.sdk_producer.is_none() { inner.sdk_producer = Some(pipeline); } else { - global::handle_error(MetricsError::Config( - "duplicate reader registration, did not register manual reader".into(), - )) + otel_error!(name: "ManualReader.RegisterPipeline.DuplicateRegistration"); } }); } diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index f1563ccb1d..5382b59377 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -2,13 +2,13 @@ use core::fmt; use std::{borrow::Cow, sync::Arc}; use opentelemetry::{ - global, metrics::{ noop::{NoopAsyncInstrument, NoopSyncInstrument}, AsyncInstrumentBuilder, Counter, Gauge, Histogram, HistogramBuilder, InstrumentBuilder, InstrumentProvider, MetricsError, ObservableCounter, ObservableGauge, ObservableUpDownCounter, Result, UpDownCounter, }, + otel_error, }; use crate::instrumentation::Scope; @@ -74,7 +74,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateCounter.ValidationError", error = err); return Ok(Counter::new(Arc::new(NoopSyncInstrument::new()))); } @@ -90,7 +90,7 @@ impl SdkMeter { { Ok(counter) => Ok(counter), Err(err) => { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateCounter.Error", error = err); Ok(Counter::new(Arc::new(NoopSyncInstrument::new()))) } } @@ -106,7 +106,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateObservableCounter.ValidationError", error = err); return Ok(ObservableCounter::new(Arc::new(NoopAsyncInstrument::new()))); } @@ -119,6 +119,7 @@ impl SdkMeter { )?; if ms.is_empty() { + otel_error!(name: "SdkMeter.CreateObservableCounter.Error", error = MetricsError::Other("no measures found".into())); return Ok(ObservableCounter::new(Arc::new(NoopAsyncInstrument::new()))); } @@ -143,7 +144,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateObservableUpDownCounter.ValidationError", error = err); return Ok(ObservableUpDownCounter::new(Arc::new( NoopAsyncInstrument::new(), ))); @@ -158,6 +159,7 @@ impl SdkMeter { )?; if ms.is_empty() { + otel_error!(name: "SdkMeter.CreateObservableUpDownCounter.Error", error = MetricsError::Other("no measures found".into())); return Ok(ObservableUpDownCounter::new(Arc::new( NoopAsyncInstrument::new(), ))); @@ -184,7 +186,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateObservableGauge.ValidationError", error = err); return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new()))); } @@ -197,6 +199,7 @@ impl SdkMeter { )?; if ms.is_empty() { + otel_error!(name: "SdkMeter.CreateObservableGauge.Error", error = MetricsError::Other("no measures found".into())); return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new()))); } @@ -221,7 +224,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateUpDownCounter.ValidationError", error = err); return Ok(UpDownCounter::new(Arc::new(NoopSyncInstrument::new()))); } @@ -237,7 +240,7 @@ impl SdkMeter { { Ok(updown_counter) => Ok(updown_counter), Err(err) => { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateUpDownCounter.Error", error = err); Ok(UpDownCounter::new(Arc::new(NoopSyncInstrument::new()))) } } @@ -253,7 +256,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateGauge.ValidationError", error = err); return Ok(Gauge::new(Arc::new(NoopSyncInstrument::new()))); } @@ -269,7 +272,7 @@ impl SdkMeter { { Ok(gauge) => Ok(gauge), Err(err) => { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateGauge.Error", error = err); Ok(Gauge::new(Arc::new(NoopSyncInstrument::new()))) } } @@ -285,7 +288,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateHistogram.ValidationError", error = err); return Ok(Histogram::new(Arc::new(NoopSyncInstrument::new()))); } @@ -301,7 +304,7 @@ impl SdkMeter { { Ok(histogram) => Ok(histogram), Err(err) => { - global::handle_error(err); + otel_error!(name: "SdkMeter.CreateHistogram.Error", error = err); Ok(Histogram::new(Arc::new(NoopSyncInstrument::new()))) } } diff --git a/opentelemetry-sdk/src/metrics/meter_provider.rs b/opentelemetry-sdk/src/metrics/meter_provider.rs index 49ad98bed6..88c11c393a 100644 --- a/opentelemetry-sdk/src/metrics/meter_provider.rs +++ b/opentelemetry-sdk/src/metrics/meter_provider.rs @@ -8,9 +8,8 @@ use std::{ }; use opentelemetry::{ - global, metrics::{noop::NoopMeter, Meter, MeterProvider, MetricsError, Result}, - KeyValue, + otel_error, KeyValue, }; use crate::{instrumentation::Scope, Resource}; @@ -137,7 +136,7 @@ impl Drop for SdkMeterProviderInner { // shutdown(), then we don't need to call shutdown again. if !self.is_shutdown.load(Ordering::Relaxed) { if let Err(err) = self.shutdown() { - global::handle_error(err); + otel_error!(name: "SdkMeterProvider.Drop.ShutdownError", error = err); } } } diff --git a/opentelemetry-sdk/src/metrics/periodic_reader.rs b/opentelemetry-sdk/src/metrics/periodic_reader.rs index 30604acc48..18640c1a3d 100644 --- a/opentelemetry-sdk/src/metrics/periodic_reader.rs +++ b/opentelemetry-sdk/src/metrics/periodic_reader.rs @@ -12,7 +12,6 @@ use futures_util::{ StreamExt, }; use opentelemetry::{ - global, metrics::{MetricsError, Result}, otel_error, }; @@ -264,20 +263,26 @@ impl PeriodicReaderWorker { match message { Message::Export => { if let Err(err) = self.collect_and_export().await { - global::handle_error(err) + otel_error!(name: "PeriodicReaderWorker.Export.Error",error = err); } } Message::Flush(ch) => { let res = self.collect_and_export().await; - if ch.send(res).is_err() { - global::handle_error(MetricsError::Other("flush channel closed".into())) + if let Err(e) = ch.send(res) { + otel_error!( + name: "PeriodicReaderWorker.Flush.ResponseSendError", + error = format!("{:?}", e), + ); } } Message::Shutdown(ch) => { let res = self.collect_and_export().await; let _ = self.reader.exporter.shutdown(); - if ch.send(res).is_err() { - global::handle_error(MetricsError::Other("shutdown channel closed".into())) + if let Err(e) = ch.send(res) { + otel_error!( + name: "PeriodicReaderWorker.Shutdown.ResponseSendError", + error = format!("{:?}", e), + ); } return false; } @@ -311,9 +316,7 @@ impl MetricReader for PeriodicReader { let worker = match &mut inner.sdk_producer_or_worker { ProducerOrWorker::Producer(_) => { // Only register once. If producer is already set, do nothing. - global::handle_error(MetricsError::Other( - "duplicate meter registration, did not register manual reader".into(), - )); + otel_error!(name: "PeriodicReader.RegisterPipeline.DuplicateRegistration"); return; } ProducerOrWorker::Worker(w) => mem::replace(w, Box::new(|_| {})), diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs index a140f65dec..da872fbc92 100644 --- a/opentelemetry-sdk/src/metrics/pipeline.rs +++ b/opentelemetry-sdk/src/metrics/pipeline.rs @@ -6,9 +6,8 @@ use std::{ }; use opentelemetry::{ - global, metrics::{MetricsError, Result}, - KeyValue, + otel_warn, KeyValue, }; use crate::{ @@ -414,15 +413,13 @@ where if existing == id { return; } - - global::handle_error(MetricsError::Other(format!( - "duplicate metric stream definitions, names: ({} and {}), descriptions: ({} and {}), kinds: ({:?} and {:?}), units: ({:?} and {:?}), and numbers: ({} and {})", - existing.name, id.name, - existing.description, id.description, - existing.kind, id.kind, - existing.unit, id.unit, - existing.number, id.number, - ))) + otel_warn!(name: "InstrumentSync.AddSync.DuplicateMetricStreamDefinitions", + name = id.name.clone(), + description = id.description.clone(), + kind = id.kind, + unit = id.unit.clone(), + number = id.number.clone(), + ); } } } diff --git a/opentelemetry-sdk/src/metrics/view.rs b/opentelemetry-sdk/src/metrics/view.rs index d9f256bd2b..7c862427ac 100644 --- a/opentelemetry-sdk/src/metrics/view.rs +++ b/opentelemetry-sdk/src/metrics/view.rs @@ -1,8 +1,8 @@ use super::instrument::{Instrument, Stream}; use glob::Pattern; use opentelemetry::{ - global, metrics::{MetricsError, Result}, + otel_error, }; fn empty_view(_inst: &Instrument) -> Option { @@ -102,9 +102,11 @@ impl View for Box { /// ``` pub fn new_view(criteria: Instrument, mask: Stream) -> Result> { if criteria.is_empty() { - global::handle_error(MetricsError::Config(format!( - "no criteria provided, dropping view. mask: {mask:?}" - ))); + otel_error!(name: "CreateView.ValidationError", error = MetricsError::Config( + "no criteria provided, dropping view".to_string()), + criteria = criteria, + mask = mask + ); return Ok(Box::new(empty_view)); } let contains_wildcard = criteria.name.contains(['*', '?']); @@ -112,9 +114,11 @@ pub fn new_view(criteria: Instrument, mask: Stream) -> Result> { let match_fn: Box bool + Send + Sync> = if contains_wildcard { if mask.name != "" { - global::handle_error(MetricsError::Config(format!( - "name replacement for multiple instruments, dropping view, criteria: {criteria:?}, mask: {mask:?}" - ))); + otel_error!(name: "CreateView.ValidationError", error = MetricsError::Config( + "name replacement for wildcard instrument, dropping view".to_string()), + criteria = criteria, + mask = mask + ); return Ok(Box::new(empty_view)); } @@ -138,10 +142,12 @@ pub fn new_view(criteria: Instrument, mask: Stream) -> Result> { match ma.validate() { Ok(_) => agg = Some(ma.clone()), Err(err) => { - global::handle_error(MetricsError::Other(format!( - "{}, proceeding as if view did not exist. criteria: {:?}, mask: {:?}", - err, err_msg_criteria, mask - ))); + otel_error!(name: "CreateView.ValidationError", + error = MetricsError::Config("invalid aggregation, dropping view".to_string()), + criteria = err_msg_criteria, + mask = mask, + error = err + ); return Ok(Box::new(empty_view)); } } diff --git a/opentelemetry/src/global/internal_logging.rs b/opentelemetry/src/global/internal_logging.rs index 3a5c24ba69..83950bc95e 100644 --- a/opentelemetry/src/global/internal_logging.rs +++ b/opentelemetry/src/global/internal_logging.rs @@ -50,11 +50,29 @@ macro_rules! otel_warn { { tracing::warn!(name: $name, target: env!("CARGO_PKG_NAME"), ""); } + #[cfg(not(feature = "internal-logs"))] + { + #[allow(unused_variables)] + { + + } + } }; (name: $name:expr, $($key:ident = $value:expr),+ $(,)?) => { #[cfg(feature = "internal-logs")] { - tracing::warn!(name: $name, target: env!("CARGO_PKG_NAME"), $($key = $value),+, ""); + tracing::warn!(name: $name, + target: env!("CARGO_PKG_NAME"), + $($key = { + $crate::format_value!($value) + }),+, + "" + ) } + #[cfg(not(feature = "internal-logs"))] + { + { + let _ = ($name, $($value),+); + } } }; } @@ -104,11 +122,34 @@ macro_rules! otel_error { { tracing::error!(name: $name, target: env!("CARGO_PKG_NAME"), ""); } + #[allow(unused_variables)] + { + + } }; (name: $name:expr, $($key:ident = $value:expr),+ $(,)?) => { #[cfg(feature = "internal-logs")] { - tracing::error!(name: $name, target: env!("CARGO_PKG_NAME"), $($key = $value),+, ""); + tracing::error!(name: $name, + target: env!("CARGO_PKG_NAME"), + $($key = { + $crate::format_value!($value) + }),+, + "" + ) } + #[cfg(not(feature = "internal-logs"))] + { + { + let _ = ($name, $($value),+); + } } }; } + +/// Helper macro to format a value +#[macro_export] +macro_rules! format_value { + ($value:expr) => {{ + format!("{:?}", $value) + }}; +} From b8cb6af6e6c7487803a005386022f91e2fb16654 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 8 Oct 2024 16:54:56 -0700 Subject: [PATCH 02/17] change name for exponential histogram --- opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs index 675a224348..317d369200 100644 --- a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs +++ b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs @@ -101,7 +101,7 @@ impl ExpoHistogramDataPoint { // With a scale of -10 there is only two buckets for the whole range of f64 values. // This can only happen if there is a max size of 1. otel_error!( - name: "ExpoHistogramDataPoint.Scale.Underflow", + name: "ExponentialHistogramDataPoint.Scale.Underflow", current_scale = self.scale, scale_delta = scale_delta, max_size = self.max_size, From acf97fa537538fe0dbb04813d4d9a440a0a2dedd Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 8 Oct 2024 17:46:55 -0700 Subject: [PATCH 03/17] rever changes --- .../src/metrics/periodic_reader.rs | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/periodic_reader.rs b/opentelemetry-sdk/src/metrics/periodic_reader.rs index 18640c1a3d..30604acc48 100644 --- a/opentelemetry-sdk/src/metrics/periodic_reader.rs +++ b/opentelemetry-sdk/src/metrics/periodic_reader.rs @@ -12,6 +12,7 @@ use futures_util::{ StreamExt, }; use opentelemetry::{ + global, metrics::{MetricsError, Result}, otel_error, }; @@ -263,26 +264,20 @@ impl PeriodicReaderWorker { match message { Message::Export => { if let Err(err) = self.collect_and_export().await { - otel_error!(name: "PeriodicReaderWorker.Export.Error",error = err); + global::handle_error(err) } } Message::Flush(ch) => { let res = self.collect_and_export().await; - if let Err(e) = ch.send(res) { - otel_error!( - name: "PeriodicReaderWorker.Flush.ResponseSendError", - error = format!("{:?}", e), - ); + if ch.send(res).is_err() { + global::handle_error(MetricsError::Other("flush channel closed".into())) } } Message::Shutdown(ch) => { let res = self.collect_and_export().await; let _ = self.reader.exporter.shutdown(); - if let Err(e) = ch.send(res) { - otel_error!( - name: "PeriodicReaderWorker.Shutdown.ResponseSendError", - error = format!("{:?}", e), - ); + if ch.send(res).is_err() { + global::handle_error(MetricsError::Other("shutdown channel closed".into())) } return false; } @@ -316,7 +311,9 @@ impl MetricReader for PeriodicReader { let worker = match &mut inner.sdk_producer_or_worker { ProducerOrWorker::Producer(_) => { // Only register once. If producer is already set, do nothing. - otel_error!(name: "PeriodicReader.RegisterPipeline.DuplicateRegistration"); + global::handle_error(MetricsError::Other( + "duplicate meter registration, did not register manual reader".into(), + )); return; } ProducerOrWorker::Worker(w) => mem::replace(w, Box::new(|_| {})), From 7b48f1487aad78689a8202e9544ed726f736535b Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 8 Oct 2024 18:40:43 -0700 Subject: [PATCH 04/17] Update opentelemetry-sdk/src/metrics/internal/mod.rs Co-authored-by: Cijo Thomas --- opentelemetry-sdk/src/metrics/internal/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index b7c8fed68f..30ae98bf5e 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -146,7 +146,7 @@ impl, T: Number, O: Operation> ValueMap { let new_tracker = AU::new_atomic_tracker(self.buckets_count); O::update_tracker(&new_tracker, measurement, index); trackers.insert(STREAM_OVERFLOW_ATTRIBUTES.clone(), Arc::new(new_tracker)); - otel_error!( name: "ValueMap.Measure.Overflow", + otel_error!( name: "MetricCardinalityLimitReached", error = MetricsError::Other("Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into()) ); } From ac61b798570973845a9c25da6712a15cb8962e62 Mon Sep 17 00:00:00 2001 From: Lalit Date: Tue, 8 Oct 2024 19:14:11 -0700 Subject: [PATCH 05/17] convert otel_error to otel_warn for cardinality limit --- opentelemetry-sdk/src/metrics/internal/aggregate.rs | 5 +++++ opentelemetry-sdk/src/metrics/internal/mod.rs | 10 ++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/internal/aggregate.rs b/opentelemetry-sdk/src/metrics/internal/aggregate.rs index e004240c60..4e589e6ced 100644 --- a/opentelemetry-sdk/src/metrics/internal/aggregate.rs +++ b/opentelemetry-sdk/src/metrics/internal/aggregate.rs @@ -16,6 +16,11 @@ pub(crate) fn is_under_cardinality_limit(size: usize) -> bool { size < STREAM_CARDINALITY_LIMIT as usize } +/// Checks whether aggregator has hit cardinality limit for metric streams +pub(crate) fn cardinality_limit() -> usize { + STREAM_CARDINALITY_LIMIT as usize +} + /// Receives measurements to be aggregated. pub(crate) trait Measure: Send + Sync + 'static { fn call(&self, measurement: T, attrs: &[KeyValue]); diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index 30ae98bf5e..3d02a1d3ce 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -12,12 +12,12 @@ use std::ops::{Add, AddAssign, Sub}; use std::sync::atomic::{AtomicBool, AtomicI64, AtomicU64, AtomicUsize, Ordering}; use std::sync::{Arc, RwLock}; -use aggregate::is_under_cardinality_limit; +use aggregate::{cardinality_limit, is_under_cardinality_limit}; pub(crate) use aggregate::{AggregateBuilder, ComputeAggregation, Measure}; pub(crate) use exponential_histogram::{EXPO_MAX_SCALE, EXPO_MIN_SCALE}; use once_cell::sync::Lazy; use opentelemetry::metrics::MetricsError; -use opentelemetry::{otel_error, KeyValue}; +use opentelemetry::{otel_warn, KeyValue}; use crate::metrics::AttributeSet; @@ -146,8 +146,10 @@ impl, T: Number, O: Operation> ValueMap { let new_tracker = AU::new_atomic_tracker(self.buckets_count); O::update_tracker(&new_tracker, measurement, index); trackers.insert(STREAM_OVERFLOW_ATTRIBUTES.clone(), Arc::new(new_tracker)); - otel_error!( name: "MetricCardinalityLimitReached", - error = MetricsError::Other("Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into()) + //TODO - include name of meter, instrument + otel_warn!( name: "MetricCardinalityLimitReached", + error = MetricsError::Other("Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into()), + cardinality_limit = cardinality_limit() ); } } From a42d516629f10895809d8bd141263f85e8e0ead4 Mon Sep 17 00:00:00 2001 From: Lalit Date: Tue, 8 Oct 2024 19:19:03 -0700 Subject: [PATCH 06/17] log both existing and new instrument values --- opentelemetry-sdk/src/metrics/pipeline.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs index da872fbc92..b267a89a43 100644 --- a/opentelemetry-sdk/src/metrics/pipeline.rs +++ b/opentelemetry-sdk/src/metrics/pipeline.rs @@ -413,12 +413,17 @@ where if existing == id { return; } - otel_warn!(name: "InstrumentSync.AddSync.DuplicateMetricStreamDefinitions", + otel_warn!(name: "Instrument.DuplicateMetricStreamDefinitions", name = id.name.clone(), description = id.description.clone(), kind = id.kind, unit = id.unit.clone(), number = id.number.clone(), + existing_name = existing.name.clone(), + existing_desc = existing.description.clone(), + existing_kind = existing.kind, + existing_unit = existing.unit, + existing_number = existing.number ); } } From 73fca4db2173a0c25eb5a79e20ae980d3b8bc0ed Mon Sep 17 00:00:00 2001 From: Lalit Date: Tue, 8 Oct 2024 19:24:02 -0700 Subject: [PATCH 07/17] build error --- opentelemetry-sdk/src/metrics/pipeline.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs index b267a89a43..e953049d7c 100644 --- a/opentelemetry-sdk/src/metrics/pipeline.rs +++ b/opentelemetry-sdk/src/metrics/pipeline.rs @@ -422,8 +422,8 @@ where existing_name = existing.name.clone(), existing_desc = existing.description.clone(), existing_kind = existing.kind, - existing_unit = existing.unit, - existing_number = existing.number + existing_unit = existing.unit.clone(), + existing_number = existing.number.clone() ); } } From 3aa97cf2ebf4ca74eb2b00f4c75e804e490d6110 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 9 Oct 2024 17:43:16 -0700 Subject: [PATCH 08/17] remove formatting in wrapper macros --- opentelemetry-sdk/src/metrics/internal/mod.rs | 4 +-- opentelemetry-sdk/src/metrics/meter.rs | 28 +++++++++---------- .../src/metrics/meter_provider.rs | 2 +- opentelemetry-sdk/src/metrics/pipeline.rs | 20 ++++++------- opentelemetry-sdk/src/metrics/view.rs | 23 ++++++++------- opentelemetry/src/global/internal_logging.rs | 12 ++------ 6 files changed, 40 insertions(+), 49 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index 3d02a1d3ce..7b9b0c8596 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -148,8 +148,8 @@ impl, T: Number, O: Operation> ValueMap { trackers.insert(STREAM_OVERFLOW_ATTRIBUTES.clone(), Arc::new(new_tracker)); //TODO - include name of meter, instrument otel_warn!( name: "MetricCardinalityLimitReached", - error = MetricsError::Other("Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into()), - cardinality_limit = cardinality_limit() + error = format!("{}", MetricsError::Other("Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into())), + cardinality_limit = cardinality_limit() as u64, ); } } diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index 947b40461a..06a498d13b 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -74,7 +74,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateCounter.ValidationError", error = err); + otel_error!(name: "SdkMeter.CreateCounter.ValidationError", error = format!("{}", err)); return Ok(Counter::new(Arc::new(NoopSyncInstrument::new()))); } @@ -90,7 +90,7 @@ impl SdkMeter { { Ok(counter) => Ok(counter), Err(err) => { - otel_error!(name: "SdkMeter.CreateCounter.Error", error = err); + otel_error!(name: "SdkMeter.CreateCounter.Error", error = format!("{}", err)); Ok(Counter::new(Arc::new(NoopSyncInstrument::new()))) } } @@ -106,7 +106,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateObservableCounter.ValidationError", error = err); + otel_error!(name: "SdkMeter.CreateObservableCounter.ValidationError", error = format!("{}", err)); return Ok(ObservableCounter::new(Arc::new(NoopAsyncInstrument::new()))); } @@ -119,7 +119,7 @@ impl SdkMeter { )?; if ms.is_empty() { - otel_error!(name: "SdkMeter.CreateObservableCounter.Error", error = MetricsError::Other("no measures found".into())); + otel_error!(name: "SdkMeter.CreateObservableCounter.Error", error = format!("{}", MetricsError::Other("no measures found".into()))); return Ok(ObservableCounter::new(Arc::new(NoopAsyncInstrument::new()))); } @@ -144,7 +144,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateObservableUpDownCounter.ValidationError", error = err); + otel_error!(name: "SdkMeter.CreateObservableUpDownCounter.ValidationError", error = format!("{}", err)); return Ok(ObservableUpDownCounter::new(Arc::new( NoopAsyncInstrument::new(), ))); @@ -159,7 +159,7 @@ impl SdkMeter { )?; if ms.is_empty() { - otel_error!(name: "SdkMeter.CreateObservableUpDownCounter.Error", error = MetricsError::Other("no measures found".into())); + otel_error!(name: "SdkMeter.CreateObservableUpDownCounter.Error", error = format!("{}",MetricsError::Other("no measures found".into()))); return Ok(ObservableUpDownCounter::new(Arc::new( NoopAsyncInstrument::new(), ))); @@ -186,7 +186,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateObservableGauge.ValidationError", error = err); + otel_error!(name: "SdkMeter.CreateObservableGauge.ValidationError", error = format!("{}", err)); return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new()))); } @@ -199,7 +199,7 @@ impl SdkMeter { )?; if ms.is_empty() { - otel_error!(name: "SdkMeter.CreateObservableGauge.Error", error = MetricsError::Other("no measures found".into())); + otel_error!(name: "SdkMeter.CreateObservableGauge.Error",error = format!("{}", MetricsError::Other("no measures found".into()))); return Ok(ObservableGauge::new(Arc::new(NoopAsyncInstrument::new()))); } @@ -224,7 +224,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateUpDownCounter.ValidationError", error = err); + otel_error!(name: "SdkMeter.CreateUpDownCounter.ValidationError", error = format!("{}",err)); return Ok(UpDownCounter::new(Arc::new(NoopSyncInstrument::new()))); } @@ -240,7 +240,7 @@ impl SdkMeter { { Ok(updown_counter) => Ok(updown_counter), Err(err) => { - otel_error!(name: "SdkMeter.CreateUpDownCounter.Error", error = err); + otel_error!(name: "SdkMeter.CreateUpDownCounter.Error", error = format!("{}", err)); Ok(UpDownCounter::new(Arc::new(NoopSyncInstrument::new()))) } } @@ -256,7 +256,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateGauge.ValidationError", error = err); + otel_error!(name: "SdkMeter.CreateGauge.ValidationError", error = format!("{}", err)); return Ok(Gauge::new(Arc::new(NoopSyncInstrument::new()))); } @@ -272,7 +272,7 @@ impl SdkMeter { { Ok(gauge) => Ok(gauge), Err(err) => { - otel_error!(name: "SdkMeter.CreateGauge.Error", error = err); + otel_error!(name: "SdkMeter.CreateGauge.Error", error = format!("{}",err)); Ok(Gauge::new(Arc::new(NoopSyncInstrument::new()))) } } @@ -288,7 +288,7 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateHistogram.ValidationError", error = err); + otel_error!(name: "SdkMeter.CreateHistogram.ValidationError", error = format!("{}", err)); return Ok(Histogram::new(Arc::new(NoopSyncInstrument::new()))); } @@ -304,7 +304,7 @@ impl SdkMeter { { Ok(histogram) => Ok(histogram), Err(err) => { - otel_error!(name: "SdkMeter.CreateHistogram.Error", error = err); + otel_error!(name: "SdkMeter.CreateHistogram.Error", error = format!("{}",err)); Ok(Histogram::new(Arc::new(NoopSyncInstrument::new()))) } } diff --git a/opentelemetry-sdk/src/metrics/meter_provider.rs b/opentelemetry-sdk/src/metrics/meter_provider.rs index 88c11c393a..a6c82a78ad 100644 --- a/opentelemetry-sdk/src/metrics/meter_provider.rs +++ b/opentelemetry-sdk/src/metrics/meter_provider.rs @@ -136,7 +136,7 @@ impl Drop for SdkMeterProviderInner { // shutdown(), then we don't need to call shutdown again. if !self.is_shutdown.load(Ordering::Relaxed) { if let Err(err) = self.shutdown() { - otel_error!(name: "SdkMeterProvider.Drop.ShutdownError", error = err); + otel_error!(name: "SdkMeterProvider.Drop.ShutdownError", error = format!("{}",err)); } } } diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs index e953049d7c..453ce1c296 100644 --- a/opentelemetry-sdk/src/metrics/pipeline.rs +++ b/opentelemetry-sdk/src/metrics/pipeline.rs @@ -414,16 +414,16 @@ where return; } otel_warn!(name: "Instrument.DuplicateMetricStreamDefinitions", - name = id.name.clone(), - description = id.description.clone(), - kind = id.kind, - unit = id.unit.clone(), - number = id.number.clone(), - existing_name = existing.name.clone(), - existing_desc = existing.description.clone(), - existing_kind = existing.kind, - existing_unit = existing.unit.clone(), - existing_number = existing.number.clone() + name = format!("{}",id.name), + description = format!("{}", id.description), + kind = format!("{:?}", id.kind), + unit = format!("{}",id.unit), + number = format!("{}", id.number), + existing_name = format!("{}", existing.name), + existing_desc = format!("{}", existing.description), + existing_kind = format!("{:?}", existing.kind), + existing_unit = format!("{}", existing.unit), + existing_number = format!("{}", existing.number), ); } } diff --git a/opentelemetry-sdk/src/metrics/view.rs b/opentelemetry-sdk/src/metrics/view.rs index 7c862427ac..176470ac3f 100644 --- a/opentelemetry-sdk/src/metrics/view.rs +++ b/opentelemetry-sdk/src/metrics/view.rs @@ -102,10 +102,10 @@ impl View for Box { /// ``` pub fn new_view(criteria: Instrument, mask: Stream) -> Result> { if criteria.is_empty() { - otel_error!(name: "CreateView.ValidationError", error = MetricsError::Config( - "no criteria provided, dropping view".to_string()), - criteria = criteria, - mask = mask + otel_error!(name: "CreateView.ValidationError", error = format!("{}",MetricsError::Config( + "no criteria provided, dropping view".to_string())), + criteria = format!("{:?}", criteria), + mask = format!("{:?}", mask) ); return Ok(Box::new(empty_view)); } @@ -114,10 +114,10 @@ pub fn new_view(criteria: Instrument, mask: Stream) -> Result> { let match_fn: Box bool + Send + Sync> = if contains_wildcard { if mask.name != "" { - otel_error!(name: "CreateView.ValidationError", error = MetricsError::Config( - "name replacement for wildcard instrument, dropping view".to_string()), - criteria = criteria, - mask = mask + otel_error!(name: "CreateView.ValidationError", error = format!("{}", MetricsError::Config( + "name replacement for wildcard instrument, dropping view".to_string())), + criteria = format!("{:?}", criteria), + mask = format!("{:?}", mask) ); return Ok(Box::new(empty_view)); } @@ -143,10 +143,9 @@ pub fn new_view(criteria: Instrument, mask: Stream) -> Result> { Ok(_) => agg = Some(ma.clone()), Err(err) => { otel_error!(name: "CreateView.ValidationError", - error = MetricsError::Config("invalid aggregation, dropping view".to_string()), - criteria = err_msg_criteria, - mask = mask, - error = err + error = format!("{}",err), + criteria = format!("{:?}", err_msg_criteria), + mask = format!("{:?}", mask), ); return Ok(Box::new(empty_view)); } diff --git a/opentelemetry/src/global/internal_logging.rs b/opentelemetry/src/global/internal_logging.rs index 83950bc95e..1557607bc2 100644 --- a/opentelemetry/src/global/internal_logging.rs +++ b/opentelemetry/src/global/internal_logging.rs @@ -64,7 +64,7 @@ macro_rules! otel_warn { tracing::warn!(name: $name, target: env!("CARGO_PKG_NAME"), $($key = { - $crate::format_value!($value) + $value }),+, "" ) } @@ -133,7 +133,7 @@ macro_rules! otel_error { tracing::error!(name: $name, target: env!("CARGO_PKG_NAME"), $($key = { - $crate::format_value!($value) + $value }),+, "" ) } @@ -145,11 +145,3 @@ macro_rules! otel_error { } }; } - -/// Helper macro to format a value -#[macro_export] -macro_rules! format_value { - ($value:expr) => {{ - format!("{:?}", $value) - }}; -} From bda9faa8730c9b59cc57b0f4302798526eb8f528 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 23 Oct 2024 12:14:24 -0700 Subject: [PATCH 09/17] review comments --- .../metrics/internal/exponential_histogram.rs | 4 +- .../src/metrics/manual_reader.rs | 4 +- opentelemetry-sdk/src/metrics/meter.rs | 105 ++++++++++++++---- opentelemetry-sdk/src/metrics/pipeline.rs | 3 +- 4 files changed, 90 insertions(+), 26 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs index 317d369200..14c7397a45 100644 --- a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs +++ b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs @@ -100,6 +100,7 @@ impl ExpoHistogramDataPoint { if (self.scale - scale_delta as i8) < EXPO_MIN_SCALE { // With a scale of -10 there is only two buckets for the whole range of f64 values. // This can only happen if there is a max size of 1. + //TBD - check for throttling requirements if can happen too frequent. otel_error!( name: "ExponentialHistogramDataPoint.Scale.Underflow", current_scale = self.scale, @@ -107,7 +108,8 @@ impl ExpoHistogramDataPoint { max_size = self.max_size, min_scale = EXPO_MIN_SCALE, record_min_max = self.record_min_max, - record_sum = self.record_sum + record_sum = self.record_sum, + error = format!("The measurement will be dropped due to scale underflow. Check the histogram configuration") ); return; } diff --git a/opentelemetry-sdk/src/metrics/manual_reader.rs b/opentelemetry-sdk/src/metrics/manual_reader.rs index df9306abf1..c83801bc9a 100644 --- a/opentelemetry-sdk/src/metrics/manual_reader.rs +++ b/opentelemetry-sdk/src/metrics/manual_reader.rs @@ -5,7 +5,7 @@ use std::{ use opentelemetry::{ metrics::{MetricsError, Result}, - otel_error, + otel_debug, }; use super::{ @@ -77,7 +77,7 @@ impl MetricReader for ManualReader { if inner.sdk_producer.is_none() { inner.sdk_producer = Some(pipeline); } else { - otel_error!(name: "ManualReader.RegisterPipeline.DuplicateRegistration"); + otel_debug!(name: "ManualReader.RegisterPipeline.DuplicateRegistration"); } }); } diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index 128882147b..0a9518ba89 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -75,14 +75,18 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateCounter.ValidationError", error = format!("{}", err)); + otel_error!( + name: "SdkMeter.CreateCounter.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err)); return Ok(Counter::new(Arc::new(NoopSyncInstrument::new()))); } match resolver .lookup( InstrumentKind::Counter, - builder.name, + builder.name.clone(), builder.description, builder.unit, None, @@ -91,7 +95,12 @@ impl SdkMeter { { Ok(counter) => Ok(counter), Err(err) => { - otel_error!(name: "SdkMeter.CreateCounter.Error", error = format!("{}", err)); + otel_error!( + name: "SdkMeter.CreateCounter.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err) + ); Ok(Counter::new(Arc::new(NoopSyncInstrument::new()))) } } @@ -107,20 +116,29 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateObservableCounter.ValidationError", error = format!("{}", err)); + otel_error!( + name: "SdkMeter.CreateObservableCounter.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err)); return Ok(ObservableCounter::new()); } let ms = resolver.measures( InstrumentKind::ObservableCounter, - builder.name, + builder.name.clone(), builder.description, builder.unit, None, )?; if ms.is_empty() { - otel_error!(name: "SdkMeter.CreateObservableCounter.Error", error = format!("{}", MetricsError::Other("no measures found".into()))); + otel_error!( + name: "SdkMeter.CreateObservableCounter.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + message = "Measurements from the instrument will be ignored. Reason: View Configuration / Drop Aggregation" + ); return Ok(ObservableCounter::new()); } @@ -145,20 +163,29 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateObservableUpDownCounter.ValidationError", error = format!("{}", err)); + otel_error!( + name: "SdkMeter.CreateObservableUpDownCounter.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err)); return Ok(ObservableUpDownCounter::new()); } let ms = resolver.measures( InstrumentKind::ObservableUpDownCounter, - builder.name, + builder.name.clone(), builder.description, builder.unit, None, )?; if ms.is_empty() { - otel_error!(name: "SdkMeter.CreateObservableUpDownCounter.Error", error = format!("{}",MetricsError::Other("no measures found".into()))); + otel_error!( + name: "SdkMeter.CreateUpDownObservableCounter.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + message = "Measurements from the instrument will be ignored. Reason: View Configuration / Drop Aggregation" + ); return Ok(ObservableUpDownCounter::new()); } @@ -183,20 +210,29 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateObservableGauge.ValidationError", error = format!("{}", err)); + otel_error!( + name: "SdkMeter.CreateObservableGauge.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err)); return Ok(ObservableGauge::new()); } let ms = resolver.measures( InstrumentKind::ObservableGauge, - builder.name, + builder.name.clone(), builder.description, builder.unit, None, )?; if ms.is_empty() { - otel_error!(name: "SdkMeter.CreateObservableGauge.Error",error = format!("{}", MetricsError::Other("no measures found".into()))); + otel_error!( + name: "SdkMeter.CreateObservableGauge.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + message = "Measurements from the instrument will be ignored. Reason: View Configuration / Drop Aggregation" + ); return Ok(ObservableGauge::new()); } @@ -221,14 +257,18 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateUpDownCounter.ValidationError", error = format!("{}",err)); + otel_error!( + name: "SdkMeter.CreateUpDownCounter.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err)); return Ok(UpDownCounter::new(Arc::new(NoopSyncInstrument::new()))); } match resolver .lookup( InstrumentKind::UpDownCounter, - builder.name, + builder.name.clone(), builder.description, builder.unit, None, @@ -237,7 +277,12 @@ impl SdkMeter { { Ok(updown_counter) => Ok(updown_counter), Err(err) => { - otel_error!(name: "SdkMeter.CreateUpDownCounter.Error", error = format!("{}", err)); + otel_error!( + name: "SdkMeter.CreateUpDownCounter.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err) + ); Ok(UpDownCounter::new(Arc::new(NoopSyncInstrument::new()))) } } @@ -253,14 +298,18 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateGauge.ValidationError", error = format!("{}", err)); + otel_error!( + name: "SdkMeter.CreateGauge.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err)); return Ok(Gauge::new(Arc::new(NoopSyncInstrument::new()))); } match resolver .lookup( InstrumentKind::Gauge, - builder.name, + builder.name.clone(), builder.description, builder.unit, None, @@ -269,7 +318,12 @@ impl SdkMeter { { Ok(gauge) => Ok(gauge), Err(err) => { - otel_error!(name: "SdkMeter.CreateGauge.Error", error = format!("{}",err)); + otel_error!( + name: "SdkMeter.CreateGauge.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err) + ); Ok(Gauge::new(Arc::new(NoopSyncInstrument::new()))) } } @@ -285,14 +339,18 @@ impl SdkMeter { { let validation_result = validate_instrument_config(builder.name.as_ref(), &builder.unit); if let Err(err) = validation_result { - otel_error!(name: "SdkMeter.CreateHistogram.ValidationError", error = format!("{}", err)); + otel_error!( + name: "SdkMeter.CreateHistogram.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err)); return Ok(Histogram::new(Arc::new(NoopSyncInstrument::new()))); } match resolver .lookup( InstrumentKind::Histogram, - builder.name, + builder.name.clone(), builder.description, builder.unit, builder.boundaries, @@ -301,7 +359,12 @@ impl SdkMeter { { Ok(histogram) => Ok(histogram), Err(err) => { - otel_error!(name: "SdkMeter.CreateHistogram.Error", error = format!("{}",err)); + otel_error!( + name: "SdkMeter.CreateHistogram.InstrumentCreationFailed", + meter_name = self.scope.name.as_ref(), + instrument_name = builder.name.as_ref(), + error = format!("Measurements from the instrument will be ignored. Reason: {}", err) + ); Ok(Histogram::new(Arc::new(NoopSyncInstrument::new()))) } } diff --git a/opentelemetry-sdk/src/metrics/pipeline.rs b/opentelemetry-sdk/src/metrics/pipeline.rs index 453ce1c296..7f15eeca92 100644 --- a/opentelemetry-sdk/src/metrics/pipeline.rs +++ b/opentelemetry-sdk/src/metrics/pipeline.rs @@ -712,8 +712,7 @@ 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 + // Error is logged elsewhere. } Ok(measures) } else { From 0087c24cfa113f016dfb7f064859df45ccad2847 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 23 Oct 2024 13:27:02 -0700 Subject: [PATCH 10/17] lint error --- opentelemetry-sdk/src/metrics/meter.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index 591d54bed5..0f18dc86d5 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -149,7 +149,7 @@ impl SdkMeter { } ObservableCounter::new() } - Err(err) => { + Err(_err) => { // TBD - Add error handling ObservableCounter::new() } @@ -199,7 +199,7 @@ impl SdkMeter { } ObservableUpDownCounter::new() } - Err(err) => { + Err(_err) => { // TBD - Add error handling ObservableUpDownCounter::new() } @@ -249,7 +249,7 @@ impl SdkMeter { } ObservableGauge::new() } - Err(err) => { + Err(_err) => { //TBD - Add error handling ObservableGauge::new() } From 115e73f116ff6cfe22b2871d18c9ea64f844a97b Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 23 Oct 2024 13:40:55 -0700 Subject: [PATCH 11/17] add comment for exponential histogram --- .../src/metrics/internal/exponential_histogram.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs index 14c7397a45..3c1baa61db 100644 --- a/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs +++ b/opentelemetry-sdk/src/metrics/internal/exponential_histogram.rs @@ -100,7 +100,11 @@ impl ExpoHistogramDataPoint { if (self.scale - scale_delta as i8) < EXPO_MIN_SCALE { // With a scale of -10 there is only two buckets for the whole range of f64 values. // This can only happen if there is a max size of 1. - //TBD - check for throttling requirements if can happen too frequent. + + // This error is logged when a measurement is dropped because its value is either too high + // or too low, falling outside the allowable range defined by the scale and max_size. + // If these values are expected, the user should adjust the histogram configuration + // to accommodate the range. otel_error!( name: "ExponentialHistogramDataPoint.Scale.Underflow", current_scale = self.scale, @@ -109,6 +113,7 @@ impl ExpoHistogramDataPoint { min_scale = EXPO_MIN_SCALE, record_min_max = self.record_min_max, record_sum = self.record_sum, + value = format!("{:?}", v), error = format!("The measurement will be dropped due to scale underflow. Check the histogram configuration") ); return; From 56682a46885bbab7cc9297574c3944e9a79969b9 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 23 Oct 2024 15:03:57 -0700 Subject: [PATCH 12/17] fix --- .../src/metrics/manual_reader.rs | 7 ++- .../src/metrics/periodic_reader.rs | 56 ++++++++++++------- opentelemetry/src/metrics/mod.rs | 10 +++- 3 files changed, 51 insertions(+), 22 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/manual_reader.rs b/opentelemetry-sdk/src/metrics/manual_reader.rs index c83801bc9a..abc0bcdc98 100644 --- a/opentelemetry-sdk/src/metrics/manual_reader.rs +++ b/opentelemetry-sdk/src/metrics/manual_reader.rs @@ -76,8 +76,11 @@ impl MetricReader for ManualReader { // Only register once. If producer is already set, do nothing. if inner.sdk_producer.is_none() { inner.sdk_producer = Some(pipeline); - } else { - otel_debug!(name: "ManualReader.RegisterPipeline.DuplicateRegistration"); + } else { + otel_debug!( + name: "ManualReader.RegisterPipeline.DuplicateRegistration", + error = "The pipeline is already registered to the Reader. Registering pipeline multiple times is not allowed." + ); } }); } diff --git a/opentelemetry-sdk/src/metrics/periodic_reader.rs b/opentelemetry-sdk/src/metrics/periodic_reader.rs index 67e887d1eb..181d87d1da 100644 --- a/opentelemetry-sdk/src/metrics/periodic_reader.rs +++ b/opentelemetry-sdk/src/metrics/periodic_reader.rs @@ -14,7 +14,7 @@ use futures_util::{ use opentelemetry::{ global, metrics::{MetricsError, Result}, - otel_error, + otel_debug, otel_error, }; use crate::runtime::Runtime; @@ -245,13 +245,11 @@ impl PeriodicReaderWorker { Either::Left((res, _)) => { res // return the status of export. } - Either::Right(_) => { - otel_error!( - name: "collect_and_export", - status = "timed_out" - ); - Err(MetricsError::Other("export timed out".into())) - } + Either::Right(_) => Err(MetricsError::ExportTimeout( + "PeriodicReader".into(), + self.timeout.as_nanos(), + ) + .into()), } } @@ -259,22 +257,44 @@ impl PeriodicReaderWorker { match message { Message::Export => { if let Err(err) = self.collect_and_export().await { - global::handle_error(err) + match err { + MetricsError::ExportTimeout(_, _) => { + otel_error!( name: "PeriodicReader.ExportFailed", error = format!("{:?}", err)); + } + MetricsError::ReaderShutdown => { + otel_debug!( name: "PeriodicReader.ReaderShutdown", error = format!("{:?}", err)); + } + MetricsError::ReaderNotRegistered => { + otel_debug!( name: "PeriodicReader.ReaderNotRegistered", error = format!("{:?}", err)); + } + _ => { + // TBD: This includes errors from both collection and export. Need to be made more specific + // and identify the levels to log them. + otel_error!( name: "PeriodicReader.ExportFailed", error = format!("{:?}", err)); + } + } } } Message::Flush(ch) => { let res = self.collect_and_export().await; - if ch.send(res).is_err() { - global::handle_error(MetricsError::Other("flush channel closed".into())) + if let Err(send_error) = ch.send(res) { + otel_debug!( + name: "PeriodicReader.Flush.SendResultError", + error = format!("{:?}", send_error), + ); } } Message::Shutdown(ch) => { let res = self.collect_and_export().await; let _ = self.reader.exporter.shutdown(); - if ch.send(res).is_err() { - global::handle_error(MetricsError::Other("shutdown channel closed".into())) + if let Err(send_error) = ch.send(res) { + otel_debug!( + name: "PeriodicReader.Flush.SendResultError", + error = format!("{:?}", send_error), + ); + //Return false to break the loop and shutdown the worker. + return false; } - return false; } } @@ -300,9 +320,7 @@ impl MetricReader for PeriodicReader { let worker = match &mut inner.sdk_producer_or_worker { ProducerOrWorker::Producer(_) => { // Only register once. If producer is already set, do nothing. - global::handle_error(MetricsError::Other( - "duplicate meter registration, did not register manual reader".into(), - )); + otel_debug!(name: "PeriodicReader.RegisterPipeline.DuplicateRegistration"); return; } ProducerOrWorker::Worker(w) => mem::replace(w, Box::new(|_| {})), @@ -315,7 +333,7 @@ impl MetricReader for PeriodicReader { fn collect(&self, rm: &mut ResourceMetrics) -> Result<()> { let inner = self.inner.lock()?; if inner.is_shutdown { - return Err(MetricsError::Other("reader is shut down".into())); + return Err(MetricsError::ReaderShutdown); } if let Some(producer) = match &inner.sdk_producer_or_worker { @@ -324,7 +342,7 @@ impl MetricReader for PeriodicReader { } { producer.produce(rm)?; } else { - return Err(MetricsError::Other("reader is not registered".into())); + return Err(MetricsError::ReaderNotRegistered); } Ok(()) diff --git a/opentelemetry/src/metrics/mod.rs b/opentelemetry/src/metrics/mod.rs index 417a37ff5e..0b58a690d4 100644 --- a/opentelemetry/src/metrics/mod.rs +++ b/opentelemetry/src/metrics/mod.rs @@ -1,5 +1,4 @@ //! # OpenTelemetry Metrics API - use std::hash::{Hash, Hasher}; use std::result; use std::sync::Arc; @@ -34,9 +33,18 @@ pub enum MetricsError { /// Invalid configuration #[error("Config error {0}")] Config(String), + /// Timeout + #[error("Metrics reader {0} failed with timeout. Max configured timeout: {1} ns")] + ExportTimeout(String, u128), /// Fail to export metrics #[error("Metrics exporter {} failed with {0}", .0.exporter_name())] ExportErr(Box), + /// Metrics Reader shutdown + #[error("Metrics reader is shutdown")] + ReaderShutdown, + /// Metrics reader is not registered + #[error("Metrics reader is not registered")] + ReaderNotRegistered, /// 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. From 7e48cbf79d91042c450f0838103117358de90c52 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 23 Oct 2024 15:04:39 -0700 Subject: [PATCH 13/17] Update opentelemetry-sdk/src/metrics/internal/mod.rs Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> --- opentelemetry-sdk/src/metrics/internal/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index 7b9b0c8596..3200cc8bae 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -148,7 +148,7 @@ impl, T: Number, O: Operation> ValueMap { trackers.insert(STREAM_OVERFLOW_ATTRIBUTES.clone(), Arc::new(new_tracker)); //TODO - include name of meter, instrument otel_warn!( name: "MetricCardinalityLimitReached", - error = format!("{}", MetricsError::Other("Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into())), + message = format!("{}", MetricsError::Other("Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into())), cardinality_limit = cardinality_limit() as u64, ); } From d81b37418f4d8a7e9bc5d08e7479e32f9308c6a8 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 23 Oct 2024 15:10:18 -0700 Subject: [PATCH 14/17] use message for otel_warn --- opentelemetry-sdk/src/metrics/internal/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index 7b9b0c8596..bf2b9f472d 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -16,7 +16,6 @@ use aggregate::{cardinality_limit, is_under_cardinality_limit}; pub(crate) use aggregate::{AggregateBuilder, ComputeAggregation, Measure}; pub(crate) use exponential_histogram::{EXPO_MAX_SCALE, EXPO_MIN_SCALE}; use once_cell::sync::Lazy; -use opentelemetry::metrics::MetricsError; use opentelemetry::{otel_warn, KeyValue}; use crate::metrics::AttributeSet; @@ -148,7 +147,7 @@ impl, T: Number, O: Operation> ValueMap { trackers.insert(STREAM_OVERFLOW_ATTRIBUTES.clone(), Arc::new(new_tracker)); //TODO - include name of meter, instrument otel_warn!( name: "MetricCardinalityLimitReached", - error = format!("{}", MetricsError::Other("Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into())), + message = format!("{}", "Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged."), cardinality_limit = cardinality_limit() as u64, ); } From 949930e9fb1ccbaffd7c45c2af21c770a8f7af00 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 23 Oct 2024 16:42:03 -0700 Subject: [PATCH 15/17] Update opentelemetry-sdk/src/metrics/meter.rs Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> --- opentelemetry-sdk/src/metrics/meter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index 0f18dc86d5..c5b6f69e16 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -150,7 +150,7 @@ impl SdkMeter { ObservableCounter::new() } Err(_err) => { - // TBD - Add error handling + // TODO - Add error handling ObservableCounter::new() } } From 6dcc9eb75b61ad3ef47cbe32623c619569cd762f Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 23 Oct 2024 16:42:26 -0700 Subject: [PATCH 16/17] Update opentelemetry-sdk/src/metrics/internal/mod.rs Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> --- opentelemetry-sdk/src/metrics/internal/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/internal/mod.rs b/opentelemetry-sdk/src/metrics/internal/mod.rs index bf2b9f472d..271c1d6e79 100644 --- a/opentelemetry-sdk/src/metrics/internal/mod.rs +++ b/opentelemetry-sdk/src/metrics/internal/mod.rs @@ -147,7 +147,7 @@ impl, T: Number, O: Operation> ValueMap { trackers.insert(STREAM_OVERFLOW_ATTRIBUTES.clone(), Arc::new(new_tracker)); //TODO - include name of meter, instrument otel_warn!( name: "MetricCardinalityLimitReached", - message = format!("{}", "Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged."), + message = format!("{}", "Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric will not be logged until next collect."), cardinality_limit = cardinality_limit() as u64, ); } From 4b42fe8f5cf4f6b8123a6c2c8a073476fe42c71a Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 23 Oct 2024 16:47:15 -0700 Subject: [PATCH 17/17] Update opentelemetry-sdk/src/metrics/meter.rs Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> --- opentelemetry-sdk/src/metrics/meter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index c5b6f69e16..c01c65487b 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -79,7 +79,7 @@ impl SdkMeter { name: "SdkMeter.CreateCounter.InstrumentCreationFailed", meter_name = self.scope.name.as_ref(), instrument_name = builder.name.as_ref(), - error = format!("Measurements from the instrument will be ignored. Reason: {}", err)); + error = format!("Measurements from the counter will be ignored. Reason: {}", err)); return Counter::new(Arc::new(NoopSyncInstrument::new())); }