Add GlobalPropagators API and have instrumentation default to it #1428
Conversation
| { | ||
| private static PropagationContext defaultPropagationContext = default; | ||
|
|
||
| public override ISet<string> Fields => null; |
There was a problem hiding this comment.
anything better than null?
| /// </summary> | ||
| public static TextMapPropagator DefaultTextMapPropagator { get; set; } = new NoOpTextMapPropagator(); | ||
|
|
||
| internal static void Reset() |
src/OpenTelemetry.Api/Context/Propagation/NoOpTextMapPropagator.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Api/Context/Propagation/NoOpTextMapPropagator.cs
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1428 +/- ##
==========================================
- Coverage 81.85% 81.75% -0.10%
==========================================
Files 227 229 +2
Lines 6089 6095 +6
==========================================
- Hits 4984 4983 -1
- Misses 1105 1112 +7
|
|
I have some comments about the API on the issue. I'll probably be in the minority on this one but basically, I'm seeing some inconsistency between having a builder for most things and a static global for this. If a user has multiple TracerProviders, with different propagation schemes, then what is the effect of something setting I feel like the API we should provide is like SetResource, which is also a global-yet-scoped type of thing... using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.SetResource(new Resource(...))
.SetPropagator(new CompositePropagator(...))
.Build();In API we would have to provide some way to discover active TracerProviders and their state (default propagator, resource, etc.) like 'TracerProviders.Active[0].DefaultPropagator = some_propagator`. ✌️ 🕊️ ❤️ |
| new TraceContextPropagator(), | ||
| new BaggagePropagator(), | ||
| }); | ||
| public TextMapPropagator Propagator { get; set; } = Propagators.DefaultTextMapPropagator; |
There was a problem hiding this comment.
Could be a timing thing here. When options are created they'll copy the ref for the current default propagator. If user sets through SDK the global propagator after that, it won't be reflected. Could leave it null here and then when it is used in instrumentation do options.Propagator ?? Propagators.DefaultTextMapPropagator. It would be a slight perf hit but it makes it hot-swappable.
|
Merging. |
Fixes #581
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes