Named Tracers / Tracer Registry

[dyladan]

Initial spike of spec/#354

Do not merge until the spec is finalized.

Which problem is this PR solving?

• Implements Implement Named Tracers #403
• Replaces feature(trace): adds named tracer factory #420

Open questions from spec

• Is name required or optional?
• What happens if you try to get the same tracer twice with the same name/version but a different config
  • Currently, we do not overwrite
• Are all configs managed by tracer or by tracer registry?
  • per-tracer configs may be desirable by some but the same can be achieved by creating a second tracer registry if required

Short description of the changes

There is now only 1 type of Tracer. BasicTracer/WebTracer/NodeTracer are gone. The platform specific logic is moved into their respective registries.

BasicTracerRegistry

This is the base class for web and node tracer registries. It maintains span processors and all created tracers. Calling getTracer(name, version) will get the tracer with the given name and version, creating it if required. The default name is the empty string. The default version is '*'

• Calling getTracer('', '*') is equivalent to getTracer()
• Calling getTracer('mysql', '*') is equivalent to getTracer('mysql')
• Calling getTracer with the same inputs twice will return the same tracer
• The 'wildcard' version of a tracer with a given name is a new tracer. It will not get a previously created versioned tracer. Calling getTracer('mysql') will return the same tracer as getTracer('mysql', '*') but NOT the same tracer as getTracer('mysql', '0.2.0')

NodeTracerRegistry

• Inherits from BasicTracerRegistry
• Loads plugins
• Uses AsyncHooksScopeManager by default
• Loads default plugins if not configured

WebTracerRegistry

• Inherits from BasicTracerRegistry
• Uses StackScopeManager by default

Plugins

Plugins are now expected to call super with a name and version. This is the name and version of the telemetry itself, NOT the name and version of the module being instrumented. For example, the mysql plugin calls super('opentelemetry.mysql', '0.2.0'). BasePlugin.enable now expects a TracerRegistry where it previously expected a Tracer. When a span is created by a tracer, it injects itself the same way it previously did, but delegates calls to getActiveSpanProcessor to the TracerRegistry that created it. In this way, span processors that are registered with a TracerRegistry after a plugin has been enabled are still called by new spans.

Core

initGlobalTracer has been replaced by initGlobalTracerRegistry. getGlobalTracerRegistry now exists to access the global tracer registry. getTracer still exists and returns a tracer from the current global tracer registry.

Tracer Options

The tracer registry is initialized with the same options that tracers were previously initialized with. It is possible to get a tracer with different options by passing an options object as a third argument to getTracer (const tracerWithLogger = registry.getTracer('mysql', '0.2.0', { logger });). The global getTracer function does not have a third tracer options parameter and you will need to call registry.getTracer if you want that functionality.

[codecov-io]

Codecov Report

Merging #582 into master will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
+ Coverage   89.77%   89.85%   +0.08%     
==========================================
  Files         215      214       -1     
  Lines       10137    10183      +46     
  Branches      936      933       -3     
==========================================
+ Hits         9100     9150      +50     
+ Misses       1037     1033       -4

Impacted Files	Coverage Δ
...ages/opentelemetry-plugin-http/test/utils/utils.ts	33.33% <0%> (-26.67%)	⬇️
packages/opentelemetry-plugin-mysql/src/utils.ts	90.9% <0%> (-4.55%)	⬇️
packages/opentelemetry-plugin-dns/src/version.ts	100% <0%> (ø)	⬆️
packages/opentelemetry-tracing/src/config.ts	100% <0%> (ø)	⬆️
packages/opentelemetry-tracing/src/utility.ts	100% <0%> (ø)	⬆️
packages/opentelemetry-plugin-redis/src/version.ts	100% <0%> (ø)	⬆️
...telemetry-plugin-grpc/test/utils/assertionUtils.ts	100% <0%> (ø)	⬆️
packages/opentelemetry-tracing/test/Span.test.ts	100% <0%> (ø)	⬆️
.../opentelemetry-plugin-dns/test/utils/assertSpan.ts	100% <0%> (ø)	⬆️
...ry-tracing/test/export/SimpleSpanProcessor.test.ts	100% <0%> (ø)	⬆️
... and 17 more

[bg451]

All of the comments I made during our essentially in person review were addressed, so ya went through this again and left a few comments but this LGTM. There's definitely a question of what to do with the tracer name and version but that's a separate issue.

[dyladan]

Updated to be up-to-date with current spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#obtaining-a-tracer where name is required. When/if #345 merges we can revisit. It is easier to make name optional later than it is to make it required later.

I'm removing the draft tag from this so we can begin the formal review process but other SIGs have already merged their named tracer implementations.

[mayurkale22]

#651 is merged now, could you please make this PR ready for review. Thanks :)

[dyladan]

#651 is merged now, could you please make this PR ready for review. Thanks :)

Should be good to review now

[dyladan]

lint & docs is failing because david.dm is down again. It is not an actual failure.

[mayurkale22]

lgtm, great work!

[mayurkale22]

Please, resolve the conflicts. @open-telemetry/javascript-approvers This is relatively big and important change, we need more reviews for this.

[dyladan]

Please, resolve the conflicts.

@mayurkale22 done

[obecny]

There is one thing which confuses me a lot
.getTracer('default’)
is the name required or can I use
.getTracer()
Do I always have to provide the default name and if that is the case should the registry has the static value then. Or if I don’t need to provide the name then I would rather remove default from all examples. If the default is required I would still remove it and make it default name when nothing is provided.
Other than that looks fine

[obecny]

is this intentional ?

[mayurkale22]

If the default is required I would still remove it and make it default name when nothing is provided.

+1 I agreed. As per the specs: In case an invalid name (null or empty string) is specified, a working default Tracer implementation as a fallback is returned. With this, we can remove default from all the examples - much simpler and easier to understand.

[mayurkale22]

As @dyladan is on vacation for a week, I was thinking to merge this one and handle @obecny's recommendation/suggestion (#582 (review)) in the separate PR. @obecny WDYT?

[obecny]

@mayurkale22 fine for me