From e524bdb93525307fee88346c8b2be339dc63f9ae Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Fri, 11 Oct 2024 14:07:38 -0700 Subject: [PATCH 1/7] Remove redundant Arc's in MeterProviderInner (#2198) --- opentelemetry-sdk/src/metrics/meter_provider.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/src/metrics/meter_provider.rs b/opentelemetry-sdk/src/metrics/meter_provider.rs index 49ad98bed6..139346b54a 100644 --- a/opentelemetry-sdk/src/metrics/meter_provider.rs +++ b/opentelemetry-sdk/src/metrics/meter_provider.rs @@ -32,11 +32,11 @@ pub struct SdkMeterProvider { inner: Arc, } -#[derive(Clone, Debug)] +#[derive(Debug)] struct SdkMeterProviderInner { pipes: Arc, - meters: Arc>>>, - is_shutdown: Arc, + meters: Mutex>>, + is_shutdown: AtomicBool, } impl Default for SdkMeterProvider { @@ -236,7 +236,7 @@ impl MeterProviderBuilder { self.views, )), meters: Default::default(), - is_shutdown: Arc::new(AtomicBool::new(false)), + is_shutdown: AtomicBool::new(false), }), } } From 20fd454c4ba33fb792fe6cccdd6d1a3b422c62bf Mon Sep 17 00:00:00 2001 From: Vaalla Date: Sat, 12 Oct 2024 19:44:58 +0300 Subject: [PATCH 2/7] Use the correct format when deserializing body field from logs (#2178) --- opentelemetry-proto/src/proto.rs | 44 +++++++------------ .../tonic/opentelemetry.proto.common.v1.rs | 15 ++++--- .../tonic/opentelemetry.proto.logs.v1.rs | 7 --- opentelemetry-proto/tests/grpc_build.rs | 9 ++-- opentelemetry-proto/tests/json_serde.rs | 32 ++++---------- 5 files changed, 36 insertions(+), 71 deletions(-) diff --git a/opentelemetry-proto/src/proto.rs b/opentelemetry-proto/src/proto.rs index ba4038072f..792e1fc945 100644 --- a/opentelemetry-proto/src/proto.rs +++ b/opentelemetry-proto/src/proto.rs @@ -6,7 +6,7 @@ pub(crate) mod serializers { use crate::tonic::common::v1::any_value::{self, Value}; use crate::tonic::common::v1::AnyValue; use serde::de::{self, MapAccess, Visitor}; - use serde::ser::SerializeStruct; + use serde::ser::{SerializeMap, SerializeStruct}; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use std::fmt; @@ -45,35 +45,23 @@ pub(crate) mod serializers { } // AnyValue <-> KeyValue conversion - pub fn serialize_to_value(value: &Option, serializer: S) -> Result + pub fn serialize_to_value(value: &Option, serializer: S) -> Result where S: Serializer, { - match value { - Some(any_value) => match &any_value.value { - Some(Value::IntValue(i)) => { - // Attempt to create a struct to wrap the intValue - let mut state = match serializer.serialize_struct("Value", 1) { - Ok(s) => s, - Err(e) => return Err(e), // Handle the error or return it - }; - - // Attempt to serialize the intValue field - if let Err(e) = state.serialize_field("intValue", &i.to_string()) { - return Err(e); // Handle the error or return it - } - - // Finalize the struct serialization - state.end() - } - Some(value) => value.serialize(serializer), - None => serializer.serialize_none(), - }, + match &value { + Some(Value::IntValue(i)) => { + // Attempt to serialize the intValue field + let mut map = serializer.serialize_map(Some(1))?; + map.serialize_entry("intValue", &i.to_string()); + map.end() + } + Some(value) => value.serialize(serializer), None => serializer.serialize_none(), } } - pub fn deserialize_from_value<'de, D>(deserializer: D) -> Result, D::Error> + pub fn deserialize_from_value<'de, D>(deserializer: D) -> Result, D::Error> where D: Deserializer<'de>, { @@ -99,13 +87,13 @@ pub(crate) mod serializers { } impl<'de> de::Visitor<'de> for ValueVisitor { - type Value = AnyValue; + type Value = Option; fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { formatter.write_str("a JSON object for AnyValue") } - fn visit_map(self, mut map: V) -> Result + fn visit_map(self, mut map: V) -> Result, V::Error> where V: de::MapAccess<'de>, { @@ -150,17 +138,17 @@ pub(crate) mod serializers { } if let Some(v) = value { - Ok(AnyValue { value: Some(v) }) + Ok(Some(v)) } else { Err(de::Error::custom( - "Invalid data for AnyValue, no known keys found", + "Invalid data for Value, no known keys found", )) } } } let value = deserializer.deserialize_map(ValueVisitor)?; - Ok(Some(value)) + Ok(value) } pub fn serialize_u64_to_string(value: &u64, serializer: S) -> Result diff --git a/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.common.v1.rs b/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.common.v1.rs index 4b08daffa2..b5bde05c27 100644 --- a/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.common.v1.rs +++ b/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.common.v1.rs @@ -10,6 +10,14 @@ pub struct AnyValue { /// The value is one of the listed fields. It is valid for all values to be unspecified /// in which case this AnyValue is considered to be "empty". #[prost(oneof = "any_value::Value", tags = "1, 2, 3, 4, 5, 6, 7")] + #[cfg_attr( + feature = "with-serde", + serde( + flatten, + serialize_with = "crate::proto::serializers::serialize_to_value", + deserialize_with = "crate::proto::serializers::deserialize_from_value" + ) + )] pub value: ::core::option::Option, } /// Nested message and enum types in `AnyValue`. @@ -75,13 +83,6 @@ pub struct KeyValue { #[prost(string, tag = "1")] pub key: ::prost::alloc::string::String, #[prost(message, optional, tag = "2")] - #[cfg_attr( - feature = "with-serde", - serde( - serialize_with = "crate::proto::serializers::serialize_to_value", - deserialize_with = "crate::proto::serializers::deserialize_from_value" - ) - )] pub value: ::core::option::Option, } /// InstrumentationScope is a message representing the instrumentation scope information diff --git a/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.logs.v1.rs b/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.logs.v1.rs index 28e5fc997d..0eb691fb3a 100644 --- a/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.logs.v1.rs +++ b/opentelemetry-proto/src/proto/tonic/opentelemetry.proto.logs.v1.rs @@ -122,13 +122,6 @@ pub struct LogRecord { /// string message (including multi-line) describing the event in a free form or it can /// be a structured data composed of arrays and maps of other values. \[Optional\]. #[prost(message, optional, tag = "5")] - #[cfg_attr( - feature = "with-serde", - serde( - serialize_with = "crate::proto::serializers::serialize_to_value", - deserialize_with = "crate::proto::serializers::deserialize_from_value" - ) - )] pub body: ::core::option::Option, /// Additional attributes that describe the specific event occurrence. \[Optional\]. /// Attribute keys MUST be unique (it is not allowed to have more than one diff --git a/opentelemetry-proto/tests/grpc_build.rs b/opentelemetry-proto/tests/grpc_build.rs index 4039239b7c..d09a13cd64 100644 --- a/opentelemetry-proto/tests/grpc_build.rs +++ b/opentelemetry-proto/tests/grpc_build.rs @@ -111,11 +111,10 @@ fn build_tonic() { .field_attribute(path, "#[cfg_attr(feature = \"with-serde\", serde(serialize_with = \"crate::proto::serializers::serialize_u64_to_string\", deserialize_with = \"crate::proto::serializers::deserialize_string_to_u64\"))]") } - // add custom serializer and deserializer for AnyValue - for path in ["common.v1.KeyValue.value", "logs.v1.LogRecord.body"] { - builder = builder - .field_attribute(path, "#[cfg_attr(feature =\"with-serde\", serde(serialize_with = \"crate::proto::serializers::serialize_to_value\", deserialize_with = \"crate::proto::serializers::deserialize_from_value\"))]"); - } + // special serializer and deserializer for value + // The Value::value field must be hidden + builder = builder + .field_attribute("common.v1.AnyValue.value", "#[cfg_attr(feature =\"with-serde\", serde(flatten, serialize_with = \"crate::proto::serializers::serialize_to_value\", deserialize_with = \"crate::proto::serializers::deserialize_from_value\"))]"); // flatten for path in ["metrics.v1.Metric.data", "metrics.v1.NumberDataPoint.value"] { diff --git a/opentelemetry-proto/tests/json_serde.rs b/opentelemetry-proto/tests/json_serde.rs index ba04cdf613..3295e682c1 100644 --- a/opentelemetry-proto/tests/json_serde.rs +++ b/opentelemetry-proto/tests/json_serde.rs @@ -518,14 +518,10 @@ mod json_serde { "arrayValue": { "values": [ { - "value": { - "stringValue": "foo" - } + "stringValue": "foo" }, { - "value": { - "stringValue": "bar" - } + "stringValue": "bar" } ] } @@ -557,14 +553,10 @@ mod json_serde { "arrayValue": { "values": [ { - "value": { - "stringValue": "foo" - } + "stringValue": "foo" }, { - "value": { - "intValue": 1337 - } + "intValue": "1337" } ] } @@ -1339,14 +1331,10 @@ mod json_serde { "arrayValue": { "values": [ { - "value": { - "stringValue": "many" - } + "stringValue": "many" }, { - "value": { - "stringValue": "values" - } + "stringValue": "values" } ] } @@ -1453,14 +1441,10 @@ mod json_serde { "arrayValue": { "values": [ { - "value": { - "stringValue": "many" - } + "stringValue": "many" }, { - "value": { - "stringValue": "values" - } + "stringValue": "values" } ] } From b6a108eedf130de39d25e6a03024bf772d6dfcc1 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Sat, 12 Oct 2024 09:58:40 -0700 Subject: [PATCH 3/7] Avoid redundant shutdown in LoggerProvider::drop when already shut down (#2195) Co-authored-by: Cijo Thomas --- opentelemetry-sdk/CHANGELOG.md | 1 + opentelemetry-sdk/src/logs/log_emitter.rs | 177 +++++++++++++++++++--- 2 files changed, 157 insertions(+), 21 deletions(-) diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md index ae8a3c7ca1..ab97a294f0 100644 --- a/opentelemetry-sdk/CHANGELOG.md +++ b/opentelemetry-sdk/CHANGELOG.md @@ -4,6 +4,7 @@ - Bump MSRV to 1.70 [#2179](https://github.com/open-telemetry/opentelemetry-rust/pull/2179) - Implement `LogRecord::set_trace_context` for `LogRecord`. Respect any trace context set on a `LogRecord` when emitting through a `Logger`. +- Improved `LoggerProvider` shutdown handling to prevent redundant shutdown calls when `drop` is invoked. [#2195](https://github.com/open-telemetry/opentelemetry-rust/pull/2195) ## v0.26.0 Released 2024-Sep-30 diff --git a/opentelemetry-sdk/src/logs/log_emitter.rs b/opentelemetry-sdk/src/logs/log_emitter.rs index 0317a33774..c6e0e79830 100644 --- a/opentelemetry-sdk/src/logs/log_emitter.rs +++ b/opentelemetry-sdk/src/logs/log_emitter.rs @@ -24,15 +24,26 @@ static NOOP_LOGGER_PROVIDER: Lazy = Lazy::new(|| LoggerProvider inner: Arc::new(LoggerProviderInner { processors: Vec::new(), resource: Resource::empty(), + is_shutdown: AtomicBool::new(true), }), - is_shutdown: Arc::new(AtomicBool::new(true)), }); #[derive(Debug, Clone)] -/// Creator for `Logger` instances. +/// Handles the creation and coordination of [`Logger`]s. +/// +/// All `Logger`s created by a `LoggerProvider` will share the same +/// [`Resource`] and have their created log records processed by the +/// configured log processors. This is a clonable handle to the `LoggerProvider` +/// itself, and cloning it will create a new reference, not a new instance of a +/// `LoggerProvider`. Dropping the last reference will trigger the shutdown of +/// the provider, ensuring that all remaining logs are flushed and no further +/// logs are processed. Shutdown can also be triggered manually by calling +/// the [`shutdown`](LoggerProvider::shutdown) method. +/// +/// [`Logger`]: opentelemetry::logs::Logger +/// [`Resource`]: crate::Resource pub struct LoggerProvider { inner: Arc, - is_shutdown: Arc, } /// Default logger name if empty string is provided. @@ -73,7 +84,7 @@ impl opentelemetry::logs::LoggerProvider for LoggerProvider { fn library_logger(&self, library: Arc) -> Self::Logger { // If the provider is shutdown, new logger will refer a no-op logger provider. - if self.is_shutdown.load(Ordering::Relaxed) { + if self.inner.is_shutdown.load(Ordering::Relaxed) { return Logger::new(library, NOOP_LOGGER_PROVIDER.clone()); } Logger::new(library, self.clone()) @@ -105,27 +116,21 @@ impl LoggerProvider { /// Shuts down this `LoggerProvider` pub fn shutdown(&self) -> LogResult<()> { if self + .inner .is_shutdown .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) .is_ok() { // propagate the shutdown signal to processors - // it's up to the processor to properly block new logs after shutdown - let mut errs = vec![]; - for processor in &self.inner.processors { - if let Err(err) = processor.shutdown() { - otel_warn!( - name: "logger_provider_shutdown_error", - error = format!("{:?}", err) - ); - errs.push(err); - } - } - + let errs = self.inner.shutdown(); if errs.is_empty() { Ok(()) } else { - Err(LogError::Other(format!("{errs:?}").into())) + otel_warn!( + name: "logger_provider_shutdown_error", + error = format!("{:?}", errs) + ); + Err(LogError::Other(format!("{:?}", errs).into())) } } else { otel_warn!( @@ -140,13 +145,28 @@ impl LoggerProvider { struct LoggerProviderInner { processors: Vec>, resource: Resource, + is_shutdown: AtomicBool, +} + +impl LoggerProviderInner { + /// Shuts down the `LoggerProviderInner` and returns any errors. + pub(crate) fn shutdown(&self) -> Vec { + let mut errs = vec![]; + for processor in &self.processors { + if let Err(err) = processor.shutdown() { + errs.push(err); + } + } + errs + } } impl Drop for LoggerProviderInner { fn drop(&mut self) { - for processor in &mut self.processors { - if let Err(err) = processor.shutdown() { - global::handle_error(err); + if !self.is_shutdown.load(Ordering::Relaxed) { + let errs = self.shutdown(); + if !errs.is_empty() { + global::handle_error(LogError::Other(format!("{:?}", errs).into())); } } } @@ -202,8 +222,8 @@ impl Builder { inner: Arc::new(LoggerProviderInner { processors: self.processors, resource, + is_shutdown: AtomicBool::new(false), }), - is_shutdown: Arc::new(AtomicBool::new(false)), }; // invoke set_resource on all the processors @@ -612,6 +632,89 @@ mod tests { assert!(!*flush_called.lock().unwrap()); } + #[test] + fn drop_test_with_multiple_providers() { + let shutdown_called = Arc::new(Mutex::new(false)); + let flush_called = Arc::new(Mutex::new(false)); + { + // Create a shared LoggerProviderInner and use it across multiple providers + let shared_inner = Arc::new(LoggerProviderInner { + processors: vec![Box::new(LazyLogProcessor::new( + shutdown_called.clone(), + flush_called.clone(), + ))], + resource: Resource::empty(), + is_shutdown: AtomicBool::new(false), + }); + + { + let logger_provider1 = LoggerProvider { + inner: shared_inner.clone(), + }; + let logger_provider2 = LoggerProvider { + inner: shared_inner.clone(), + }; + + let logger1 = logger_provider1.logger("test-logger1"); + let logger2 = logger_provider2.logger("test-logger2"); + + logger1.emit(logger1.create_log_record()); + logger2.emit(logger1.create_log_record()); + + // LoggerProviderInner should not be dropped yet, since both providers and `shared_inner` + // are still holding a reference. + } + // At this point, both `logger_provider1` and `logger_provider2` are dropped, + // but `shared_inner` still holds a reference, so `LoggerProviderInner` is NOT dropped yet. + } + // Verify shutdown was called during the drop of the shared LoggerProviderInner + assert!(*shutdown_called.lock().unwrap()); + // Verify flush was not called during drop + assert!(!*flush_called.lock().unwrap()); + } + + #[test] + fn drop_after_shutdown_test_with_multiple_providers() { + let shutdown_called = Arc::new(Mutex::new(0)); // Count the number of times shutdown is called + let flush_called = Arc::new(Mutex::new(false)); + + // Create a shared LoggerProviderInner and use it across multiple providers + let shared_inner = Arc::new(LoggerProviderInner { + processors: vec![Box::new(CountingShutdownProcessor::new( + shutdown_called.clone(), + flush_called.clone(), + ))], + resource: Resource::empty(), + is_shutdown: AtomicBool::new(false), + }); + + // Create a scope to test behavior when providers are dropped + { + let logger_provider1 = LoggerProvider { + inner: shared_inner.clone(), + }; + let logger_provider2 = LoggerProvider { + inner: shared_inner.clone(), + }; + + // Explicitly shut down the logger provider + let shutdown_result = logger_provider1.shutdown(); + assert!(shutdown_result.is_ok()); + + // Verify that shutdown was called exactly once + assert_eq!(*shutdown_called.lock().unwrap(), 1); + + // LoggerProvider2 should observe the shutdown state but not trigger another shutdown + let shutdown_result2 = logger_provider2.shutdown(); + assert!(shutdown_result2.is_err()); + + // Both logger providers will be dropped at the end of this scope + } + + // Verify that shutdown was only called once, even after drop + assert_eq!(*shutdown_called.lock().unwrap(), 1); + } + #[derive(Debug)] pub(crate) struct LazyLogProcessor { shutdown_called: Arc>, @@ -645,4 +748,36 @@ mod tests { Ok(()) } } + + #[derive(Debug)] + struct CountingShutdownProcessor { + shutdown_count: Arc>, + flush_called: Arc>, + } + + impl CountingShutdownProcessor { + fn new(shutdown_count: Arc>, flush_called: Arc>) -> Self { + CountingShutdownProcessor { + shutdown_count, + flush_called, + } + } + } + + impl LogProcessor for CountingShutdownProcessor { + fn emit(&self, _data: &mut LogRecord, _library: &InstrumentationLibrary) { + // nothing to do + } + + fn force_flush(&self) -> LogResult<()> { + *self.flush_called.lock().unwrap() = true; + Ok(()) + } + + fn shutdown(&self) -> LogResult<()> { + let mut count = self.shutdown_count.lock().unwrap(); + *count += 1; + Ok(()) + } + } } From caa4246ca3aea391375e1c504c3ae144809cc234 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Sat, 12 Oct 2024 10:29:11 -0700 Subject: [PATCH 4/7] Add tests to confirm known deadlock issues (#2199) --- opentelemetry-appender-tracing/Cargo.toml | 1 + opentelemetry-appender-tracing/src/layer.rs | 73 ++++++++++++++++++++- 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/opentelemetry-appender-tracing/Cargo.toml b/opentelemetry-appender-tracing/Cargo.toml index e443b91fba..38e2cb21d9 100644 --- a/opentelemetry-appender-tracing/Cargo.toml +++ b/opentelemetry-appender-tracing/Cargo.toml @@ -26,6 +26,7 @@ tracing-subscriber = { workspace = true, features = ["registry", "std", "env-fil tracing-log = "0.2" async-trait = { workspace = true } criterion = { workspace = true } +tokio = { workspace = true, features = ["full"]} [target.'cfg(not(target_os = "windows"))'.dev-dependencies] pprof = { version = "0.13", features = ["flamegraph", "criterion"] } diff --git a/opentelemetry-appender-tracing/src/layer.rs b/opentelemetry-appender-tracing/src/layer.rs index 1bfbef0fcf..fbbf8e4d97 100644 --- a/opentelemetry-appender-tracing/src/layer.rs +++ b/opentelemetry-appender-tracing/src/layer.rs @@ -208,17 +208,20 @@ const fn severity_of_level(level: &Level) -> Severity { #[cfg(test)] mod tests { use crate::layer; - use opentelemetry::logs::Severity; + use async_trait::async_trait; + use opentelemetry::logs::{LogResult, Severity}; use opentelemetry::trace::TracerProvider as _; use opentelemetry::trace::{TraceContextExt, TraceFlags, Tracer}; use opentelemetry::{logs::AnyValue, Key}; + use opentelemetry_sdk::export::logs::{LogBatch, LogExporter}; use opentelemetry_sdk::logs::{LogRecord, LoggerProvider}; use opentelemetry_sdk::testing::logs::InMemoryLogsExporter; use opentelemetry_sdk::trace; use opentelemetry_sdk::trace::{Sampler, TracerProvider}; - use tracing::error; + use tracing::{error, warn}; use tracing_subscriber::prelude::__tracing_subscriber_SubscriberExt; - use tracing_subscriber::Layer; + use tracing_subscriber::util::SubscriberInitExt; + use tracing_subscriber::{EnvFilter, Layer}; pub fn attributes_contains(log_record: &LogRecord, key: &Key, value: &AnyValue) -> bool { log_record @@ -238,6 +241,70 @@ mod tests { } // cargo test --features=testing + + #[derive(Clone, Debug, Default)] + struct ReentrantLogExporter; + + #[async_trait] + impl LogExporter for ReentrantLogExporter { + async fn export(&mut self, _batch: LogBatch<'_>) -> LogResult<()> { + // This will cause a deadlock as the export itself creates a log + // while still within the lock of the SimpleLogProcessor. + warn!(name: "my-event-name", target: "reentrant", event_id = 20, user_name = "otel", user_email = "otel@opentelemetry.io"); + Ok(()) + } + } + + #[test] + #[ignore = "See issue: https://github.com/open-telemetry/opentelemetry-rust/issues/1745"] + fn simple_processor_deadlock() { + let exporter: ReentrantLogExporter = ReentrantLogExporter; + let logger_provider = LoggerProvider::builder() + .with_simple_exporter(exporter.clone()) + .build(); + + let layer = layer::OpenTelemetryTracingBridge::new(&logger_provider); + + // Setting subscriber as global as that is the only way to test this scenario. + tracing_subscriber::registry().with(layer).init(); + warn!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "otel@opentelemetry.io"); + } + + #[test] + #[ignore = "While this test runs fine, this uses global subscriber and does not play well with other tests."] + fn simple_processor_no_deadlock() { + let exporter: ReentrantLogExporter = ReentrantLogExporter; + let logger_provider = LoggerProvider::builder() + .with_simple_exporter(exporter.clone()) + .build(); + + let layer = layer::OpenTelemetryTracingBridge::new(&logger_provider); + + // This filter will prevent the deadlock as the reentrant log will be + // ignored. + let filter = EnvFilter::new("debug").add_directive("reentrant=error".parse().unwrap()); + // Setting subscriber as global as that is the only way to test this scenario. + tracing_subscriber::registry() + .with(filter) + .with(layer) + .init(); + warn!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "otel@opentelemetry.io"); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + #[ignore = "While this test runs fine, this uses global subscriber and does not play well with other tests."] + async fn batch_processor_no_deadlock() { + let exporter: ReentrantLogExporter = ReentrantLogExporter; + let logger_provider = LoggerProvider::builder() + .with_batch_exporter(exporter.clone(), opentelemetry_sdk::runtime::Tokio) + .build(); + + let layer = layer::OpenTelemetryTracingBridge::new(&logger_provider); + + tracing_subscriber::registry().with(layer).init(); + warn!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "otel@opentelemetry.io"); + } + #[test] fn tracing_appender_standalone() { // Arrange From f1a6e164854bb86291e38cb530d050684c62b510 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Sat, 12 Oct 2024 18:27:38 -0700 Subject: [PATCH 5/7] [Metrics API] Mark structs non-exhaustive (#2202) --- opentelemetry/src/metrics/instruments/counter.rs | 2 ++ opentelemetry/src/metrics/instruments/gauge.rs | 2 ++ opentelemetry/src/metrics/instruments/histogram.rs | 1 + opentelemetry/src/metrics/instruments/up_down_counter.rs | 2 ++ opentelemetry/src/metrics/meter.rs | 1 + 5 files changed, 8 insertions(+) diff --git a/opentelemetry/src/metrics/instruments/counter.rs b/opentelemetry/src/metrics/instruments/counter.rs index 2d4ea1ec27..a03618a0f8 100644 --- a/opentelemetry/src/metrics/instruments/counter.rs +++ b/opentelemetry/src/metrics/instruments/counter.rs @@ -10,6 +10,7 @@ pub trait SyncCounter { /// An instrument that records increasing values. #[derive(Clone)] +#[non_exhaustive] pub struct Counter(Arc + Send + Sync>); impl fmt::Debug for Counter @@ -35,6 +36,7 @@ impl Counter { /// An async instrument that records increasing values. #[derive(Clone)] +#[non_exhaustive] pub struct ObservableCounter(Arc>); impl ObservableCounter { diff --git a/opentelemetry/src/metrics/instruments/gauge.rs b/opentelemetry/src/metrics/instruments/gauge.rs index 85042502f3..b1c14e1f04 100644 --- a/opentelemetry/src/metrics/instruments/gauge.rs +++ b/opentelemetry/src/metrics/instruments/gauge.rs @@ -10,6 +10,7 @@ pub trait SyncGauge { /// An instrument that records independent values #[derive(Clone)] +#[non_exhaustive] pub struct Gauge(Arc + Send + Sync>); impl fmt::Debug for Gauge @@ -35,6 +36,7 @@ impl Gauge { /// An async instrument that records independent readings. #[derive(Clone)] +#[non_exhaustive] pub struct ObservableGauge(Arc>); impl fmt::Debug for ObservableGauge diff --git a/opentelemetry/src/metrics/instruments/histogram.rs b/opentelemetry/src/metrics/instruments/histogram.rs index ceef12f860..d20826e998 100644 --- a/opentelemetry/src/metrics/instruments/histogram.rs +++ b/opentelemetry/src/metrics/instruments/histogram.rs @@ -10,6 +10,7 @@ pub trait SyncHistogram { /// An instrument that records a distribution of values. #[derive(Clone)] +#[non_exhaustive] pub struct Histogram(Arc + Send + Sync>); impl fmt::Debug for Histogram diff --git a/opentelemetry/src/metrics/instruments/up_down_counter.rs b/opentelemetry/src/metrics/instruments/up_down_counter.rs index 90f3804793..2fb3dcaaf0 100644 --- a/opentelemetry/src/metrics/instruments/up_down_counter.rs +++ b/opentelemetry/src/metrics/instruments/up_down_counter.rs @@ -12,6 +12,7 @@ pub trait SyncUpDownCounter { /// An instrument that records increasing or decreasing values. #[derive(Clone)] +#[non_exhaustive] pub struct UpDownCounter(Arc + Send + Sync>); impl fmt::Debug for UpDownCounter @@ -40,6 +41,7 @@ impl UpDownCounter { /// An async instrument that records increasing or decreasing values. #[derive(Clone)] +#[non_exhaustive] pub struct ObservableUpDownCounter(Arc>); impl fmt::Debug for ObservableUpDownCounter diff --git a/opentelemetry/src/metrics/meter.rs b/opentelemetry/src/metrics/meter.rs index 8af075af0f..7b3f851dd5 100644 --- a/opentelemetry/src/metrics/meter.rs +++ b/opentelemetry/src/metrics/meter.rs @@ -290,6 +290,7 @@ pub trait MeterProvider { /// ``` /// #[derive(Clone)] +#[non_exhaustive] pub struct Meter { pub(crate) instrument_provider: Arc, } From c3687d4fb6ff9ae8af6976f36e16b59fdaf310f4 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Sat, 12 Oct 2024 19:12:15 -0700 Subject: [PATCH 6/7] [Metrics-API] Mark no-op structs as pub(crate) (#2203) Co-authored-by: Cijo Thomas --- opentelemetry-sdk/src/metrics/meter.rs | 3 +- .../src/metrics/meter_provider.rs | 6 +- opentelemetry-sdk/src/metrics/mod.rs | 1 + opentelemetry-sdk/src/metrics/noop.rs | 78 +++++++++++++++++++ opentelemetry/src/global/metrics.rs | 2 +- opentelemetry/src/metrics/mod.rs | 2 +- opentelemetry/src/metrics/noop.rs | 12 +-- 7 files changed, 93 insertions(+), 11 deletions(-) create mode 100644 opentelemetry-sdk/src/metrics/noop.rs diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index 3ef7037f44..26169f9616 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -4,7 +4,6 @@ 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, @@ -18,6 +17,8 @@ use crate::metrics::{ pipeline::{Pipelines, Resolver}, }; +use super::noop::{NoopAsyncInstrument, NoopSyncInstrument}; + // maximum length of instrument name const INSTRUMENT_NAME_MAX_LENGTH: usize = 255; // maximum length of instrument unit name diff --git a/opentelemetry-sdk/src/metrics/meter_provider.rs b/opentelemetry-sdk/src/metrics/meter_provider.rs index 139346b54a..1468ed0f30 100644 --- a/opentelemetry-sdk/src/metrics/meter_provider.rs +++ b/opentelemetry-sdk/src/metrics/meter_provider.rs @@ -9,13 +9,15 @@ use std::{ use opentelemetry::{ global, - metrics::{noop::NoopMeter, Meter, MeterProvider, MetricsError, Result}, + metrics::{Meter, MeterProvider, MetricsError, Result}, KeyValue, }; use crate::{instrumentation::Scope, Resource}; -use super::{meter::SdkMeter, pipeline::Pipelines, reader::MetricReader, view::View}; +use super::{ + meter::SdkMeter, noop::NoopMeter, pipeline::Pipelines, reader::MetricReader, view::View, +}; /// Handles the creation and coordination of [Meter]s. /// diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index 8b1f33370a..e4c0de3026 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -47,6 +47,7 @@ pub(crate) mod internal; pub(crate) mod manual_reader; pub(crate) mod meter; mod meter_provider; +pub(crate) mod noop; pub(crate) mod periodic_reader; pub(crate) mod pipeline; pub mod reader; diff --git a/opentelemetry-sdk/src/metrics/noop.rs b/opentelemetry-sdk/src/metrics/noop.rs new file mode 100644 index 0000000000..cb58706cef --- /dev/null +++ b/opentelemetry-sdk/src/metrics/noop.rs @@ -0,0 +1,78 @@ +use opentelemetry::{ + metrics::{ + AsyncInstrument, InstrumentProvider, SyncCounter, SyncGauge, SyncHistogram, + SyncUpDownCounter, + }, + KeyValue, +}; + +/// A no-op instance of a `Meter` +#[derive(Debug, Default)] +pub(crate) struct NoopMeter { + _private: (), +} + +impl NoopMeter { + /// Create a new no-op meter core. + pub(crate) fn new() -> Self { + NoopMeter { _private: () } + } +} + +impl InstrumentProvider for NoopMeter {} + +/// A no-op sync instrument +#[derive(Debug, Default)] +pub(crate) struct NoopSyncInstrument { + _private: (), +} + +impl NoopSyncInstrument { + /// Create a new no-op sync instrument + pub(crate) fn new() -> Self { + NoopSyncInstrument { _private: () } + } +} + +impl SyncCounter for NoopSyncInstrument { + fn add(&self, _value: T, _attributes: &[KeyValue]) { + // Ignored + } +} + +impl SyncUpDownCounter for NoopSyncInstrument { + fn add(&self, _value: T, _attributes: &[KeyValue]) { + // Ignored + } +} + +impl SyncHistogram for NoopSyncInstrument { + fn record(&self, _value: T, _attributes: &[KeyValue]) { + // Ignored + } +} + +impl SyncGauge for NoopSyncInstrument { + fn record(&self, _value: T, _attributes: &[KeyValue]) { + // Ignored + } +} + +/// A no-op async instrument. +#[derive(Debug, Default)] +pub(crate) struct NoopAsyncInstrument { + _private: (), +} + +impl NoopAsyncInstrument { + /// Create a new no-op async instrument + pub(crate) fn new() -> Self { + NoopAsyncInstrument { _private: () } + } +} + +impl AsyncInstrument for NoopAsyncInstrument { + fn observe(&self, _value: T, _attributes: &[KeyValue]) { + // Ignored + } +} diff --git a/opentelemetry/src/global/metrics.rs b/opentelemetry/src/global/metrics.rs index 630e9e2ba7..e23a746966 100644 --- a/opentelemetry/src/global/metrics.rs +++ b/opentelemetry/src/global/metrics.rs @@ -7,7 +7,7 @@ type GlobalMeterProvider = Arc; /// The global `MeterProvider` singleton. static GLOBAL_METER_PROVIDER: Lazy> = - Lazy::new(|| RwLock::new(Arc::new(metrics::noop::NoopMeterProvider::new()))); + Lazy::new(|| RwLock::new(Arc::new(crate::metrics::noop::NoopMeterProvider::new()))); /// Sets the given [`MeterProvider`] instance as the current global meter /// provider. diff --git a/opentelemetry/src/metrics/mod.rs b/opentelemetry/src/metrics/mod.rs index ea69881596..4b89765d41 100644 --- a/opentelemetry/src/metrics/mod.rs +++ b/opentelemetry/src/metrics/mod.rs @@ -9,7 +9,7 @@ use thiserror::Error; mod instruments; mod meter; -pub mod noop; +pub(crate) mod noop; use crate::{Array, ExportError, KeyValue, Value}; pub use instruments::{ diff --git a/opentelemetry/src/metrics/noop.rs b/opentelemetry/src/metrics/noop.rs index d009a8a300..2c8f204a5c 100644 --- a/opentelemetry/src/metrics/noop.rs +++ b/opentelemetry/src/metrics/noop.rs @@ -39,13 +39,13 @@ impl MeterProvider for NoopMeterProvider { /// A no-op instance of a `Meter` #[derive(Debug, Default)] -pub struct NoopMeter { +pub(crate) struct NoopMeter { _private: (), } impl NoopMeter { /// Create a new no-op meter core. - pub fn new() -> Self { + pub(crate) fn new() -> Self { NoopMeter { _private: () } } } @@ -54,13 +54,13 @@ impl InstrumentProvider for NoopMeter {} /// A no-op sync instrument #[derive(Debug, Default)] -pub struct NoopSyncInstrument { +pub(crate) struct NoopSyncInstrument { _private: (), } impl NoopSyncInstrument { /// Create a new no-op sync instrument - pub fn new() -> Self { + pub(crate) fn new() -> Self { NoopSyncInstrument { _private: () } } } @@ -91,13 +91,13 @@ impl SyncGauge for NoopSyncInstrument { /// A no-op async instrument. #[derive(Debug, Default)] -pub struct NoopAsyncInstrument { +pub(crate) struct NoopAsyncInstrument { _private: (), } impl NoopAsyncInstrument { /// Create a new no-op async instrument - pub fn new() -> Self { + pub(crate) fn new() -> Self { NoopAsyncInstrument { _private: () } } } From bacb3dab7823150c7a49e42bc1b865a92b60b7b9 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Sun, 13 Oct 2024 00:14:11 -0700 Subject: [PATCH 7/7] Prevent lint warnings if internal-logs features is not enabled. (#2196) --- opentelemetry/src/global/internal_logging.rs | 32 ++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/opentelemetry/src/global/internal_logging.rs b/opentelemetry/src/global/internal_logging.rs index 3a5c24ba69..4c09f38b0c 100644 --- a/opentelemetry/src/global/internal_logging.rs +++ b/opentelemetry/src/global/internal_logging.rs @@ -23,12 +23,20 @@ macro_rules! otel_info { { tracing::info!( name: $name, target: env!("CARGO_PKG_NAME"), ""); } + #[cfg(not(feature = "internal-logs"))] + { + let _ = $name; // Compiler will optimize this out as it's unused. + } }; (name: $name:expr, $($key:ident = $value:expr),+ $(,)?) => { #[cfg(feature = "internal-logs")] { tracing::info!(name: $name, target: env!("CARGO_PKG_NAME"), $($key = $value),+, ""); } + #[cfg(not(feature = "internal-logs"))] + { + let _ = ($name, $($value),+); // Compiler will optimize this out as it's unused. + } }; } @@ -50,12 +58,20 @@ macro_rules! otel_warn { { tracing::warn!(name: $name, target: env!("CARGO_PKG_NAME"), ""); } + #[cfg(not(feature = "internal-logs"))] + { + let _ = $name; // Compiler will optimize this out as it's unused. + } }; (name: $name:expr, $($key:ident = $value:expr),+ $(,)?) => { #[cfg(feature = "internal-logs")] { tracing::warn!(name: $name, target: env!("CARGO_PKG_NAME"), $($key = $value),+, ""); } + #[cfg(not(feature = "internal-logs"))] + { + let _ = ($name, $($value),+); // Compiler will optimize this out as it's unused. + } }; } @@ -77,12 +93,20 @@ macro_rules! otel_debug { { tracing::debug!(name: $name, target: env!("CARGO_PKG_NAME"),""); } + #[cfg(not(feature = "internal-logs"))] + { + let _ = $name; // Compiler will optimize this out as it's unused. + } }; (name: $name:expr, $($key:ident = $value:expr),+ $(,)?) => { #[cfg(feature = "internal-logs")] { tracing::debug!(name: $name, target: env!("CARGO_PKG_NAME"), $($key = $value),+, ""); } + #[cfg(not(feature = "internal-logs"))] + { + let _ = ($name, $($value),+); // Compiler will optimize this out as it's unused. + } }; } @@ -104,11 +128,19 @@ macro_rules! otel_error { { tracing::error!(name: $name, target: env!("CARGO_PKG_NAME"), ""); } + #[cfg(not(feature = "internal-logs"))] + { + let _ = $name; // Compiler will optimize this out as it's unused. + } }; (name: $name:expr, $($key:ident = $value:expr),+ $(,)?) => { #[cfg(feature = "internal-logs")] { tracing::error!(name: $name, target: env!("CARGO_PKG_NAME"), $($key = $value),+, ""); } + #[cfg(not(feature = "internal-logs"))] + { + let _ = ($name, $($value),+); // Compiler will optimize this out as it's unused. + } }; }