Opentelemetry-dotnet: Replace TracerFactory with TracerProvider

[MikeGoldsmith]

Once the API (#573) has TraceProvider defined, the SDK also needs to implement it. We currently use a TracerFactory to build tracers, we can probably move most of the logic from that to the TracerProvider implementation.

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#tracer-creation

[eddynaka]

@MikeGoldsmith , the PR that I create is renaming the TracerFactory* to TracerProvider*. I would like to know what else should we change to complete this.

Thanks

[longility]

@eddynaka Do you still plan on working on this? I think I can take a stab at it if you aren't working on this.

@MikeGoldsmith
Based on back and forth comments on the closed PR. This is what I gathered as potential tasks.

1. TracerProvider to be a singleton those has a reference to TracerFactory and exposes API based on spec.
2. TracerFactory becomes "internal" and not expected to be used. I will have to see how to hide that if possible.
3. Update docs to where there isn't references to TracerFactory, but only to TracerProvider.

[eddynaka]

@longility , ok! go for it! thanks :)

[longility]

I will try to have something by Monday if not sooner.

[cijothomas]

@longility I am also looking at the config pieces , but feel free to go ahead with this approach.
Also - are you in our gitter channel? Its faster to quickly send messages there.

[longility]

I have not been in gitter channel. I will try it out. I'll try to do things incrementally and get feedback.

[longility]

As I'm studying the code, it is a bit trickier so I'm going to need some feedback considering I'm a newbie in this codebase and may not see the impact.

I would like to understand how we want to consume before going into implementation detail.

I'm going to start with consumer usage taking our first example in the README.md.

Current

