fix(proto): preserve InstrumentationScope version and attributes#3332
fix(proto): preserve InstrumentationScope version and attributes#3332djvcom wants to merge 7 commits intoopen-telemetry:mainfrom
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.2% 83.3% +0.1%
=======================================
Files 128 128
Lines 25045 25089 +44
=======================================
+ Hits 20858 20923 +65
+ Misses 4187 4166 -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! |
| attributes: Attributes::from(library.attributes().cloned()).0, | ||
| ..Default::default() | ||
| } | ||
| InstrumentationScope { |
There was a problem hiding this comment.
not introduced by this PR, but this area is quite expensive due to copy/allocation etc. #3307 exposes the cost of this conversion!
Just FYI, nothing for this PR. The whole idea of target and instrumentation scope needs to be revisited to improve the performance.
There was a problem hiding this comment.
@djvcom Out of curiosity, can you elaborate on how did you hit this issue yourselves? The log appenders shipped in this repo always set a target and the scope attributes are empty, so this should have caused no issue. So wondering how did you hit the issue here. This is merely to help us understand the space better which will help future perf improvements!
There was a problem hiding this comment.
I was creating a scope manually as well (i've been building a little rust project to help me test collector configurations are valid). I doubt that most users would ever encounter it, and it was only because I was looking to verify and assert against resource and scope attrs that I noticed it, and the issue raised. All of my use cases I use the tracing appender, so I won't even benefit from this change in my day to day, but still thought it worth addressing.
|
@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? |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@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 |
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