Avoid overriding configuration of tracer provider #1633
Conversation
ijsong
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
lizthegrey,
MrAlias and
XSAM
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1633 +/- ##
=====================================
Coverage 77.1% 77.2%
=====================================
Files 128 128
Lines 6735 6742 +7
=====================================
+ Hits 5196 5205 +9
+ Misses 1288 1286 -2
Partials 251 251
|
There was a problem hiding this comment.
This looks good to me, though as @MrAlias mentioned in Slack it would be good if we went one step further and avoided exporting the TracerProvider.ApplyConfig method and related Config struct. It looks like they're only used in a couple places that should be able to be rewritten using the new options.
Tracking this topic here so the scope of this PR isn't overloaded. |
ijsong
force-pushed
the
tracer-provider-config
branch
from
62a661b
to
665fcb0
Compare
|
It seems to be unrelated. Waiting time for reconnection maybe not enough sometimes. |
MrAlias
added
area:trace
|
@ijsong it looks like |
This change adds `WithDefaultSampler` and `WithSpanLimits` to the tracer provider and removed `WithConfig` from it. Before this change, `WithConfig` is the only way to set sampler or limits of a span. However, it is prone to misuse, since `WithConfig` can override tracing configurations that are configured by `WithResource` or `WithIDGenerator`. Thus to fix this, it adds new functional options - `WithDefaultSampler` and `WithSpanLimits` and removes `WithConfig`. Resolves open-telemetry#1631.
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
ijsong
force-pushed
the
tracer-provider-config
branch
from
18ec5ad
to
4117ec1
Compare
| @@ -56,6 +56,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
| | Windows | 1.14 | amd64 | | |||
| | Windows | 1.15 | 386 | | |||
| | Windows | 1.14 | 386 | | |||
| - Added `WithDefaultSampler` and `WithSpanLimits` to tracer provider. (#1633) | |||
There was a problem hiding this comment.
It seems it is the wrong place to put this changelog.
There was a problem hiding this comment.
Sorry to not have checked carefully.
There was a problem hiding this comment.
That is fine. This should be handled by an automatic tool, which lowers everyone's mental burden. ;)
This change adds
WithDefaultSamplerandWithSpanLimitsto the tracer provider and removedWithConfigfrom it.Before this change,
WithConfigis the only way to set sampler or limits of a span. However, it is prone to misuse, sinceWithConfigcan override tracing configurations that are configured byWithResourceorWithIDGenerator. Thus to fix this, it adds new functional options -WithDefaultSamplerandWithSpanLimitsand removesWithConfig.Resolves #1631.