More user-friendly Context util classes

[trask]

Throwing this out for discussion.

I think it has some interesting benefits:

• Removes the confusing (to me) util classes:
  • TracingContextUtils
  • CorrelationsContextUtils
  • ContextUtils
• Makes it possible to isolate the part of the API that deals with thread-bound contexts, as the methods below can be removed (with the given replacement). This is a nice separation for users who are absolutely opposed to thread-bound contexts, or want to use a different underlying thread-bound context.
  • Tracer.getCurrentSpan() → CurrentContext.getSpan()
  • Tracer.withSpan() → CurrentContext.withSpan()
  • CorrelationContextManager.getCurrentContext() → CurrentSpan.getCorrelationContext()
  • CorrelationContextManager.withContext() → CurrentContext.withCorrelationContext()
• Context is an interface (or maybe an abstract class, see TODO notes), so users (or contrib modules) can provide other implementations that store the state somewhere else, e.g. gRPC Context, Reactor Context, etc

[yurishkuro]

This breaks the layering principle to me, as well as creates a circular dependency between context layer and higher telemetry layers.

[carlosalberto]

Concur with this. Also, the idea is that when adding new concerns/layers, they all have their own Context related handling code (such as TracingContextUtils, which we can rename to something better, btw).

[trask]

It seems to create a more user-friendly API if we invert the dependency, and make the context depend on the "stuff", instead of making the "stuff" depend on the context:

E.g. here is some code in master:

    Context context =
        TracingContextUtils.withSpan(
            DefaultSpan.create(contextShim.getSpanContext()), Context.current());
    context =
        CorrelationsContextUtils.withCorrelationContext(
            contextShim.getCorrelationContext(), context);

and this is what is looks like in this PR:

    Context context =
        CurrentContext.get()
            .withSpan(DefaultSpan.create(contextShim.getSpanContext()))
            .withCorrelationContext(contextShim.getCorrelationContext());

[Oberon00]

One could add additional an "user friendly context" layer that depends on both the low level context and "stuff" to provide a more friendly API, if this is deemed important enough.

[carlosalberto]

Context is an interface (or maybe an abstract class, see TODO notes), so users (or contrib modules) can provide other implementations that store the state somewhere else, e.g. gRPC Context, Reactor Context, etc

We already had this in the past, but decided not to go on that route, to avoid double wrapping of Context and related objects. @bogdandrutu might have more information on that.

[carlosalberto]

Overall I'm a little bit confused. I thought the effort to isolate grpc.Context into its own thing was to avoid having intermediate layers. Or maybe would this be an alternative, in case we don't manage to isolate grpc.Context?

Also, one of the goals of the current Context API as defined in the Specification, is to allow users to define their own Propagators and put their extra concerns in Context as they please. In that regard, 'sealing' our Context abstraction to only expose Span and CorrelationContext kind of breaks that concept.

[trask]

I recommend to hold off on further review, I'm making some significant changes to address initial feedback above and from @bogdandrutu, I'll post back when ready to re-review.

[trask]

Ok, I'm back to being confused. From a user-friendly API perspective I like this PR. But I also totally don't understand all of the implication and desires around context propagation, so keep shooting holes in it 😄.

[trask]

Just to continue this experiment, I removed the Tracer and CorrelationContextManager methods that rely on the thread-bound context.

This makes it easier to see whether we can invert the dependency (e.g. making the context depend on "stuff") without creating a circular dependency.

The results show two notable exceptions:

• Span.Builder relies on CurrentContext.getSpan()
• CorrelationContext.Builder relies on CurrentContext.getCorrelationContext()

[yurishkuro]

Context is merely a container for key-value pairs. It cannot know about things like Span.

[trask]

I converted Context to key-value pairs only.

[codecov-io]

Codecov Report

Merging #1189 into master will increase coverage by 0.02%.
The diff coverage is 97.93%.

@@             Coverage Diff              @@
##             master    #1189      +/-   ##
============================================
+ Coverage     85.08%   85.10%   +0.02%     
+ Complexity     1174     1160      -14     
============================================
  Files           149      148       -1     
  Lines          4378     4384       +6     
  Branches        405      403       -2     
============================================
+ Hits           3725     3731       +6     
  Misses          494      494              
  Partials        159      159              

