Skip to content
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 Global Propagators API #581

Closed
MikeGoldsmith opened this issue Apr 7, 2020 · 8 comments · Fixed by #1428
Closed

Add Global Propagators API #581

MikeGoldsmith opened this issue Apr 7, 2020 · 8 comments · Fixed by #1428
Assignees
Labels
pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package

Comments

@MikeGoldsmith
Copy link
Member

Propagators were moved from Tracer into a separate area so they can be consumed by meters too.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#global-propagators

@cijothomas
Copy link
Member

Here's the proposal:
Introduce the following in API:
Propagators.Default {set;get} = NoOpPropagator;

In SDK,
In SDK.cs class, do the following inside a static ctor()
{
Propagators.Default = new Composite(TraceContext, Baggage);
}

(Logging does not rely on SDK.cs, so we "touch" SDK.cs from LoggingProvider, to trigger above initialization)

In each instrumentations,
By default, use the Propagators.Default.
But keep the ability to override InstrumentationOptions.IPropagator, so that users can override propagators per instrumentation basis.

@CodeBlanch
Copy link
Member

@cijothomas Should we add a builder method as a convenience like...

public TracerProviderBuilder SetPropagator(Propagator propagator)
{
   Propagators.Default = propagator ?? throw new ArgumentNullException(nameof(propagator));
}

But that also begs the question: What if the user wants to have multiple TracerProviderSdks in the process using different propagation styles? I feel like we should either fully support multiple TracerProviders in the process or limit to just one 😄

If we want to support multiple we could move from static to a field on TracerProviderSdk and then give instrumentation a way to grab it from the TracerProviderSdk they are registered for. I feel like we could make that work, and it would be useful. If instrumentation & exporters could resolve the TracerProvider they are registered for we could use that for other things like retrieving Resource, which would eliminate the need to store it on each activity.

@cijothomas
Copy link
Member

cijothomas commented Oct 30, 2020

If one wants to have separate propagator per TracerProviderSdk, then they can achieve it, as every instrumentation allows override.
So provider1 = instrumentation1 with propagator1, provider2 = instrumentation1 with propagator2 is possible.

(About activity avoiding the storage of Resource, yes, i am still thinking of the best approach. #1397)

@CodeBlanch
Copy link
Member

I figured you'd say that 😄 Agree they can make it work. But I also feel like we could provide a nice and consistent API. Some stuff is static, some stuff is a builder, some stuff is carried around on activity. Code smells right? Everything working off the builder/TracerProvider would be nice, and I don't think that difficult to achieve. The goal being: One way to configure propagation regardless of how many TracerProviders you want to use 🚀 And it would be the same API used for propagation & resources 🚀🚀

@cijothomas
Copy link
Member

Ok, so there will be 3 places to set Propagator.

  1. The global one. Everyone respects this unless overridden.
  2. The one via TracerProviderBuilder, which will be respected by all the instrumentations built as part of that provider. This overrides the global
  3. Then instrumentation specific one which can override anything set by either of the above.

Or are you saying we just need 1,2 and eliminate the instrumentation specific one.?

@CodeBlanch
Copy link
Member

I'm thinking we should only have 2 & 3. I know the spec says "Global" but that doesn't really make sense for our (current) API/SDK. Our "global" is really scoped to a TracerProvider, at least in my mind. Each TracerProvider should have a Resource and a Propagator. Any instrumentation, processor, exporter, etc., should be able to resolve its parent TracerProvider and it's "global" state. Since everything is constructed through the builder, should be easy to pass TracerProvider as a "parent" ref? Something like that.

@cijothomas
Copy link
Member

PR #1428 makes the following:
Add propagators API which can set the global, default propagator. Propagators.DefaultTextMapPropagator.
There is no ability to set a propagator specific to a TracerProvider.
There is ability to set propagator to each instrumentation, which leverages propagators. They can still access the global one, or ignore it and use the instrumentation level propagator. So this PR is implementing 1,3 now.

Open to make the link PR doe 2 as well, but it doesn't eliminate the need for 1 as its a spec requirement.

@cijothomas
Copy link
Member

PR 1428 added Propagators.GetDefault in API. (to be consumed by instrumentations)
Propagators.SetDefault in Sdk, so as to allow override from the SDK.
Propagators.Default is set to the Composite(Baggage,TraceContext) by the Sdk.

To be discussed and finalized:
Should we still keep the per instrumentation overrides? its not required by the spec, and hence it may be eliminated.
Need to see if there is enough us case for the keeping per instrumentation propapagator override

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants