Skip to content

fix: Ensure InstrumentationScope is properly propagated. #3175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: Ensure InstrumentationScope is properly propagated. #3175

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5cd7a9
fix: Ensure InstrumentationScope is properly propagated.
calavera Sep 24, 2025
827bad7
Merge branch 'main' into propagate_instrumentation_scope
lalitb Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 133 additions & 10 deletions opentelemetry-proto/src/transform/logs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider pushing this down into InstrumentationScope::from? I've not dug into it in depth but it looks like the only place it is being used is the code you're touching, and i'm wondering if all in cases, the callers are going to expect this behaviour anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it would be great to document, somewhere, exactly how target relates to instrumentation scope. It took me a while to grok what's happening here:

This could be on the fn that does applies the relevant logic. Perhaps something like (overly detailed description to make sure i've not missed something 😄 ):

When you explicitly specify target in a log (e.g., error!(target: "security.audit", ...)), it overrides the default module path and becomes the InstrumentationScope.name
when the log is exported to OpenTelemetry backends.

If the target differs from the InstrumentationScope name, the exported scope will contain only that target
string as the name, with empty version and attributes.

This allows you to logically group logs under custom identifiers (like "postgres", "authentication", etc.) rather
than module paths, but at the cost of losing the instrumentation library's metadata.

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)]
Expand All @@ -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;
Expand All @@ -246,6 +260,7 @@ mod tests {
fn create_test_log_data(
instrumentation_name: &str,
_message: &str,
attrs: Vec<KeyValue>,
) -> (SdkLogRecord, InstrumentationScope) {
let processor = MockProcessor {};
let logger = SdkLoggerProvider::builder()
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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");

Comment on lines +372 to +381
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log record targets are being set after the log batch is created and grouped. This means the test is not actually testing the scenario described in the comment. Move these lines before creating the log batch to test the intended behavior.

Suggested change
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");
// This makes the grouping to match the existent InstrumentationScope, preserving the scope attributes.
log_record1.set_target("lib1");
log_record2.set_target("lib2");
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);

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be missing something, but I think copilot is right on this one - its not clear to me why the target is set after the call to group_logs_by_resource_and_scope

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());
}
}