RFC: Add an extension to Tracer interface for custom go context creation

[krnowak]

In the opentracing to opentelemetry bridge we need to have an access to the go context when creating the span (see https://github.com/open-telemetry/opentelemetry-go/blob/eed647d11f57d72d7cfa3e1d25721e21e4a0c833/api/trace/api.go#L29-L30)

This new interface adds the StartSpanWithContext function, so the vendor can implement it in their own way.

Still not sure if this is the final extension we may need for the bridge, but that's the starter.

cc: @jmacd

[yurishkuro]

In the opentracing to opentelemetry bridge we need to have an access to the go context when creating the span

Are you referring to an adapter that supports the OpenTracing API but internally is implemented with OpenTelemetry SDK? I think I understand the reason for this proposal (OTel SDK method requires Context as argument), but it doesn't seem like it would make any difference since the typical OpenTracing instrumentation is not going to use the new Ext1 API.

[yurishkuro]

If adapter returns OpenTracing Span that wraps OTel span, and OpenTracing traditional instrumentation then stores that span in Context, then it may still be compatible with future use. Maybe there needs to be a method in OTel that accepts parent span directly, instead of always going through Context (that method could be in a separate migration-specific interface).

[krnowak]

In the opentracing to opentelemetry bridge we need to have an access to the go context when creating the span

Are you referring to an adapter that supports the OpenTracing API but internally is implemented with OpenTelemetry SDK?

Yes. For now it's available as a PR in open-telemetry/opentelemetry-go#98

I think I understand the reason for this proposal (OTel SDK method requires Context as argument), but it doesn't seem like it would make any difference since the typical OpenTracing instrumentation is not going to use the new Ext1 API.

My impression was that while the root span might be created with the tracer.CreateSpan, then the children spans are created by StartSpanFromContext or StartSpanFromContextWithTracer, because it's usually the go context that's being passed down the call stack, not the span itself. No?

If adapter returns OpenTracing Span that wraps OTel span, and OpenTracing traditional instrumentation then stores that span in Context, then it may still be compatible with future use. Maybe there needs to be a method in OTel that accepts parent span directly, instead of always going through Context (that method could be in a separate migration-specific interface).

Do you mean something like following in OpenTelemetry?

type TracerMigration interface {
	Tracer

	StartWithSpan(parent Span, name string, o ...SpanOption) Span
}

There might be a weakness in it that OTel won't be able to store whatever it wants in the go context created (like current span) by opentracing.ContextWithSpan, because there is currently no hook in this function for the OTel to plug into. Not sure yet how critical is this, though.

[yurishkuro]

Yes , like that interface (only it doesn't need to embed Tracer).

Also, I'm not sure what the plans are, but storing more things in the context than the Span doesn't sound like a good idea.

[yurishkuro]

Re StartSpanFromContext helper functions - they are just that, helpers. I've seen plenty of instrumentation that doesn't use those. I personally tend to avoid them because they depend on global tracer, which is a bad and infectious pattern.

[jmacd]

I thought about it some more and think we can probably avoid any changes in opentracing to get the shim support we need for OTel. I think there are four cases, where the parent span equals OTr or OTel, and where the start API equals OTr or OTel. The OTr->OTr and OTel->OTel cases should be trivial.

For an OTel start with an OTr parent, we have the opentracing.SpanFromContext() call which returns a Span that we can case into the underlying OTel implementation.

For an OTr start with an OTel parent, we have to use opentracing.StartSpanFromContext() to store the current OTr span, but we should be able to handle this from the shim.

I haven't thought exhaustively through the cases, but this may be feasible.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@94e0bdd). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #220   +/-   ##
=========================================
  Coverage          ?   59.54%           
=========================================
  Files             ?       14           
  Lines             ?      833           
  Branches          ?        0           
=========================================
  Hits              ?      496           
  Misses            ?      297           
  Partials          ?       40

Impacted Files	Coverage Δ
gocontext.go	87.5% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94e0bdd...60fbc08. Read the comment docs.

[jmacd]

This extension API is used internally, not by end-user code. When upgrading to OTel, this OT dependency will be brought in automatically. We need not bump the minor version number in the next release, even, but this code (or something similar) is necessary to support the OTel bridge.