Remove Context interaction from Tracer/CorrelationContextManager

[carlosalberto]

From #424:

Should the Context interactions be part of the Tracer/CorrelationContextManager, or can we have a ContextUtil class in the each package that handles this interaction? This would mean removing members such as:

• Tracer.getCurrentSpan()
• CorrelationContextManager.getCurrentContext()

Now that Context is a well defined notion, probably we don't need an abstraction on top of it.

[dyladan]

I'm confused. Is this issue in favor of removing Tracer? What would be left on the tracer if span operations are removed?

[bogdandrutu]

On the Tracer we will still have the "Span" creation API which is important in order to be able to provide different implementations for the "Span".

[carlosalberto]

@dyladan Updated the description to make this clear, hope that helps ;)

[dyladan]

So the Tracer will no longer modify context, but may still read from the current context (in order to find parent for a span, etc). Does that mean startSpan is the only method left on the Tracer now that Propagation is removed?

[bogdandrutu]

@dyladan I would say most likely yes at this point. Similar with Meter where we have methods just to create the instruments.

[dyladan]

Be aware that this language will probably need to change if this proposal is accepted https://github.com/open-telemetry/opentelemetry-specification/pull/424/files#diff-ea4f4a46fe8755cf03f9fb3a6434cb0cR132 as it implies that the Tracer is accessing/setting context values

[carlosalberto]

@dyladan Oh definitely. This part will be updated if we proceed with this.

[toumorokoshi]

+1 on this idea, but are there thoughts on being stricter on things like tracer having span setting methods?

I like as much consistency as possible between APIs, today I'm able to run through an API I'm familiar with (Python), and quickly relate the concepts to other languages. If, for example, setting current span in the tracer becomes popular in one language, and is non-existent in the other language, people will have to remember that detail as a caveat.

I personally advocate for exclusively having context set methods in a context util object, and disencouraging setter methods elsewhere.

[toumorokoshi]

@carlosalberto is there any update on this? In Python, I think I'd like to move out the span getter methods outside of the Tracer itself, which I think requires this detail to be made explicit in the spec.

[tsloughter]

What is the benefit to being outside the Tracer?

Something has to know the context key used to store the current span/correlationcontext/etc and assigning that role to a utils module seems odd.

[tsloughter]

We are intending to start returning the span context and either the context or a context token from start_span in the Erlang/Elixir implementation.

The issue is the need by some libraries to return to a previous context while it was originally built for returning only to a previous span context as the active span.

{SpanCtx, Ctx} = ot_tracer:start_span("some-span"),
try
  // some-span is the active span here and will be the parent
  // of any span created in do_some_stuff() 
  do_some_stuff()
after
  ot_span:end_span(SpanCtx),
  ot_ctx:set_context(Ctx)
end.

