DistributedContext: SDK initialization model is very different from Tracer and Meter

[SergeyKanzhelev]

Initialization model chosen for DistributedContext is very different than for Tracer and Meter. Ideally, there should not be a need for an explicit override of the DistributedContext. Perhaps even move implementation in API

[tarekgh]

@SergeyKanzhelev There is a difference between Tracer/Meter and DistributedContext. Tracer and Meter are abstract interfaces while DistributedContext is just a concrete type that can be created by the Builder. Following the Tracer/Meter way will require making the DistributedContext/Builder to be abstract which I don't think we need to complicate the interfaces for DistributedContext just to follow the factory pattern. Right now the SDK will need to add the following simple line when it gets loaded

            DistributedContext.Carrier = AsyncLocalDistributedContextCarrier.Instance;

I am open to any other opinion if there are real scenarios that require going with the same Factory pattern as the Tracer and Meter.

By the way, for the Tracer, I recall we have discussed before to have the static Tracer.Default which holds the default active tracer. The default tracer will be no-op one when there is no SDK loaded and then will get overridden when loading the SDK. @lmolkova is this still the plan or we are not going to do that? I am asking because this initialization pattern is similar to the DistributedContext initialization and also will simplify the library code (when there is no SDK) by simply using the Default property and no need to use any Factory class to get the tracer.

[MikeGoldsmith]

Are we going to allow consumers to replace the DistributedContext implementation like we do for Tracer/Meter? If yes, then I think having a consistent approach with using an abstract base class with default implementation makes sense. I think having a consistent API, whether that uses builders or static defaults, is a good thing.

Regarding default tracer, OTEP PR 74 indicates we should default to a No-Op Tracer until one has been registered. It doesn't say how we set the Tracer instance or make reference to it (eg via static)

[tarekgh]

Are we going to allow consumers to replace the DistributedContext implementation like we do for Tracer/Meter?

I am not seeing any reason we need to have the DC be abstract and replaced. It is a really simple key-value list carrier. That is the point I am trying to make. I am open to change that if we really think in a scenario that requires replacement as I may be missing something.

Regarding default tracer, OTEP PR 74 indicates we should default to a No-Op Tracer until one has been registered. It doesn't say how we set the Tracer instance or make reference to it (eg via static)

That is right. Now we should think which way is simpler for the library code to get the default tracer and how the SDK can override the default. what are your thoughts about that? writing some library snippet code demonstrate the different options that can help.

[MikeGoldsmith]

I agree we probably don't need DistributedContext to be be abstract and won't be replaced in application code.

Currently we utilise TracerFactory to get Tracer instances, and I believe typically expect application code to manage an instance of that (eg DI). I think some sort of static default tracer type make sense, which would default to the API's NoopTracer.

To follow on from that, there has been some discussion around having a limited buffer of spans that can be created / ended before a Tracer has been registered. Once a tracer is registered, the spans are then passed to the tracer. I'm not certain how that would work right now, but wanted to highlight it as I think it's related.

[tarekgh]

Thanks, @MikeGoldsmith. looks static default tracer would making sense and that will be the same way we initialize DistributedContext.

@SergeyKanzhelev @lmolkova could you please let's know what are your thoughts around that and keeping the DistributedContext initialization as what we have today?

[tarekgh]

@SergeyKanzhelev do we need to keep this issue opened?

[cijothomas]

Closing as not relevant now.