-
Notifications
You must be signed in to change notification settings - Fork 463
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
Refactor/deprecate trace config #2303
Refactor/deprecate trace config #2303
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2303 +/- ##
=======================================
- Coverage 79.6% 79.5% -0.2%
=======================================
Files 123 123
Lines 21263 21293 +30
=======================================
- Hits 16940 16938 -2
- Misses 4323 4355 +32 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
"tracing-jaeger", | ||
)])), | ||
) | ||
.with_resource(Resource::new(vec![KeyValue::new( |
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.
we could potentially offer an easier ways for users who is just trying to add service name. For another time!
@@ -308,10 +310,77 @@ impl Builder { | |||
} | |||
|
|||
/// The sdk [`crate::trace::Config`] that this provider will use. | |||
#[deprecated( |
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.
Please add a changelog too.
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. Lets add changelog too, will merge after one more approval.
Note: This will be part of 0.27.1 as there is no actual method removal, just deprecation warning this time. Actual removal will be 0.28.0 time only
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, with nit comments.
@@ -106,6 +106,7 @@ impl ZipkinPipelineBuilder { | |||
)); | |||
cfg | |||
} else { | |||
#[allow(deprecated)] |
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.
Would be good to cleanup this instead of having deprecated code. Not a blocker for this PR though :)
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.
it seems like Zipkin needs to be updated to follow the builder pattern that was added for otlp also, its currently using the pipeline version.
I will try and get this refactored to not use the allow.
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.
maybe there can be a followup for the zipkin to match the otlp crate.
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.
Its best to do as a follow up to keep the scope for this PR smaller.
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.
yup, that's what I was thinking. I went simple with the approach on this PR, but I would like to open another to move to the Builder pattern if that would be welcomed
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.
my current change actually ignores the config, I'm going to revert to using the deprecated annotation, and then do a followup PR to move to the builder pattern to avoid too many changes in this PR.
Moving away from trace::Config
making this more consistent with the other signal providers open-telemetry#2294
3c1ba0c
to
be6b8dd
Compare
3a37cc8
to
7e6645c
Compare
resolves: #2294
Changes
Moving away from
trace::Config
and.with_config(...)
to remain consistent with the MeterProvider, andLoggerProvider
.Migration Guide
Moving away from
TracerProvider::builder().with_config(trace::Config::default())
.Simple Example
The provider interface should be similar to the old method. For example, a
TracerProvider
being configured.Advanced Example
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes