Implement Named Tracers

[z1c0]

The "Named Tracers and Meters" RFC (https://github.com/open-telemetry/oteps/blob/master/text/0016-named-tracers.md) has been approved and was added to the specification (open-telemetry/opentelemetry-specification#264).

Please make sure to also update the documentation if necessary (tracer, meter creation).

OpenTelemetry language repositories now need to implement this mechanisms.

[bg451]

This is turning out to be a much bigger code change than expected, especially around the node SDK.

Following the specification instructions, I started by adding a TracerFactory type that has the function getTracer(name?: string, version?: string). In @opentelemetry/core, initGlobalTracer becomes initGlobalTracerFactory and we end up with something like

getTracer(name?: string, version?: string) {
  return globalTracerFactoryDelegate.getTracer(name, string);
}

so that, ergonomically, existing code feels the same.

However, for the node and basic tracer code, a kind of ugly issue rears its head when it comes to sharing exporters among tracers created by the tracer factory. Let's say that we have a simple sdk factory:

class BasicTracerFactory implements types.TracerFactory {
  constructor(config: BasicTracerConfig) {
    this._defaultConfig = config;
    this._defaultTracer = new BasicTracer(config);
  }
  getTracer(name?: string, version?: string) {
    return this._defaultTracer
  }
}

If we add an exporter, all we have to do is opentelemetry.getTracer().addSpanProcessor(...). However, if we begin to actually create new tracers based on name, e.g.

getTracer(name? : string, version?: string) {
  if (name) {
    // this would probably be more like a get or create tracer operation
    return new BasicTracer({ name: name, config...})
  }
  return this._defaultTracer;
}

span processors added via opentelemetry.getTracer().addSpanProcessor would not be on the tracer opentelemetry.getTracer('http-plugin'). To solve this, addSpanProcessor and other methods would need to be added to the factory and so BasicTracerFactory would become responsible for managing all shared configuration for tracers. Not ideal at all.

Instead of using a factory, I propose adding a new method to types.Tracer: withComponent(name: string, version?: string): Tracer. Implementations can either return the original tracer or return a wrapper class that maintains a reference to the original tracer while attaching the component to all spans. The same thing can be added to Meter (#402).

Thoughts @open-telemetry/node-approvers @draffensperger @dyladan>

[dyladan]

@bg451 would you expect span processors added via opentelemetry.getTracer().addSpanProcessor to be on opentelemetry.getTracer('http-plugin')? When I read the spec I read that as creation (if needed) of a new Tracer with its own set of configs and span processors.

[bg451]

@dyladan Yeah I would. The person using opentelemetry.getTracer('http-plugin') is writing an shared instrumentation library (not an end user), so they wouldn't be doing any configuration and also doesn't know where the end user wants the spans to go.

[dyladan]

This just feels like deviating from the spec a little too much for me. Could the tracer factory return a wrapper class around the global tracer that implemented naming but delegated everything else to the wrapped tracer?

FWIW the ruby implementation also would not satisfy your constraint open-telemetry/opentelemetry-ruby#114. I didn't find another OpenTelemetry language that has implemented named tracers/meters yet.

Reading through their PR conversation and the associated issue, it seems pretty clear that they don't understand the need for named tracers or what they may be used for. The OTEP and associated spec PR is really unhelpful in this regard. Maybe @z1c0 can be of some help here since he wrote the OTEP and the spec around it. I must admit the named tracers/meters spec left me with more questions than answers and it seems I am not the only one.

[bg451]

The otep provides an example of something similar in the Prior art section:

existing (global) Tracers could return additional indirection objects (called e.g. TraceComponent), which would be able to produce spans for specifically named traced components.

My understanding of named tracers is that traditionally 3rd party libraries ould instrument themselves with opentracing and use the global tracer. This way, users could also get visibility of what was happening in their dependencies. There was this tag in opentracing called component that would specify where the instrumentation was, so all spans created in and by gRPC or express or node-mongodb-native would have the tag component=node-mongo-native set on them. This is super tedious though and only some libraries did this so the concept of named tracers came about, where the 3rd party library could do tracer = getTracer('node-mongo-native') and not have to attach the component tag manually. @danielkhan could also help explain some of the benefits of having this 3rd party library naming as well.

edit:
One other thing, is that from a types pov, opentelemetry.getTracer().addSpanProcessor should not be allowed as getTracer returns an interface, not a concrete BasicTracer. If we want to move forward with the factory approach, tracers can't have dynamic configuration, meaning that spanProcessors would have to go into the config object as opposed to being appended through a method.

[bg451]

I think this is worth bringing up to the specification because it is really not clear what the desired side effects are.

[yurishkuro]

To solve this, addSpanProcessor and other methods would need to be added to the factory ... Not ideal at all.

Why is it not ideal? If tracers are moving one level down by becoming namespaced (instead of being a singleton), then exporters naturally have to move one level up, to the factory.

[obecny]

Can we simply allow two things so you would be able to inject the exporter during the initialisation, but we can add function to tracer to update this at any given time too in case you want to use a different exporter later.

[dyladan]

I think this is worth bringing up to the specification because it is really not clear what the desired side effects are.

I think it is probably worth bringing up with spec. Even if we decide what we want, the spec should make it more clear than it does. If the spec isn't cleared up, then different implementations may have different behaviors as we already see with the Ruby PR implementing in a naive way that would not share span processors among multiple tracers.

A

const defaultTracer = TracerFactory.getTracer().addSpanProcessor(p1);
const httpTracer = TracerFactory.getTracer('http'); // Should this tracer contain p1?

B

const httpTracer = TracerFactory.getTracer('http').addSpanProcessor(p1); 
const defaultTracer = TracerFactory.getTracer(); // Should this tracer contain p1?

C

const defaultTracer = TracerFactory.getTracer();
const httpTracer = TracerFactory.getTracer('http');
defaultTracer.addSpanProcessor(p1)
// should httpTracer contain p1?

• According to @bg451 at least A should be yes.
• The naive Factory would answer no to all questions
• The ruby implementation answers no to all questions

[yurishkuro]

All 3 are incorrect.

[z1c0]

As @yurishkuro said, all 3 are incorrect.
Setting a spanProcessor must move up to the TracerFactory

I didn't find another OpenTelemetry language that has implemented named tracers/meters yet.

Maybe the .NET implementation helps (open-telemetry/opentelemetry-dotnet#239)

[dyladan]

Thank you @z1c0 that's what I thought but I wanted to make sure.

Perhaps the spec should mention that setting span processor moves to the factory in order to minimize confusion between implementations. In this case, the ruby implementation needs to be amended. The current version (https://github.com/open-telemetry/opentelemetry-ruby/blob/7122835f6cc874e02d75e94078dafce6cbb4a99c/sdk/lib/opentelemetry/sdk/trace/tracer_factory.rb) does not have a mechanism to set the span processor on the tracer factory.

[z1c0]

Thanks @dyladan, I can see how this causes confusion.
I will make sure to update the specification where necessary and comment on the ruby repository.

[draffensperger]

I personally like the withComponent idea, which would take care of adding the 'component' label, but otherwise delegate to the same core tracer. Then we could have span processors that could be added like new ExcludeComponentsSpanProcessor(['mongodb']) could be added to the span processor chain to just drop all mongodb spans for example on the floor to prevent them from being exported.

That said, I agree we should discuss this in the spec. Being the first language to implement this also feels tricky since we are the first real tester of it.

[dyladan]

@draffensperger Ruby and .NET also have implemented but Ruby is a very minimal implementation. .NET was implemented by the OTEP and spec writer.

withComponent doesn't give the core benefit which is that naming tracers becomes the default recommended approach. If you build a tracer without a name, nothing stops you from never calling withComponent and many libraries may never do so. If the factory getTracer method takes a name and version argument it becomes much more clear that you're expected to name and version them.

[obecny]

If I understand correctly in my case it should look like this

import * as opentelemetry from '@opentelemetry/core';
const { SimpleSpanProcessor } = require('@opentelemetry/tracer-basic');
const { ConsoleExporter } = require('@opentelemetry/console-exporter');
import { WebTracer } from '@opentelemetry/tracer-web';
import { DocumentLoad } from '@opentelemetry/plugin-document-load';

const exporter = new ConsoleExporter();
const webTracer = new WebTracer({
  plugins: [
    new DocumentLoad()
  ]
});
opentelemetry.getTracer().addSpanProcessor(new SimpleSpanProcessor(exporter));
// or ?
opentelemetry.getTracer('webTracer').addSpanProcessor(new SimpleSpanProcessor(exporter));

opentelemetry.initGlobalTracer(webTracer);

instead of what I like much more and I think is more intuitive and natural for js

import { ConsoleExporter } from '@opentelemetry/console-exporter';
import { WebTracer } from '@opentelemetry/tracer-web';
import { DocumentLoad } from '@opentelemetry/plugin-document-load';

const webTracer = new WebTracer({
  exporter: new ConsoleExporter(),
  plugins: [
    new DocumentLoad()
  ]
});

ConsoleExporter - just a simple exporter to show spans in console nothing fancy so far

[bg451]

Okay I'm going to continue forward with the factory approach, but I agree with @obecny that the exporter should go into the configuration of the tracer.

[dyladan]

I don't see any reason that both can't work. That would be a question for specification though. I think @z1c0 is going to bring it up with them, but maybe we should make an issue to track these types of questions.

[z1c0]

The question where these configurations objects need to go, seems to be highly controversial. I think we need to be very clear in the specification on this, so I created an issue in the specification repository. We should continue this discussion there: open-telemetry/opentelemetry-specification#304

[mayurkale22]

Named Tracer is implemented via #582, closing this one. Will open a separate issue for Named Meter.