This ensures that if there is bad code in whatever do_some_stuff() runs when we return to the code we control it ends the correct span (not the currently active span since that could have changed and never ended by bad code in do_some_stuff() and reset the entire context to what it was when some-span was created.

Within a context the active span contexts are a list that prepends the new span it is created and a call to ot_tracer:end_span() will end the span at the head and remove it from the list, making the next span the currently active span once again. But in the above code the user does not want this and instead wants to ensure they reset to a copy of the context at the time it created its span.

I think this has finally solved some of our issues, though I haven't opened the PR to move to this API yet.

What I'm unsure about is CorrelationContext. if do_some_stuff() sets some label in the CorrelationContext, like for user_id if its an HTTP request, this is then lost when ot_ctx:set_context(Ctx) is called.

Maybe it should be? I'm not sure.

I guess what I am rambling on towards is whether the API should

[Oberon00]

@tsloughter

Something has to know the context key used to store the current span/correlationcontext/etc and assigning that role to a utils module seems odd.

One could also say that it is odd that you have to create a Tracer just to retrieve the current span (one could also say that's good though since then we know which instrumentation library accesses the span).

EDIT: Also note that Propagators get a full Context, so they also need to retrieve the span from it, and it does seem odd for these to need a Tracer instance.

[toumorokoshi]

One could also say that it is odd that you have to create a Tracer just to retrieve the current span (one could also say that's good though since then we know which instrumentation library accesses the span).

This is (for me) the precise motivation with the separation. I think that the trace get / set methods need to be part of the module or package, but not part of the tracer object.

Python has already divorced trace get / set methods from the tracer, which has made the code simpler:

trace = opentelemetry.trace.get_current_trace()

vs

tracer = opentelemetry.trace.get_tracer(__name__)  # name isn't significant here, but you need the field.
trace = tracer.get_current_trace()

Erlang with it's actor model may not disambiguate between a module and a class instance, and at that point it wouldn't be as relevant.

We are intending to start returning the span context and either the context or a context token from start_span in the Erlang/Elixir implementation.

This is a good pattern. We don't do that directly in Python, but we do offer an API to manually set an active span and get the context token to pop afterward:

https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-api/src/opentelemetry/trace/propagation/__init__.py#L23

[toumorokoshi]

@bogdandrutu if it'll help I'm happy to take this one. I have a bit of context and investment (since I pushed Python to adopt this early).

EDIT: This is largely a discussion ticket, and the action isn't as simple as writing a PR. I started a tangential discussion around package-level span getter / setters in this issue, and I'm creating a new ticket to reduce the confusion (#924)

[Oberon00]

Actually, isn't this already done? AFAIK we only have optional convenience methods specified in the tracing API.

[tsloughter]

Erlang with it's actor model may not disambiguate between a module and a class instance, and at that point it wouldn't be as relevant.

Right. It is just a module and does not call into any process in order to look up the current context, it is just that the tracer module knows the key to use for lookup.

So equivalent to trace = opentelemetry.trace.get_current_trace().

I suppose it is more I objected to ContextUtils, so more a pedantic concern with naming :)

[Oberon00]

I think the key to the current span must not be in a generic ContextUtils class, since such a class should be one layer below and agnostic of concerns like tracing/metrics/baggage. E.g. in Java, this is handled in a class TracingContextUtils which is part of the trace API module.

[andrewhsu]

@bogdandrutu i had a look over this with @carlosalberto , it seems like this is possibly breaking API by removing methods in trace so would be good to get this change in before the freeze.

[tsloughter]

This is a good pattern. We don't do that directly in Python, but we do offer an API to manually set an active span and get the context token to pop afterward:

This may be getting off topic but I just realized when discussing how we are doing context with others that there is the matter of CorrelationContext. I'm thinking specifically about the use of start_inactive_span and passing it to a new process, or in other languages/runtimes I guess async tasks, to be set as active.

This works fine for spans but leaves CorrelationContext behind.

How are other languages handling this? I know I've seen others creating inactive spans to pass to new processes/tasks/coroutines but don't recall them doing anything about CorrelationContext. I'll dig up examples of what I mean in case this isn't clear.

[Oberon00]

I think this is currently a major unanswered question, see #867 (I think my #875 would solve it in a way)

[tsloughter]

Oh, good, I'm not the only one :)

[toumorokoshi]

Please check out #923, I think it addresses the major concern of this issue.

EDIT: although the PR is somewhat related, it doesn't resolve the discussion, as Context interactions are still performed by the Trace-related code.

[toumorokoshi]

Actually, isn't this already done? AFAIK we only have optional convenience methods specified in the tracing API.

Currently, the convenience methods are encouraged, not completely optional:

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

The Tracer SHOULD provide methods to:

- Get the currently active Span
- Mark a given Span as active

And I don't think there's a requirement to expose any get / set of active span elsewhere in the codebase. However, it is possible for a consumer to re-implement the behavior by replicating the Context interaction.

[toumorokoshi]

Although I previously wrote +1, I'm switching my vote to a -1 as this has an impact on usability (e.g. starting and activating the tracer would not be possible in the same call), and I think this can also lead to proliferating boilerplate, such as naming the specific key that the span populates in the context.

Also, I am moving my discussion around providing simpler active span get / set APIs to a new issue #924