using (var tracerFactory = TracerFactory.Create(builder => builder
        .UseZipkin(options => {})
        .SetResource(Resources.CreateServiceResource("http-client-test")))
{
    // Obtain Tracer from the factory above.
    var tracer = tracerFactory.GetTracer("zipkin-test");
}

ref https://github.com/open-telemetry/opentelemetry-dotnet#basic-configuration

Option 1

Remove usage and knowledge of TracerFactory

using (var tracerProvider = TracerProvider.Create(builder => builder
        .UseZipkin(options => {})
        .SetResource(Resources.CreateServiceResource("http-client-test")))
{
    // Obtain Tracer from the provider above.
    var tracer = tracerProvider.GetTracer("zipkin-test");
}

Option 2

Expected knowledge and usage of TracerFactory still.

using (var tracerFactory = TracerFactory.Create(builder => builder
        .UseZipkin(options => {})
        .SetResource(Resources.CreateServiceResource("http-client-test")))
using (var tracerProvider = TracerProvider.Create(tracerFactory))
{
    // Obtain Tracer from the provider above.
    var tracer = tracerProvider.GetTracer("zipkin-test");
}

Option 3

Expected knowledge and usage of TracerFactory still but without dependency injected (uncomfortable but is it necessary?)

using (var tracerFactory = TracerFactory.Create(builder => builder
        .UseZipkin(options => {})
        .SetResource(Resources.CreateServiceResource("http-client-test")))
{
    // Obtain Tracer from the provider above.
    var tracer = TracerProvider.Current.GetTracer("zipkin-test");
}

Other options? Something else?

I'm not used to this factory / singleton pattern so struggling a bit to mix. I tend to work on enterprise applications where I'm initializing lifetimes such as singleton at the DI container level.

[longility]

Now that I realize there is a difference between SDK and API, I'm seeing more of it now such as TracerProvider is specific to SDK, which is OpenTelemetry project and not OpenTelemetry.Api.

[cijothomas]

@longility TracerProvider is an API concept. https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#obtaining-a-tracer

It must be implemented in API, so libraries who depend only on API can get a Tracer from TracerProvider.

[longility]

Thanks for catching that! You are right @cijothomas.

[longility]

Here I go with bold change proposals with limited experience and knowledge...

Proposal

1. TracerProvider not to be a singleton, separate object as singleton container

Prior Art

Both go and js approaches this way. One difference between them is that go has one singleton container while js has specific singleton containers (i.e. one for metric and one for tracer).
https://github.com/open-telemetry/opentelemetry-go/blob/master/api/global/global.go
https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-api/src/api/trace.ts

Thoughts

• This makes sense to me to keep TracerProvider focused with the responsibility of creating tracers and not have the responsibility of its lifetime. Also, I think personally as a consumer of OT, I would not care about this singleton container(s) as I would be leveraging OpenTelemetry.AspNetCore. If I needed the TracerProvider, I would inject ITracerProvider. However, I probably would try to keep it simple and just capture traces based on adapters only.
• I can go either way on one single singleton container like go or specific singleton containers like js.
• I would do at least two PRs to reduce risk and make review easier, one is to introduce singleton container(s) and second is to introduce TracerProvider (and maybe replace TracerFactory)
  

  What it might look like (compared to options in other comment above)

// configuration
var tracerProvider = new TracerProvider(builder => builder
        .UseZipkin(options => {})
        .SetResource(Resources.CreateServiceResource("http-client-test"));
OpenTelemetryApi.TracerProvider = tracerProvider; // singleton container...naming may need some work...

// usage
var tracer = OpenTelemetryApi.TracerProvider.GetTracer("zipkin-test");

OR similar to Activity

// configuration
var tracerProvider = new TracerProvider(builder => builder
        .UseZipkin(options => {})
        .SetResource(Resources.CreateServiceResource("http-client-test"));
TracerProvider.Current = tracerProvider;

// usage
var tracer = TracerProvider.Current.GetTracer("zipkin-test");

2. More consistent naming with noop or default identifier

Prior Art

Both js and go have noop span, noop tracer, and noop tracer provider.
https://github.com/open-telemetry/opentelemetry-js/tree/master/packages/opentelemetry-api/src/trace
https://github.com/open-telemetry/opentelemetry-go/tree/master/api/trace

Before I can approach and tackle this (assuming I'm capable and right person for the job), I would need guidance or feedback of proposals.

[longility]

Alright, I have a WIP that I'm actually comfortable with (slimmed down to allow better reviewing of approach):
https://github.com/longility/opentelemetry-dotnet/commit/f700828635bace0517142b06029093d39b16270c

Not sure what the best way to communicate the change is. I'll do my best here.

1. BlankSpan -> NoopSpan
   
   
   
   • Simplify by removing singleton pattern.
   
   
   • Keep consistent with no-op naming.
   
   
2. ProxyTracer -> NoopTracer
   
   
   
   • Simplify by removing proxy pattern.
   
   
   • Keep consistent with no-op naming.
   
   
3. TracerFactoryBase split into NoopTracerProvider and TracerProvider
   
   
   
   • TracerProvider can GetTracer(..) from the global instance of ITracerProvider, which is defaulted to NoopTracerProvider.
   
   
   • Naming TracerProvider to stay in line with spec.
   
   
4. Introduce ITracerProvider
   
   
   
   • For injection and structure to different implementation of tracer provider and expectation when setting on TracerProvider (the weird "singleton"/global container for ITracerProvider.
   
   
5. TracerFactory -> TracerProviderSdk
   
   
   
   • Consistent naming with TracerSdk and SpanSdk
   
   
   • TracerProviderSdk sets itself as TracerProvider.GlobalInstance. I haven't read through details, but I think this approach also fixes #630
   
   
6. OpenTelemetry.Hosting DI using ITracerProvider
   
   
   
   • Consumers can use ITracerProvider to inject the TracerProviderSdk singleton.
   
   

Before I can go any further, I need feedback. I think I provided as much options and an approach that I'm comfortable with as a more refined proposal.

[cijothomas]

@longility Thanks for sharing this.
I added comments in the commit diff itself.
Can you share examples here on how the usage would look like? Specifically, how does libraries get Tracers? They should get noops if no SDK available. If SDK available, they should get real Tracers.

[longility]

I will answer based on current WIP commit and focused on your question here.

If the SDK is not initialized, calling TracerProvider.GlobalInstance will give you no-ops as TracerProvider.GlobalInstance is initialized with a new instance of NoopTracerProvider. If they use TracerProvider.GetTracer(..) static method, it will delegate to TracerProvider.GlobalInstance.GetTracer(..).

When TracerProviderSdk is being instantiated, I currently have this line GlobalInstance = this; which when you call, TracerProvider.GlobalInstance, it is the instance of TracerProviderSdk. Yes, it won't support multiple tracer providers globally.

I will follow up with your comments in the commit in a bit.

[cijothomas]

As mentioned in the PR - the issue not solved is when libraries call TracerProvider.GlobalInstance, before SDK is done iniitalizing (i.e setting GlobalInstance = this in your PR), then the library will always live with the NoOp. Libraries mostly get Tracer once at its startup and then cache it.

[longility]

Libraries mostly get Tracer once at its startup and then cache it.

That comment helped.

Updated v2 at April 27th, 7:30 AM CST

https://github.com/longility/opentelemetry-dotnet/commit/3118007d6764c5be0952782c0f859399ef0e3cd7

@cijothomas here is v2 based on your feedback.

1. TracerProvider will be an abstract class following patterns of Tracer and TelemetrySpan.
2. NoopTracerProvider and TracerProviderSdk that inherits TracerProvider
3. TracerFactory is now TracerProviderSdk with no relationship to TracerFactoryBase.
4. Keep ProxyTracer but introduce NoopTracer to separate concerns. ProxyTracer is used at the factory/singleton level.
5. TracerFactoryBase is now TracerFactory that manages the default instance of TracerFactory, where you can create multiple TracerFactory that associates to a TracerProvider if you need to.
6. I believe the redesign to TracerFactory also fixes #630 ??

Usage (added 8:12 AM CST)

// this is the initial tracer factory with NoopTracerProvider as the associated tracer provider
TracerFactory.Default;
// this gets the proxy tracer which points to the default noop tracer provider
TracerFactory.GetTracer(..);

// create new tracer factory with TracerProviderSdk
var sdkTracerFactory = new TracerFactory(TracerProviderSdk.Create(b => b...));
// Sets new default and updates proxy tracer like how it was doing before.
TracerFactory.SetDefault(sdkTracerFactory);

// points to the new default tracer provider's get tracer
TracerFactory.GetTracer(..); 

// support multiple tracer factories
var tracerFactory2 = new TracerFactory(TracerProviderSdk.Create(b => b...)); 

// throws exception. only allow to set one default like how it was doing before. no change.
TracerFactory.SetDefault(tracerFactory2); 

Question (added 8:27 AM CST)

With this approach, do we need to support multiple tracer factories, or can we simplify by having only one tracer factory where we would set a default TracerProvider? It would look more like this https://github.com/longility/opentelemetry-dotnet/blob/6088c5a5f6d06e26d1067c0565332546560bb59f/src/OpenTelemetry.Api/Trace/TracerFactory.cs

// this is the default tracer provider, which is NoopTracerProvider
TracerFactory.Default;
// this gets the proxy tracer which points to the default noop tracer provider
TracerFactory.GetTracer(..);

// Sets TracerProviderSdk as new  default and updates proxy tracer like how it was doing before.
TracerFactory.SetDefault(TracerProviderSdk.Create(b => b...));

// points to the new default tracer provider's get tracer
TracerFactory.GetTracer(..); 

// support multiple tracer providers
var tracerProviderSdk2 = TracerProviderSdk.Create(b => b...);

// throws exception. only allow to set one default like how it was doing before. no change.
TracerFactory.SetDefault(tracerProviderSdk2); 

[cijothomas]

@longility Can you open PRs in draft mode with your changes? This makes it easier to try out things. Once we are ready, simply mark PR as ready for final review from draft mode.

[longility]

@cijothomas #641 has WIP with two meaningful commit: 1 refactor and 1 wip introduce tracer provider.

If we want to split into two PRs, one dependent on the other, we can do that. Let me know what is best to review and tackle this as this is pretty large. I'm very comfortable with git to break things up as needed. I just have it all together so that you have a full working branch to try out things.

[cijothomas]

Done.