Impacted Files	Coverage Δ	Complexity Δ
...ationcontext/DefaultCorrelationContextManager.java	50.00% <ø> (-6.25%)	4.00 <0.00> (-2.00)
...text/DefaultCorrelationContextManagerProvider.java	100.00% <ø> (ø)	3.00 <0.00> (ø)
.../main/java/io/opentelemetry/trace/DefaultSpan.java	80.00% <ø> (ø)	19.00 <0.00> (ø)
...ntelemetry/exporters/inmemory/InMemoryTracing.java	100.00% <ø> (ø)	2.00 <0.00> (ø)
...ontrib/trace/propagation/B3PropagatorInjector.java	93.54% <50.00%> (ø)	6.00 <0.00> (ø)
...ry/contrib/trace/propagation/JaegerPropagator.java	87.50% <66.66%> (ø)	20.00 <0.00> (ø)
.../src/main/java/io/opentelemetry/OpenTelemetry.java	93.10% <100.00%> (+0.24%)	21.00 <1.00> (+1.00)
...lemetry/correlationcontext/CorrelationContext.java	100.00% <100.00%> (ø)	0.00 <0.00> (?)
...va/io/opentelemetry/scope/DefaultScopeManager.java	100.00% <100.00%> (ø)	7.00 <7.00> (?)
...a/io/opentelemetry/trace/DefaultTraceProvider.java	100.00% <100.00%> (ø)	3.00 <1.00> (ø)
... and 23 more

---

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 c930d13...ae5fd38. Read the comment docs.

[trask]

I added back gRPC Context, because this PR has evolved mostly into changes that I think make the API around Context usage nicer, and are worth considering independent of the gRPC Context question.

New summary:

CurrentContext API (with 5 methods) is single place users need to go for working with thread-bound contexts, replacing:

• TracingContextUtils
• CorrelationsContextUtils
• ContextUtils
• Tracer.getCurrentSpan()
• Tracer.withSpan()
• CorrelationContextManager.getCurrentContext()
• CorrelationContextManager.withCurrentContext()

For users that are not working with thread-bound Contexts, the only API they need is Span.KEY and CorrelationContext.KEY, which define the keys for adding and retrieving Spans and CorrelationContexts from the Context.

I saw the note in the specification that says "it is recommended that concerns mediate data access via an API, rather than provide direct public access to their keys", but putting the keys in the interfaces makes them more discoverable/rememberable for users compared to mediating access via a Util class, and it makes user's code more readable, so I think it's at least worth considering, e.g.

Using util classes in master:

Context context =
    TracingContextUtils.withSpan(
        DefaultSpan.create(contextShim.getSpanContext()), Context.current());
context =
    CorrelationsContextUtils.withCorrelationContext(
        contextShim.getCorrelationContext(), context);

Using keys in PR:

Context context =
    Context.current()
        .withValue(Span.KEY, DefaultSpan.create(contextShim.getSpanContext()))
        .withValue(CorrelationContext.KEY, contextShim.getCorrelationContext());

[bogdandrutu]

@trask if we expose the Key then we will never be able to use a different Key and you may ask when would we need to do that:

1. If for example we do a v2.Span interface the we cannot share the same key, and in that case you will need a different Key for v2.Span - so far nothing unexpected or impossible.
2. If we try now to build a bridge between v1.Span and v2.Span we have trouble because there is now place where we can "convert" from v2 to v1 when user interacts with the Context. We had this problem in OpenCensus when we started thinking to redirect all the calls to OpenTelemetry and we had to hide to Key so that when user does getCurrentSpan we actually do a getCurrentV2Span and return a wrapper v1.Span on top of the v2.Span.

You may argue that we never need to do this :), but I wouldn't design an API with this potential problem.

[trask]

Ah, that helps to understand the use case for that 😄.

Ok, so I've re-hidden the context keys. This part of the context API is now essentially back to what it is in master (static util class), but with nicer(?) naming.

Here's the same example as above from io.opentelemetry.opentracingshim.Propagation:

Using util classes in master:

Context context =
    TracingContextUtils.withSpan(
        DefaultSpan.create(contextShim.getSpanContext()), Context.current());
context =
    CorrelationsContextUtils.withCorrelationContext(
        contextShim.getCorrelationContext(), context);

Using nicer(?) named util classes in PR:

Context context =
    Span.Key.put(DefaultSpan.create(contextShim.getSpanContext()), Context.current());
