Moving active span interaction To Trace Context Utilities

[toumorokoshi]

Fixes #924

Motivation

The Trace API currently

Changes

The previous spec stated that the active span get / set behavior MUST
behave the same across Tracers retrieved from the same TracerProvider.
As such, the active span manipulation methods are better provided by the
TracerProvider directly.

As it is still cumbersome to retrieve the current TracerProvider simply
to get the current span, add in the concept of a "Trace Package", that can
provide utilty methods to delegate to the configured default TracerProvider.
This enables a fluid interface to retrieve the current span:

from opentelemetry import trace

current_span = trace.get_current_span()

To help maintain backwards compatibility, the Tracer may still expose
active span methods.

[yurishkuro]

I don't understand the motivation for this. Span interactions, including active span, are best done via Tracer, it minimizes the number of objects that instrumentation has to deal with. How that's done by the tracer is an implementation detail, the only requirement is for active span accessors to act consistently.

And it does not fix #455, where the objective was to achieve the most normalized (albeit more verbose) API, which are maximally decoupled (i.e. Tracer does not need to know anything about Context - see opentracing.Tracer, for example).

[toumorokoshi]

@yurishkuro regarding #455: yes, it's a good point that the original point of the issue isn't solved. I've added my rationale and moved the references to a new issue #924 that describes the desire in more detail.

Regarding who should perform span interactions: creating a Span currently requires a Tracer to ensure named tracers, so that certainly can't be moved.

But there is a common use case of retrieving the active span, for the purposes of adding or modifying it in some fashion. The API as written implies that the active span should be the same regardless of which Tracer retrieves it. As such, retrieving the span from a tracer has a few cons, from my perspective:

• allocating an otherwise unused Tracer if the use case is just modifying the active span
• unclear to a user how the tracer name impacts that calculation for the active span (unless you have a strong understanding of how tracing works).

APIs are subjective, but adding get_current_span to the module in Python had a quorum that this was a change that improved the user experience significantly.

[carlosalberto]

@toumorokoshi Thanks for the PR. As @Oberon00 mentioned, this is better separated from Tracer, but it doesn't need to be in TracerProvider neither. As any Tracer / TracerProvider can consume whatever is in the current Context, I'd put it in its separate class.

Your example would stay the same (i.e. trace.current_span()) as the active Span would depend on the top level package for tracing (opentelemetry.trace) which is fine ;)

To help maintain backwards compatibility, the Tracer may still expose
active span methods.

In OT we had these as shortcuts, but I heard some users got confused by it. Something to ponder.

[toumorokoshi]

@toumorokoshi Thanks for the PR. As @Oberon00 mentioned, this is better separated from Tracer, but it doesn't need to be in TracerProvider neither. As any Tracer / TracerProvider can consume whatever is in the current Context, I'd put it in its separate class.

Ok! That sounds like another opinion supporting #923 (comment), so I'll get that PR up as well.

In OT we had these as shortcuts, but I heard some users got confused by it. Something to ponder.

@carlosalberto would you mind elaborating? I think we still need to leave those methods for backwards compatibility, but would be interested in more data to inform the final design.

[toumorokoshi]

@Oberon00 @carlosalberto addressed your concerns, PTAL.

[carlosalberto]

would you mind elaborating?

So in OT we had the context functionality in a ScopeManager class, which would expose active() and activateSpan() methods; Tracer in turn would have an activeSpan() method that was simply a shortcut to the respective ScopeManager method.

But to simplify things, lets consider removing such Tracer calls in a follow up, to not delay this PR ;)

[carlosalberto]

Thanks, looks great! @open-telemetry/specs-approvers Please review.

[Oberon00]

@yurishkuro

Span interactions, including active span, are best done via Tracer, it minimizes the number of objects that instrumentation has to deal with. How that's done by the tracer is an implementation detail,

That might be a good design too, but in OpenTelemetry, the Tracer MUST use the Context system, that is even specified in the API spec.

The Tracer MUST delegate to the Context to perform these tasks, i.e. the above methods MUST do the same as a single equivalent method of the Context management system.

-- https://github.com/open-telemetry/opentelemetry-specification/blob/a0828f6223f7b25aaba567179575c48e942bcead/specification/trace/api.md#tracer-operations

[bogdandrutu]

Thanks for taking care of this.

[carlosalberto]

Ping @yurishkuro - all feedback has addressed, I think, but would love to have our final review (take?) on this before we actually merge it ;)

[bogdandrutu]

@toumorokoshi please rebase

[arminru]

@bogdandrutu I updated the branch (just a conflict in the changelog) but we're still waiting for @yurishkuro to approve (or dismiss his change request).