bridge/opentracing: introduce NewTracerProvider that wraps TracerProvider instead of Tracer

[bobheadxi]

Currently, using the OpenTracing bridge tracer pairs, e.g. NewTracerPair, means that all tracing is named with the tracer initially provided to WrappedTracerProvider - calls to Trace() with names and options always returns the same tracer.

This change creates a new opentracing.TracerProvider to allow it to accept trace.TracerProviders and create named/configured tracers dynamically with bridging configured, and adds a new constructor, opentracing.NewTracerProvider, that accepts trace.TracerProvider. It also marks the old static constructor NewWrappedTracerProvider as deprecated.

There is an example usage here: https://github.com/sourcegraph/sourcegraph/pull/40945 . See discussion on this PR for more details.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: bobheadxi / name: Robert Lin (0ffe14b)

[codecov]

Codecov Report

Merging #3116 (b421d60) into main (89cb6a4) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #3116   +/-   ##
=====================================
  Coverage   78.9%   78.9%           
=====================================
  Files        169     170    +1     
  Lines      12437   12460   +23     
=====================================
+ Hits        9813    9836   +23     
  Misses      2415    2415           
  Partials     209     209           

Impacted Files	Coverage Δ
bridge/opentracing/wrapper.go	100.0% <ø> (ø)
bridge/opentracing/provider.go	100.0% <100.0%> (ø)

[dmathieu]

While this makes sense, it creates a new wrapped tracer every time Tracer() is called, which may cause performance issues.

[bobheadxi]

While this makes sense, it creates a new wrapped tracer every time Tracer() is called, which may cause performance issues.

The wrapped tracer is a very lightweight struct, and the creation of the underlying Tracer has some caching of created Tracers already: https://sourcegraph.com/github.com/open-telemetry/opentelemetry-go/-/blob/internal/global/trace.go?L83-110

However @dmathieu I can implement a similar approach here when creating wrapped tracers if that would make this change acceptable!

[bobheadxi]

@dmathieu I've added a caching layer and testing in 8a83e1f

[dmathieu]

Thank you. I'm personally not super fond of the name NewDynamicWrappedTracerProvider, but I'm not finding any.
I wonder if we should deprecate NewWrappedTracerProvider as well, to avoid keeping both methods around.

[bobheadxi]

Thank you. I'm personally not super fond of the name NewDynamicWrappedTracerProvider,

What do you think about the name NewBridgedTracerProvider? It indicates that it provides tracers that bridges requests to OpenTracing as well, which is a bit clearer IMO than just saying the provider is wrapped.

Alternatively, maybe a simple NewTracerProvider will suffice? We are exporting it from a bridge package after all - (bridge/opentracing).NewTracerProvider(...) (otel/trace.TracerProvider) reads reasonably well I think

I wonder if we should deprecate NewWrappedTracerProvider as well, to avoid keeping both methods around.

I can add a deprecation godoc to the other constructor!

[dmathieu]

You're right, NewTracerProvider may be good.
Let's wait for opinions from other approvers/maintainers about all this.

[bobheadxi]

59f2c6b renames the constructor and adds a deprecation notice to NewWrappedTracerProvider.

It might be valuable to provide new variants of NewTracerPair and NewTracerPairWithContext as well and deprecate the existing utility constructors, but the naming gets clunky, for example:

• NewTracerPairWithProvider(trace.TracerProvider)
• NewTracerPairWithProviderAndContext(context.Context, trace.TracerProvider)

We could also introduce new naming like:

• NewBridgeTracers(trace.TracerProvider) (or simply NewBridge(trace.TracerProvider))
• NewBridgeTracersWithContext(context.Context, trace.TracerProvider) (or simply NewBridgeWithContext(context.Context, trace.TracerProvider))

Alternatively, we could also deprecate the utilities entirely and rely on users to replicate the setup themselves.

Let me know what you think!

[bobheadxi]

hey @dmathieu, any chance this PR might get another round of reviews? The lack of appropriate Tracer naming in emitted traces is making it difficult at times to narrow down where trace spans are coming from.

[bobheadxi]

@dmathieu @MrAlias - I've addressed the review comments, could one you please take another look when you get the chance? Thank you!

[bobheadxi]

Thanks for the review @dmathieu - addressed your last comment, and all checks seem to be green :) (I'm not authorized to merge, so would appreciate some help with that 🙏 )