Move context utils outside of the Tracer #527

bogdandrutu · 2020-03-24T17:24:12Z

Rational:

Context is now a standalone concept in OpenTelemetry and we don't need to provide abstractions on top of context interactions, so no need to have getCurrentSpan an interface.
Remove duplicate APIs in some languages like Java where there is an util class to allow context interactions and also the methods on the tracer.
Simplify context interaction by not requiring to pass a Tracer to classes/functions where no need to create a new Span.

Fixes: #455

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

specification/api-tracing.md

yurishkuro · 2020-03-24T17:35:41Z

specification/api-tracing.md

+The API MUST provide methods to (depending on the language, global functions
+or util class e.g. `TracingContextUtils`):


Suggested change

The API MUST provide methods to (depending on the language, global functions

or util class e.g. `TracingContextUtils`):

The API MUST provide methods (or, depending on the language, global functions

or util class e.g. `TracingContextUtils`) to:

also s/TracingContextUtils/TracingContext/

I think we need something as suffix or prefix if we want to be consistent for CorrelationContext which has Context at the end, because will be ugly to name that CorrelationContextContext :(. Any ideas?

If context interaction is done via static util class, then TracingContext.activeSpan() looks a lot better than TracingContextUtils.activeSpan().

I do agree with you but I am trying to think how would I name this class for "CorrelationContext" package. If I find a good name for that and ensure consistency I am all to remove Utils.

CorrelationContext can remain CorrelationContext. It can represent both the object that stores correlations and the namespace for static functions.

@yurishkuro I'd be up for not using TraceContext and keeping the activation bits in CorrelationContext - we will end up with too many classes having the Context suffix (some of them doing different things, i.e. TraceContext doing activation handling only, CorrelationContext doing both activation handling PLUS as a container of key-values).

There was already some confusion in the past because of the fact we had both CorrelationContext and Context, so this could get worse ;)

specification/api-tracing.md

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

specification/api-tracing.md

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Oberon00 · 2020-03-24T18:48:30Z

specification/api-tracing.md

+The OpenTelemetry library achieves in-process context propagation of `Span`s by
+way of the [Context](./context.md).
+
+An `active Span` referes to a Span that is set to an


That means, for other languages (e.g. Go) all Spans are inactive?

that means there is no concept of active span. The current span is defined as the Span in the context passed to the function so no need to getCurrentSpan just getSpanFromContext.

I would suggest:

having a clear definition of "current" in the context.md, which can be defined both for implicit prop and for explicit prop (closest context in the lexical scope)

converge on a single term for spans, either "active" or "current", not both, and define it as "span stored in the current context"

@yurishkuro I agree on keeping either current or active, not both terms. However, I'm wondering if this should be done in a follow up PR.

@carlosalberto I agree but this PR adds wording using both terms, at least within the PR it should be consistent.

This PR does not add any new reference to either active or current, both were in the previous text.

carlosalberto · 2020-03-24T19:53:31Z

@bogdandrutu Overall LGTM. @yurishkuro raises a few interesting points (on which all I agree), so I'd be up for merging this one after the issues have been resolved (a few of them can be done in a follow up too, as already mentioned).

tsloughter · 2020-03-24T20:06:47Z

specification/api-tracing.md

+* Get/Set a Span from/to a Context
+* Make a given Span as active
+
+The API MUST internally leverage the Context in order to get and set the current


This sounds like logic is going into the API? Before the API was almost entirely interfaces.

Unless I'm misunderstanding this it seems instead of the Tracer simply calling to the sdk with a context and some options to start a span there is log in the API for juggling the span in the context, resulting in multiple calls from the API to the SDK?

How does this work out for the streaming SDK for example?

It was my understanding that the API should never call the SDK and cannot depend on it. Otherwise it would be impossible to have third party SDKs

The API is functions/methods that are implemented by the SDK, calling API functions results in whatever SDK is being used (if any) being called. Users do not call the SDK directly.

@tsloughter As far as I'm aware, Context is not a part of the SDK but a third top-level item that the API and SDK depend on.

jmacd · 2020-03-25T01:49:02Z

There used to be specification language about the Span having a method to get the Tracer that created it. I am opposed to this functionality, but I can no longer find any specification for it. In Go we have this:

https://github.com/open-telemetry/opentelemetry-go/blob/4c69dd3234c6b71d030ecd1b523644cd3ccde0a2/api/trace/api.go#L87

Can we remove that now

In my work on Resource Scope, open-telemetry/oteps#78, that method caused a headache. If something similar must be done, I would argue that Spans should be able to provide you with their parent's Context, not their Tracer.

tsloughter · 2020-03-25T02:21:35Z

@jmacd if the span has no method to get the Tracer then how does it run processors when it ends?

The same problem I had that lead me to open #516

jmacd · 2020-03-25T02:32:36Z

@tsloughter I am assuming that the Span has a reference to the Tracer that created it, but that it does not expose this to the user. I believe that if a user wants to get at the Tracer that created a Span, the user should retain a reference to the Context that was used to start the Span.

specification/api-tracing.md

arminru · 2020-03-25T17:58:16Z

specification/api-tracing.md

+The OpenTelemetry library achieves in-process context propagation of `Span`s by
+way of the [Context](./context.md).
+
+An `active Span` referes to a Span that is set to an


"a span that is set to an attached context" sounds unclear to me

specification/api-tracing.md

arminru · 2020-03-25T18:01:30Z

specification/api-tracing.md

+The OpenTelemetry library achieves in-process context propagation of `Span`s by
+way of the [Context](./context.md).
+
+An `active Span` referes to a Span that is set to an


@carlosalberto I agree but this PR adds wording using both terms, at least within the PR it should be consistent.

specification/api-tracing.md

Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>

bogdandrutu · 2020-03-27T15:59:40Z

@jmacd @tsloughter This PR does not changes anything related to Span having or not having a method to return the Tracer that created it.

tsloughter · 2020-03-31T15:44:04Z

From the spec meeting I thought I'd show how we do propagation in the tracer in Erlang.

-spec w3c_propagators() -> {ot_propagation:http_extractor(), ot_propagation:http_injector()}.
w3c_propagators() ->
    ToText = fun ot_propagation_http_w3c:inject/2,
    FromText = fun ot_propagation_http_w3c:extract/2,
    Injector = ot_ctx:http_injector(?TRACER_KEY, ?TRACER_CTX, ToText),
    Extractor = ot_ctx:http_extractor(?TRACER_KEY, ?EXTERNAL_SPAN_CTX, FromText),
    {Extractor, Injector}.

Applications can then setup a propagation in the SDK for the tracer of the SDK:

{CorrelationExtractor, CorrelationInjector} = ot_correlations:get_http_propagators(),
{TracerExtractor, TracerInjector} = ot_tracer_default:w3c_propagators(),

opentelemetry:set_http_extractor([TracerExtractor, CorrelationExtractor]),
opentelemetry:set_http_injector([TracerInjector, CorrelationInjector])

But now I see maybe your issue is that I'm using the SDK tracer directly, ot_tracer_default? Which I think makes sense, it is what knows the keys for tracer context.

Oberon00 · 2020-07-07T09:13:01Z

I think this is partially obsolete after #619.

Move context utils outside of the Tracer

9442fa3

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

bogdandrutu requested review from arminru, c24t, carlosalberto, iredelmeier, jmacd, reyang, SergeyKanzhelev, tedsuo, tigrannajaryan and yurishkuro as code owners March 24, 2020 17:24

yurishkuro reviewed Mar 24, 2020

View reviewed changes

specification/api-tracing.md Show resolved Hide resolved

yurishkuro reviewed Mar 24, 2020

View reviewed changes

specification/api-tracing.md Outdated Show resolved Hide resolved

bogdandrutu added 2 commits March 24, 2020 10:51

Apply feedback.

f29a958

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

Apply more feedback.

7248740

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

yurishkuro reviewed Mar 24, 2020

View reviewed changes

specification/api-tracing.md Outdated Show resolved Hide resolved

yurishkuro reviewed Mar 24, 2020

View reviewed changes

specification/api-tracing.md Show resolved Hide resolved

Update specification/api-tracing.md

25365ce

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Oberon00 reviewed Mar 24, 2020

View reviewed changes

carlosalberto approved these changes Mar 24, 2020

View reviewed changes

tsloughter reviewed Mar 24, 2020

View reviewed changes

arminru reviewed Mar 25, 2020

View reviewed changes

bogdandrutu and others added 3 commits March 27, 2020 08:15

Update specification/api-tracing.md

b61b7fd

Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>

Update specification/api-tracing.md

93d9e14

Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>

Update specification/api-tracing.md

33649f0

Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>

bogdandrutu and others added 2 commits March 27, 2020 08:55

Update specification/api-tracing.md

d9a767d

Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>

Update api-tracing.md

57774d4

Oberon00 mentioned this pull request Mar 31, 2020

Use Context to stop tracing #530

Open

carlosalberto mentioned this pull request Apr 7, 2020

distinguish starting an active span and creating an inactive span #485

Closed

carlosalberto added spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory labels Jun 12, 2020

carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 2, 2020

bogdandrutu closed this Jul 20, 2020

bogdandrutu deleted the tracercontext branch July 20, 2020 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move context utils outside of the Tracer #527

Move context utils outside of the Tracer #527

bogdandrutu commented Mar 24, 2020 •

edited

Loading

yurishkuro Mar 24, 2020

yurishkuro Mar 24, 2020

bogdandrutu Mar 24, 2020

yurishkuro Mar 24, 2020

bogdandrutu Mar 24, 2020

yurishkuro Mar 24, 2020

carlosalberto Mar 24, 2020

Oberon00 Mar 24, 2020

bogdandrutu Mar 24, 2020

yurishkuro Mar 24, 2020 •

edited

Loading

carlosalberto Mar 24, 2020

arminru Mar 25, 2020

bogdandrutu Mar 27, 2020

carlosalberto commented Mar 24, 2020

tsloughter Mar 24, 2020

dyladan Mar 27, 2020

tsloughter Mar 27, 2020

dyladan Mar 27, 2020

jmacd commented Mar 25, 2020

tsloughter commented Mar 25, 2020

jmacd commented Mar 25, 2020

arminru Mar 25, 2020

arminru Mar 25, 2020

bogdandrutu commented Mar 27, 2020

tsloughter commented Mar 31, 2020

Oberon00 commented Jul 7, 2020

		The API MUST provide methods to (depending on the language, global functions
		or util class e.g. `TracingContextUtils`):

Move context utils outside of the Tracer #527

Move context utils outside of the Tracer #527

Conversation

bogdandrutu commented Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmacd commented Mar 25, 2020

tsloughter commented Mar 25, 2020

jmacd commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Mar 27, 2020

tsloughter commented Mar 31, 2020

Oberon00 commented Jul 7, 2020

bogdandrutu commented Mar 24, 2020 •

edited

Loading

yurishkuro Mar 24, 2020 •

edited

Loading