-
Notifications
You must be signed in to change notification settings - Fork 436
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
Unify builders across signals #2220
Unify builders across signals #2220
Conversation
3cdff17
to
8b09a51
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2220 +/- ##
======================================
Coverage 79.5% 79.6%
======================================
Files 121 121
Lines 21116 20918 -198
======================================
- Hits 16805 16666 -139
+ Misses 4311 4252 -59 ☔ View full report in Codecov by Sentry. |
There were some concerns about having |
8b09a51
to
fc0d3c0
Compare
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.
Left few comments but looks good overall.
Please check the comments and see if they make sense.
yes I suggest to hide Arc inside. |
1d0ea2f
to
02574ef
Compare
@stormshield-fabs Thanks a lot! I think the following are to be addressed before merge. We also need to write a migration guide in the PR description showing before/after, so users can easily (relatively) react to this. Lets keep InstrumentationScope simple, and accept the perf cost now, given it is infrequent operation. Rename library-signal to something like signal_with_scope/better names. |
f3c67cd
to
a6bd2db
Compare
I've addressed the pending comments and added changelog entries (feel free to edit them!) |
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.
Could you modify the PR description to show the before/after example for logs/metrics/traces, so users can easily do the migration?
@stormshield-fabs oops we just cause some merge conflicts! Could you resolve them. |
a6bd2db
to
afeb1aa
Compare
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.
Left some minor suggestions.
Great work @stormshield-fabs!!
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. Thanks for the changes.
@stormshield-fabs Can you resolve conflicts and address the nits, and we can merge! As suggested offline, if you can add a migration guide in the PR description, similar to #2221, that'd be awesome! (The next release is a flood of breaking changes, so we need to write an overall migration guide, which will refer to the sub-migration-guides within each PR description.) |
afeb1aa
to
bd34c52
Compare
@stormshield-fabs I took the liberty of fixing minor merge conflicts in this PR, as it was introduced by changes from my own PR. Hope that's fine :) |
Fixes #1527 and fixes #2164
Changes
The consensus reached during the SIG meeting is that we'd like to unify the builder APIs across signals, limit duplication and keep our traits object-safe.
opentelemetry
InstrumentationLibrary::new
InstrumentationLibrary
toInstrumentationScope
InstrumentationLibraryBuilder
toInstrumentationScopeBuilder
LoggerProvider::versioned_logger
andTracerProvider::versioned_tracer
LoggerProvider::logger_builder
,TracerProvider::tracer_builder
andMeterProvider::versioned_meter
LoggerProvider::logger_with_scope
,TracerProvider::logger_with_scope
,MeterProvider::meter_with_scope
global::meter_with_version
withglobal::meter_with_scope
global::tracer_with_scope
opentelemetry_sdk
:InstrumentationLibrary
re-export and itsScope
alias, useopentelemetry::InstrumentationLibrary
instead.LoggerProvider::versioned_logger
,TracerProvider::versioned_tracer
MeterProvider::versioned_meter
LoggerProvider::logger_with_scope
,TracerProvider::logger_with_scope
,MeterProvider::meter_with_scope
Migration guide (example for
logger
but the changes are similar across all 3 signals)Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes