-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add Unwrap method on tracer provider wrapper and use it in zpagesextension #14006
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.
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
Add Unwrap method on tracer provider wrapper and use it in zpagesextension #14006
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (72.72%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #14006 +/- ##
==========================================
- Coverage 91.67% 91.65% -0.02%
==========================================
Files 654 654
Lines 42659 42659
==========================================
- Hits 39107 39100 -7
- Misses 2738 2744 +6
- Partials 814 815 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I think we should document that Unwrap
method though. Maybe in both component.TelemetrySettings
and telemetry.Factory
?
// If the TracerProvider was wrapped by the service implementation, access the underlying SDK provider | ||
if wrapped, ok := tp.(interface{ Unwrap() trace.TracerProvider }); ok { | ||
tp = wrapped.Unwrap() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a for loop, like in errors.Is? i.e. keep unwrapping until we reach one without an Unwrap method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would make sense. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
5bdfd09
Description
This allows components to access SDK-specific
TracerProvider
methods (inzpagesextension
's case,RegisterSpanProcessor
), without requiringcomponentattribute
to have a dependency on the official SDK.This is an alternative proposal to #13947
Link to tracking issue
Updates #13842
Testing
Should be covered by
TestServiceTelemetryZPages
. (In fact the "missing coverage" should be covered by that test as well, but it's in another module.)