fix(proto): preserve InstrumentationScope version and attributes#3332
Conversation
When logs have a target set, the InstrumentationScope's version and attributes were being discarded during OTLP export. The target parameter now correctly overrides only the scope name, preserving other metadata. This behaviour was introduced in PR open-telemetry#1869 to avoid exporting the appender crate's version when the scope name was overridden by target. However, PRs open-telemetry#2735 and open-telemetry#2796 subsequently changed both appenders to use empty scopes, so this concern no longer applies. Additionally, users who explicitly create scopes via logger_with_scope() would reasonably expect their metadata to be preserved. Fixes open-telemetry#3276
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3332 +/- ##
=======================================
+ Coverage 83.4% 83.5% +0.1%
=======================================
Files 127 127
Lines 25197 25241 +44
=======================================
+ Hits 21026 21091 +65
+ Misses 4171 4150 -21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi there, first time contributing here so please let me know if I need to change anything (or if I'm not allowed to contribute etc). Sorry if I've done anything wrong! |
|
@scottgerring yep, I did look at his work as a reference point for getting started on it. Do you think this is still needed? Happy to make any changes as required or close if not? I'd forgotten about it 😅 |
As there's a couple folks who've hit it now and this looks like a clean change to me I reckon we should merge it. @cijothomas wdyt? |
|
@cijothomas this one looks like it's been hit with the merge queue issues too ? |
|
@scottgerring / @cijothomas feel free to tag me when you come across any flaky tests - hopefully we can burn those down so that CI is a bit more reliable and approved PRs are less likely to get kicked out of the merge queue. I see it was updated with main so hopefully that fixed the flaky tests it encountered |
|
Hi @cijothomas, I'm wondering whether there's anything I can do on this one to get it merged? Or would you prefer I close the PR? (Just trying to wrap up some loose ends). |
|
Hi @scottgerring, I'm not sure whether you might be able to help get this one merged (or let me know if I should close it)? Sorry to chase! |
Hey @djvcom , @cijothomas and @lalitb should be back on deck this week and hold the merge keys; pinging for visibility |
Added to merge-queue. There are some Github issues going on right now, so this will be merged as soon as they are resolved. |
5 participants
Summary
When logs have a target set, the
InstrumentationScope's version and attributes were being discarded during OTLP export. The target parameter now correctly overrides only the scope name, preserving other metadata.Fromimplementations forInstrumentationScopeto preserve version and attributes when target is providedFromimplementationsgroup_logs_by_resource_and_scopeContext
This behaviour was introduced in PR #1869 to avoid exporting the appender crate's version when the scope name was overridden by target. However, PRs #2735 and #2796 subsequently changed both appenders to use empty scopes, so this concern no longer applies. Additionally, users who explicitly create scopes via
logger_with_scope()would reasonably expect their metadata to be preserved.Test plan
Fromimplementationsgroup_logs_by_resource_and_scopewith target setcargo test -p opentelemetry-proto --all-features --libpassescargo clippy -p opentelemetry-proto --all-features -- -DwarningspassesFixes #3276