From c5cd7a913c8f47557c2e2dfba1171be2b4bbbe19 Mon Sep 17 00:00:00 2001 From: David Calavera <1050+calavera@users.noreply.github.com> Date: Tue, 23 Sep 2025 17:03:40 -0700 Subject: [PATCH] fix: Ensure InstrumentationScope is properly propagated. With the current implementation, there is no way for an InstrumentationScope to export its version and attributes correctly. The reason why this is happening is because when the InstrumentationScope is exported, the target key is always `Some`. This is because when the code creates the `scope_map`, the target key always has a value, either comming from the log record's target, or from the name of the instrumentation scope. That means that when the code calls `InstrumentationScope::from((&InstrumentationScope, Option>))`, the option is always `Some`. This causes the new InstrumentationScope to always from version and attributes from the incoming scope. This change assumes that if the target and the instrumentation scope name match, the user wants to preserve the version and attributes of the incoming scope. There are other alternatives implementations that check whether the log record target is None that could also be valid, but I think both would have the same outcome. --- opentelemetry-proto/src/transform/logs.rs | 143 ++++++++++++++++++++-- 1 file changed, 133 insertions(+), 10 deletions(-) diff --git a/opentelemetry-proto/src/transform/logs.rs b/opentelemetry-proto/src/transform/logs.rs index 65d0598216..b190ea8e98 100644 --- a/opentelemetry-proto/src/transform/logs.rs +++ b/opentelemetry-proto/src/transform/logs.rs @@ -195,10 +195,7 @@ pub mod tonic { let scope_logs = scope_map .into_iter() .map(|(key, log_data)| ScopeLogs { - scope: Some(InstrumentationScope::from(( - log_data.first().unwrap().1, - Some(key.into_owned().into()), - ))), + scope: Some(new_instrumentation_scope(log_data.first().unwrap().1, key)), schema_url: resource.schema_url.clone().unwrap_or_default(), log_records: log_data .into_iter() @@ -217,6 +214,22 @@ pub mod tonic { schema_url: resource.schema_url.clone().unwrap_or_default(), }] } + + /// Ensure that InstrumentationScope is propagated correctly. + /// These are the rules: + /// + /// 1. If the log scope name matches the key, return a new scope with all the fields from the original scope. + /// 2. Otherwise, return a new scope with the key as the name of the scope, and the rest of the fields empty. + fn new_instrumentation_scope( + log_scope: &opentelemetry::InstrumentationScope, + key: Cow<'static, str>, + ) -> InstrumentationScope { + if key == log_scope.name() { + return InstrumentationScope::from((log_scope, None)); + } + + InstrumentationScope::from((log_scope, Some(key))) + } } #[cfg(test)] @@ -227,6 +240,7 @@ mod tests { use opentelemetry::logs::LoggerProvider; use opentelemetry::time::now; use opentelemetry::InstrumentationScope; + use opentelemetry::KeyValue; use opentelemetry_sdk::error::OTelSdkResult; use opentelemetry_sdk::logs::LogProcessor; use opentelemetry_sdk::logs::SdkLoggerProvider; @@ -246,6 +260,7 @@ mod tests { fn create_test_log_data( instrumentation_name: &str, _message: &str, + attrs: Vec, ) -> (SdkLogRecord, InstrumentationScope) { let processor = MockProcessor {}; let logger = SdkLoggerProvider::builder() @@ -255,16 +270,17 @@ mod tests { let mut logrecord = logger.create_log_record(); logrecord.set_timestamp(now()); logrecord.set_observed_timestamp(now()); - let instrumentation = - InstrumentationScope::builder(instrumentation_name.to_string()).build(); + let instrumentation = InstrumentationScope::builder(instrumentation_name.to_string()) + .with_attributes(attrs) + .build(); (logrecord, instrumentation) } #[test] fn test_group_logs_by_resource_and_scope_single_scope() { let resource = Resource::builder().build(); - let (log_record1, instrum_lib1) = create_test_log_data("test-lib", "Log 1"); - let (log_record2, instrum_lib2) = create_test_log_data("test-lib", "Log 2"); + let (log_record1, instrum_lib1) = create_test_log_data("test-lib", "Log 1", vec![]); + let (log_record2, instrum_lib2) = create_test_log_data("test-lib", "Log 2", vec![]); let logs = [(&log_record1, &instrum_lib1), (&log_record2, &instrum_lib2)]; let log_batch = LogBatch::new(&logs); @@ -284,8 +300,8 @@ mod tests { #[test] fn test_group_logs_by_resource_and_scope_multiple_scopes() { let resource = Resource::builder().build(); - let (log_record1, instrum_lib1) = create_test_log_data("lib1", "Log 1"); - let (log_record2, instrum_lib2) = create_test_log_data("lib2", "Log 2"); + let (log_record1, instrum_lib1) = create_test_log_data("lib1", "Log 1", vec![]); + let (log_record2, instrum_lib2) = create_test_log_data("lib2", "Log 2", vec![]); let logs = [(&log_record1, &instrum_lib1), (&log_record2, &instrum_lib2)]; let log_batch = LogBatch::new(&logs); @@ -311,4 +327,111 @@ mod tests { assert_eq!(scope_logs_1.log_records.len(), 1); assert_eq!(scope_logs_2.log_records.len(), 1); } + + #[test] + fn test_group_logs_by_resource_preserving_scope_attributes_when_log_target_empty() { + let resource = Resource::builder().build(); + let (log_record1, instrum_lib1) = + create_test_log_data("lib1", "Log 1", vec![KeyValue::new("key1", "value1")]); + let (log_record2, instrum_lib2) = + create_test_log_data("lib2", "Log 2", vec![KeyValue::new("key2", "value2")]); + + let logs = [(&log_record1, &instrum_lib1), (&log_record2, &instrum_lib2)]; + let log_batch = LogBatch::new(&logs); + let resource: ResourceAttributesWithSchema = (&resource).into(); // Convert Resource to ResourceAttributesWithSchema + let grouped_logs = + crate::transform::logs::tonic::group_logs_by_resource_and_scope(log_batch, &resource); + + assert_eq!(grouped_logs.len(), 1); + let resource_logs = &grouped_logs[0]; + assert_eq!(resource_logs.scope_logs.len(), 2); + + let scope_logs_1 = &resource_logs + .scope_logs + .iter() + .find(|scope| scope.scope.as_ref().unwrap().name == "lib1") + .unwrap(); + let scope_logs_2 = &resource_logs + .scope_logs + .iter() + .find(|scope| scope.scope.as_ref().unwrap().name == "lib2") + .unwrap(); + + assert_eq!(1, scope_logs_1.scope.as_ref().unwrap().attributes.len()); + assert_eq!(1, scope_logs_2.scope.as_ref().unwrap().attributes.len()); + } + + #[test] + fn test_group_logs_by_resource_preserving_scope_attributes_when_log_target_matching() { + let resource = Resource::builder().build(); + let (mut log_record1, instrum_lib1) = + create_test_log_data("lib1", "Log 1", vec![KeyValue::new("key1", "value1")]); + let (mut log_record2, instrum_lib2) = + create_test_log_data("lib2", "Log 2", vec![KeyValue::new("key2", "value2")]); + + let logs = [(&log_record1, &instrum_lib1), (&log_record2, &instrum_lib2)]; + let log_batch = LogBatch::new(&logs); + let resource: ResourceAttributesWithSchema = (&resource).into(); // Convert Resource to ResourceAttributesWithSchema + let grouped_logs = + crate::transform::logs::tonic::group_logs_by_resource_and_scope(log_batch, &resource); + + // This makes the grouping to match the existent InstrumentationScope, preserving the scope attributes. + log_record1.set_target("lib1"); + log_record2.set_target("lib2"); + + assert_eq!(grouped_logs.len(), 1); + let resource_logs = &grouped_logs[0]; + assert_eq!(resource_logs.scope_logs.len(), 2); + + let scope_logs_1 = &resource_logs + .scope_logs + .iter() + .find(|scope| scope.scope.as_ref().unwrap().name == "lib1") + .unwrap(); + let scope_logs_2 = &resource_logs + .scope_logs + .iter() + .find(|scope| scope.scope.as_ref().unwrap().name == "lib2") + .unwrap(); + + assert_eq!(1, scope_logs_1.scope.as_ref().unwrap().attributes.len()); + assert_eq!(1, scope_logs_2.scope.as_ref().unwrap().attributes.len()); + } + + #[test] + fn test_group_logs_by_resource_ignoring_scope_attributes_when_log_target_not_matching() { + let resource = Resource::builder().build(); + let (mut log_record1, instrum_lib1) = + create_test_log_data("lib1", "Log 1", vec![KeyValue::new("key1", "value1")]); + let (mut log_record2, instrum_lib2) = + create_test_log_data("lib2", "Log 2", vec![KeyValue::new("key2", "value2")]); + + // This makes the grouping to not match the existent InstrumentationScope, ignoring the scope attributes. + log_record1.set_target("target1"); + log_record2.set_target("target2"); + + let logs = [(&log_record1, &instrum_lib1), (&log_record2, &instrum_lib2)]; + let log_batch = LogBatch::new(&logs); + let resource: ResourceAttributesWithSchema = (&resource).into(); // Convert Resource to ResourceAttributesWithSchema + let grouped_logs = + crate::transform::logs::tonic::group_logs_by_resource_and_scope(log_batch, &resource); + + assert_eq!(grouped_logs.len(), 1); + let resource_logs = &grouped_logs[0]; + assert_eq!(resource_logs.scope_logs.len(), 2); + + let scope_logs_1 = &resource_logs + .scope_logs + .iter() + .find(|scope| scope.scope.as_ref().unwrap().name == "target1") + .unwrap(); + let scope_logs_2 = &resource_logs + .scope_logs + .iter() + .find(|scope| scope.scope.as_ref().unwrap().name == "target2") + .unwrap(); + + assert!(scope_logs_1.scope.as_ref().unwrap().attributes.is_empty()); + assert!(scope_logs_2.scope.as_ref().unwrap().attributes.is_empty()); + } }