From c454832a83ea8fd9dabcf9e762e8997e05043eed Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Wed, 12 Mar 2025 09:56:17 -0700 Subject: [PATCH 1/5] fix: Avoid stringifying int values unless necessary --- opentelemetry-appender-tracing/CHANGELOG.md | 5 +++ opentelemetry-appender-tracing/src/layer.rs | 43 +++++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/opentelemetry-appender-tracing/CHANGELOG.md b/opentelemetry-appender-tracing/CHANGELOG.md index dacc012cf7..2817291fea 100644 --- a/opentelemetry-appender-tracing/CHANGELOG.md +++ b/opentelemetry-appender-tracing/CHANGELOG.md @@ -42,6 +42,11 @@ transparent to most users. implementations (SDK, processor, exporters) to leverage this additional information to determine if an event is enabled. +- `u64` and `usize` values are stored as `opentelemetry::logs::AnyValue::Int` +when conversion is feasible. Otherwise stored as +`opentelemetry::logs::AnyValue::String`. This avoids unnecessary string +allocation when values can be represented in their original types. + ## 0.28.1 Released 2025-Feb-12 diff --git a/opentelemetry-appender-tracing/src/layer.rs b/opentelemetry-appender-tracing/src/layer.rs index 6a15bc9dba..e3baa477e5 100644 --- a/opentelemetry-appender-tracing/src/layer.rs +++ b/opentelemetry-appender-tracing/src/layer.rs @@ -114,6 +114,19 @@ impl tracing::field::Visit for EventVisitor<'_, LR> { .add_attribute(Key::new(field.name()), AnyValue::from(value)); } + fn record_u64(&mut self, field: &tracing::field::Field, value: u64) { + match i64::try_from(value) { + Ok(signed) => { + self.log_record + .add_attribute(Key::new(field.name()), AnyValue::from(signed)); + } + Err(_) => { + self.log_record + .add_attribute(Key::new(field.name()), AnyValue::from(format!("{value:?}"))); + } + } + } + // TODO: Remaining field types from AnyValue : Bytes, ListAny, Boolean } @@ -331,7 +344,11 @@ mod tests { let _guard = tracing::subscriber::set_default(subscriber); // Act - error!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "otel@opentelemetry.io"); + let small_u64value: u64 = 42; + let big_u64value: u64 = u64::MAX; + let small_usizevalue: usize = 42; + let big_usizevalue: usize = usize::MAX; + error!(name: "my-event-name", target: "my-system", event_id = 20, small_u64value, big_u64value, small_usizevalue, big_usizevalue, user_name = "otel", user_email = "otel@opentelemetry.io"); assert!(logger_provider.force_flush().is_ok()); // Assert TODO: move to helper methods @@ -362,9 +379,9 @@ mod tests { // Validate attributes #[cfg(not(feature = "experimental_metadata_attributes"))] - assert_eq!(log.record.attributes_iter().count(), 3); - #[cfg(feature = "experimental_metadata_attributes")] assert_eq!(log.record.attributes_iter().count(), 7); + #[cfg(feature = "experimental_metadata_attributes")] + assert_eq!(log.record.attributes_iter().count(), 11); assert!(attributes_contains( &log.record, &Key::new("event_id"), @@ -380,6 +397,26 @@ mod tests { &Key::new("user_email"), &AnyValue::String("otel@opentelemetry.io".into()) )); + assert!(attributes_contains( + &log.record, + &Key::new("small_u64value"), + &AnyValue::Int(42.into()) + )); + assert!(attributes_contains( + &log.record, + &Key::new("big_u64value"), + &AnyValue::String(format!("{}", u64::MAX).into()) + )); + assert!(attributes_contains( + &log.record, + &Key::new("small_usizevalue"), + &AnyValue::Int(42.into()) + )); + assert!(attributes_contains( + &log.record, + &Key::new("big_usizevalue"), + &AnyValue::String(format!("{}", u64::MAX).into()) + )); #[cfg(feature = "experimental_metadata_attributes")] { assert!(attributes_contains( From e7eb7d1a35efc9c47fa074e2290cb4c82a4b4d40 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Wed, 12 Mar 2025 10:14:52 -0700 Subject: [PATCH 2/5] fix experimental stuff --- opentelemetry-appender-tracing/src/layer.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/opentelemetry-appender-tracing/src/layer.rs b/opentelemetry-appender-tracing/src/layer.rs index e3baa477e5..a570ff1d47 100644 --- a/opentelemetry-appender-tracing/src/layer.rs +++ b/opentelemetry-appender-tracing/src/layer.rs @@ -115,6 +115,10 @@ impl tracing::field::Visit for EventVisitor<'_, LR> { } fn record_u64(&mut self, field: &tracing::field::Field, value: u64) { + #[cfg(feature = "experimental_metadata_attributes")] + if is_duplicated_metadata(field.name()) { + return; + } match i64::try_from(value) { Ok(signed) => { self.log_record @@ -790,6 +794,10 @@ mod tests { TraceFlags::SAMPLED ); + for attribute in log.record.attributes_iter() { + println!("key: {:?}, value: {:?}", attribute.0, attribute.1); + } + // Attributes can be polluted when we don't use this feature. #[cfg(feature = "experimental_metadata_attributes")] assert_eq!(log.record.attributes_iter().count(), 4); From fbe687a9a5917f3ba094693517b3023f0572bb37 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Wed, 12 Mar 2025 12:18:18 -0700 Subject: [PATCH 3/5] code comment --- opentelemetry-appender-tracing/src/layer.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/opentelemetry-appender-tracing/src/layer.rs b/opentelemetry-appender-tracing/src/layer.rs index a570ff1d47..47e58cbc7c 100644 --- a/opentelemetry-appender-tracing/src/layer.rs +++ b/opentelemetry-appender-tracing/src/layer.rs @@ -114,6 +114,8 @@ impl tracing::field::Visit for EventVisitor<'_, LR> { .add_attribute(Key::new(field.name()), AnyValue::from(value)); } + // TODO: We might need to do similar for record_i128,record_u128 too + // to avoid stringification, unless needed. fn record_u64(&mut self, field: &tracing::field::Field, value: u64) { #[cfg(feature = "experimental_metadata_attributes")] if is_duplicated_metadata(field.name()) { From 114986144a9de4ff0fa4feff3f15f93da593a518 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Thu, 13 Mar 2025 09:27:19 -0700 Subject: [PATCH 4/5] use more idiomatic match --- opentelemetry-appender-tracing/src/layer.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/opentelemetry-appender-tracing/src/layer.rs b/opentelemetry-appender-tracing/src/layer.rs index 47e58cbc7c..5eb22524e3 100644 --- a/opentelemetry-appender-tracing/src/layer.rs +++ b/opentelemetry-appender-tracing/src/layer.rs @@ -121,15 +121,12 @@ impl tracing::field::Visit for EventVisitor<'_, LR> { if is_duplicated_metadata(field.name()) { return; } - match i64::try_from(value) { - Ok(signed) => { - self.log_record - .add_attribute(Key::new(field.name()), AnyValue::from(signed)); - } - Err(_) => { - self.log_record - .add_attribute(Key::new(field.name()), AnyValue::from(format!("{value:?}"))); - } + if let Ok(signed) = i64::try_from(value) { + self.log_record + .add_attribute(Key::new(field.name()), AnyValue::from(signed)); + } else { + self.log_record + .add_attribute(Key::new(field.name()), AnyValue::from(format!("{value:?}"))); } } From 5f4b837063f5e3fb3fe64966ea994bce5c48e994 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Thu, 13 Mar 2025 09:48:50 -0700 Subject: [PATCH 5/5] fmt --- opentelemetry-appender-tracing/src/layer.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry-appender-tracing/src/layer.rs b/opentelemetry-appender-tracing/src/layer.rs index 5eb22524e3..9333df861f 100644 --- a/opentelemetry-appender-tracing/src/layer.rs +++ b/opentelemetry-appender-tracing/src/layer.rs @@ -123,10 +123,10 @@ impl tracing::field::Visit for EventVisitor<'_, LR> { } if let Ok(signed) = i64::try_from(value) { self.log_record - .add_attribute(Key::new(field.name()), AnyValue::from(signed)); + .add_attribute(Key::new(field.name()), AnyValue::from(signed)); } else { self.log_record - .add_attribute(Key::new(field.name()), AnyValue::from(format!("{value:?}"))); + .add_attribute(Key::new(field.name()), AnyValue::from(format!("{value:?}"))); } }