context = CorrelationContext.Key.put(contextShim.getCorrelationContext(), context);

[carlosalberto]

Span.Builder relies on CurrentContext.getSpan()

Actually I have locally changes to also have SpanBuilder.setParent(Context ctx) (which were removed from the BIG changes as part of OTEP 66, to not include too many things). The plan was/is to remove the dust and create a PR based on them (this would also include CorrelationContext.setParent(Context ctx) btw).

For users that are not working with thread-bound Contexts, the only API they need is Span.KEY and CorrelationContext.KEY, which define the keys for adding and retrieving Spans and CorrelationContexts from the Context.

The specification explicitly tries to prevent cross-cutting concerns to expose the keys directly, so we keep this as an implementation detail. Also, we would need to potentially add SpanContext.KEY as it might be stored there as well.

[carlosalberto]

This makes me remember some idea we had back in OT about having a low level API (the current API/SDK) and a higher level one (as an extra artifact), which would make things simpler for end users. This would map to something like:

try (Scope scope = OpenTelemetry.createSpan("hello")) {
  OpenTelemetry.propagate(carrier); // Use the current Context
  OpenTelemetry.setSpanAttribute("mykey", "myvalue"); // Use the current Span

  // Or access objects directly.
  log.info(OpenTelemetry.getCurrentSpan());
  log.info(OpenTelemetry.getCurrentCorrelationContext());
}

The low-level API would exist for actual instrumentation and advanced scenarios that need fine-tuned code. Otherwise, general users would be advised to use the high level one.

Something to consider (and prototype eventually) ;)

[codefromthecrypt]

maybe you can add a RATIONALE doc for why this appears to require classloader based configuration (statics)? Otherwise let me know what I'm missing

[trask]

@adriancole both thread-bound context usage (statics) and non- thread-bound context usage (non-statics) is supported. I'm not sure if that answers your question?

[codefromthecrypt]

when the current context impl is accessed statically, this implies it is configured statically.

ex.

try (Scope scope = CurrentContext.withSpan(span)) {

vs a field or similar

try (Scope scope = currentContext.withSpan(span)) {

This is the part to focus on. Whether what's configured internally uses an instance bound or static bound thread local or none at all is separate topic.

[codefromthecrypt]

particularly when most (all?) code uses static access, this hints at optimizing for this, which is a problem for configuration. Even if non-static is permitted, if in practice everything is written to use statics, and doesn't have a way to override this, it limits applicability.

imho code examples like

try (Scope scope = CurrentContext.withSpan(span)) {

should never be written as they put people into corners that at best rely on static registries.

Alternatively, a field is better. In the edge case you want to use static, you can default the ctor to it.

try (Scope scope = currentContext.withSpan(span)) {

If you believe that static access is better, then basically please put that in a RATIONALE as when this causes problems people can go to one place for the answer.

[codefromthecrypt]

As it might not be obvious to folks used to using agents, there are a few main points (non exhaustive, just to jog the mind)

• static is only really required for things that initialize statically, such as JDBC drivers.
  • in brave we have the ability to lookup statically for only this use case really
• static can still handy for end user code doing something like tagging, but that's a narrow case
  • some users will not have a dependency injection system at all and can still look up statically, but shouldn't be encouraged to do this as the first
• static implies overhead to lookup something, sometimes lazy registration etc
  • as this is telemetry code, it should not do things by default that cause more overhead
• static might not imply overhead when bytecode instrumentation (ex agents) are used
  • however agents can also overwrite fields just as easily!

You may not agree with these points, but in your rationale, please mention there are trade offs when the library is biased towards everything static registry.

[trask]

@adriancole What if I rename CurrentContext to ThreadContext to make it clearer that this util class is only applicable for users who wish to thread-bind their Context?

I agree that the word "current" is taking on too much here, as there are lots of ways to configure and pass around a "current" Context.

It sounds like your other concern is that the example code is skewed towards using this, rather than injecting and passing around the Context manually?

[codefromthecrypt]

it is less about the name more about the last thing you mentioned. Though I wouldn't say using a field implies end users are doing this manually.

First, that something besides static registry is managing something doesn't imply manual. Normal Spring, Guice etc can bind and so I wouldn't call something that is field based manual especially in examples copresented with tracer as a field. Context is an even lower abstraction than tracer..

Secondly, this is less about end users, more about instrumentors. Most end users are only going to be tagging, using automatic features like annotations, etc. not using tracing libraries to perform scoping commands. An api for the minimal surface is a different problem than scoping ops iotw.

Make sense?

[codefromthecrypt]

the point about audience means, we should encourage behavior that leads to least bugs and config glitches and classloader problems when the audience is instrumentors, as misunderstandings here cause a large blast radius of problems. Examples are how people learn, so we should be careful what they are learning to do.

[trask]

For dependency injection, are you talking about an object that has short-lived scope and gets the real Context injected:

@Bean
@RequestScope
class MyBean {
  @Autowired
  Tracer tracer;

  @Autowired
  Context context;

  ...
}

or are you talking about injecting something similar to CurrentContext, which gives access to the thread-bound Context from inside the class (without any static methods exposed to the user at least):

@Controller
class MyController {
  @Autowired
  Tracer tracer;

  @Autowired
  CurrentContext currentContext;

  ...
}

[codefromthecrypt]

ps renaming doesnt address the root concern. if getting the instance of context statically it is better to do something like Context.current() which returns a context vs expose methods that it has as statics. even if folks like static initialization it is similar logic to Logger which while often initialized statically doesnt also expose log as a static.. rather that's always an instance method. opting out of static methods helps people do the right thing in normal cases where you can inject something or abnormal when you cant and thus rely on static. ack that normal is relative to pov but in any case the point remains. hope that helps

…

On Tue, May 12, 2020, 11:27 AM Trask Stalnaker ***@***.***> wrote: @adriancole <https://github.com/adriancole> What if I rename CurrentContext to ThreadContext to make it clearer that this util class is only applicable for users who wish to thread-bind their Context? I agree that the word "current" is taking on too much here, as there are lots of ways to configure and pass around a "current" Context. It sounds like your other concern is that the example code is skewed towards using this, rather than injecting and passing around the Context manually? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1189 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPVV6XWDLRG4XLHOCRJVDRRC6YVANCNFSM4M35GGYA> .

[yurishkuro]

@trask I think Adrian is talking about your second example: a "context manager" provided as dependency (similar to tracer provided as dependency), where "context manager" is a pure interface without static methods for getting current context. This allows maximum flexibility in swapping the implementations. It is also compatible with explicit context propagation.

[codefromthecrypt]

I think what you are calling CurrentContext is similar to what we call CurrentTraceContext in brave, except that yours takes a span also I think. At any rate, yeah this is the thing I'm discussing.

A slimmed down current span api for users is a separate thing.. in brave we have SpanCustomizer for this which avoids some problems. (ex a lazy lookup implementation is possible as is a no-op for request based customizations)

[trask]

@adriancole @yurishkuro thanks for the feedback. I'll take another stab at it in a few days and post back for more input.

[trask]

Latest changes:

• Renamed CurrentContext to ScopeManager, and changed its static methods to instance methods
• Injected ScopeManager everywhere (some TODOs left in contrib code)

It definitely makes dependencies clearer, e.g. having to pass ScopeManager to the TracerProvider:

public interface TraceProvider {
  TracerProvider create(ScopeManager scopeManager);
}

The circular dependency between the io.opentelemetry.scope and io.opentelemetry.trace (and io.opentelemetry.correlationcontext) packages is not ideal.

I could remove the Span (and CorrelationContext) methods in ScopeManager, and bring back Tracer.withSpan, Tracer.getCurrentSpan (and similar methods in CorrelationContextManager).

That would not keep as clear of a separation for thread-bound context usage though, which was nice about isolating them all inside ScopeManager.

Though we use "current span" in Tracer already (when creating a new span, inheriting the "current span" as parent), so I guess the separation is not perfect anyways, and bringing back Tracer.withSpan maybe not so bad.

I guess the other option is to just drop those helper methods completely, but that leaves the user doing a lot:

scopeManager.withContext(Span.Key.put(span, scopeManager.getContext()))

and

Span.Key.get(scopeManager.getContext())

(the later isn't so bad, but not as nice as scopeManager.getSpan() or Tracer.getCurrentSpan())

One last thing, along these lines of reducing statics, maybe we should replace usage of Context.current() with scopeManager.getContext().

[trask]

Closing this. There's some good discussion archived above, and various things prototyped in the PR, but the PR itself is very out-of-date now, and mostly useful as a reference if someone wants to continue moving any of these ideas forward.