[Do not merge] Initial prototype for context-prop overhaul.

[carlosalberto]

This is an initial prototype for the context-prop changes - below is a summary of the changes, in addition to a possible alternative approach for this (also, this prototype is meant to be reviewed by non-Javaers, in order to validate the contest-prop proposal).

Observe that the main code is not touched, and only the new classes have been added (I had intended to re-adjust the entire code base, which I'd be happy to do once we decide the path to take).

Also, DistributedContext and its related class would be removed under this initial protoype.

Summary

Context

The current effort makes Context an explicit parameter for all related operations - with the help of Context.current(), which makes the current instance available all time. This Context class acts as layer on top of io.grpc.Context, in case we/some vendor want to replace with something else (while also hiding advanced features, such as cancellation).

Context ctx = OpenTelemetry.baggageManager().setValue(Context.current(), "val1", "key1");
try (Scope scope = Context.setCurrent(ctx)) {
   OpenTelemetry.baggageManager().getValue(Context.current(), "val1");
}

Actual top-level objects (active Span, the container for baggage, the container for correlations, etc) all live in its own bucket within Context, which can be set/accessed through a key object, which is kept privately by each owner (and accessible to the Injector/Extractor in turn):

class TracerSdk {
    static final Context.Key currentSpanKey = Context.createKey("activeSpan");
    Span getCurrentSpan() {
        return Context.current().getValue(currentSpanKey);
    }

Simple string keys cannot be used to access the underlying object, as a limitation imposed by io.grpc.Context. But direct access to the objects in Context could be offered (as opposed to have convenience methods such as Tracer.getCurrentSpan()), through exposing a set of predefined keys:

CURRENT_SPAN_KEY = Context.createKey("currentSpan");
CURRENT_CORRELATIONS_KEY = Context.createKey("currentCorrelations");
...
    span = Context.current().getValue(CURRENT_SPAN_KEY);

Question: Should we make this interface, and make it pluggable as part of the OpenTelemetry object? Then it could be accessed through OpenTelemetry.getContextManager().current().

Baggage/Correlation managers

No direct access would exist to the baggage/correlation containers, and such values would have to be fetched through their respective managers:

ctx = OpenTelemetry.baggageManager().setValue(Context.current(), "key1", "val1");
ctx = OpenTelemetry.correlationManager().setValue(Context.current(),
                       label, HopLimit.NO_PROPAGATION);

String val = OpenTelemetry.baggageManager().getValue(ctx, "key1");

Propagators

Injectors/extractors would work directly with a Context object, fetching each own the bucket they want and work with it:

import static com.myimplementation.InternalContextKeys.BAGGAGE_KEY; 

class HttpInjectorSdk implements HttpInjector {
    @Override
    public <C> void inject(Context ctx, C carrier, Setter<C> setter) {
        BaggageSdk baggageSdk = (BaggageSdk) ctx.getValue(BAGGAGE_KEY);
        ....
    }
}

Alternative approach

During playing with the approach above, I though about an alternative approach:

• Make Context an explicit component (just like above) which works mostly behind the scenes.
• Rename DistributedContext to CorrelationContext and adjust the operations (remove getEntryValue(), etc).
• Create a new BaggageContext object, with the set/clear operations (BagaggeContext.removeValue(), for example).
• Have operations such as Tracer.getCurrentSpan() and CorrelationContext.setCurrent() stay, which would work with Context behind the scenes, implicitly (so the user does not have to think of the context all the time). Context could, however, be accessed all the time as an advanced feature.
• Propagators would be the only component using Context directly, so Injection/Extraction can be done in a single step.

Code with this design would look like this:

Context ctx = extractor.extract(...);
try (Scope scope = Context.setCurrent(ctx)) {
   // Do work
   // ...
   try (Span span = tracer.setCurrent(span)) {
       // Context.current() would hold the current Span behind the scenes.
       // ...
      injector.inject(Context.current(), ...);
   }
}

[codecov-io]

Codecov Report

Merging #655 into master will decrease coverage by 0.65%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master     #655      +/-   ##
============================================
- Coverage     79.39%   78.73%   -0.66%     
  Complexity      712      712              
============================================
  Files            88       90       +2     
  Lines          2514     2535      +21     
  Branches        238      238              
============================================
  Hits           1996     1996              
- Misses          418      439      +21     
  Partials        100      100

Impacted Files	Coverage Δ	Complexity Δ
...emetry/distributedcontext/CorrelationsManager.java	0% <0%> (ø)	0 <0> (?)
...rc/main/java/io/opentelemetry/context/Context.java	0% <0%> (ø)	0 <0> (?)

---

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 478eb3b...3034102. Read the comment docs.

[bogdandrutu]

Is this what we called Tags in opencensus? I like Tags better.

[carlosalberto]

Yes - I called this Correlations as this is the used name in the related OTEPS ;)

[bogdandrutu]

Please tell @tedsuo that Tags is a better name :)

[tedsuo]

@bogdandrutu you and @yurishkuro have a jello wrestling match, the winner can pick the name. :)

[bogdandrutu]

If we add this class we also need all the helpers:

• Wrap callable/runnable
• Wrapper for an Executor
• Run/Call helper to run a Runnable/Callable with the context.

Also we need to make sure somehow that users are not double wrapping things with this Context and io.grpc.Context when they use both opentelemetry and io.grpc.Context.

[carlosalberto]

If we add this class we also need all the helpers:

Good point, yes. Although I wonder if we could postpone them (for a second version), or you think we would need them right from the start?

[bogdandrutu]

Without them users cannot instrument their code. That is the main reason I actually used directly the grpc one, to not duplicate code unnecessary.

I would like to see how an RPC integration will look like with the new Injector/Extractor

[tylerbenson]

Is baggage expected to be attached to and reported with each span? If not, maybe we should consider separating baggage out to a separate project as well.

(Sorry, I don’t have a great intuition around baggage.)

[carlosalberto]

Is baggage expected to be attached to and reported with each span?

No, it won't be attached (by default). And yes, there has been some talk about separating this one (as well as context prop itself ;) )

[carlosalberto]

cc @yurishkuro (probably this can be of interest to you ;) )

[tedsuo]

Not to complicate things, but having the SDK be able to access the entire context during span calls was my solution for having baggage be an independent system, which could still optionally be reported by the observability system.

[carlosalberto]

access the entire context during span calls

So, as long as Context an explicit component, the SDK could access the underlying (implicitly active) instance at anytime (including all its members), i.e.

Span getCurrentSpan() {
  String baggageVal1 = OpenTelemetry.baggageManager().getValue(
                 Context.current(), "mykey");
  ...
  return (Span) Context.current().getValue(CURRENT_SPAN_KEY);
}

[carlosalberto]

That is the main reason I actually used directly the grpc one, to not duplicate code unnecessary.

This concern raised by @bogdandrutu makes me wonder about not having an intermediate layer abstracting the Context concept in every language - for Java, this would simplify things, but it would force all implementations to have to use io.grpc.Context.

[bogdandrutu]

In languages like Go, Python that have a Context already we should just use that. In other languages we should decide between implement our own or use an existing one.

[carlosalberto]

Hey @arminru

Was wondering about your opinion here, regarding using grpc.Context explicitly (and making it a hard dependency from the API side), as you guys might want to create/have your own SDK ;)

[trask]

Hi @carlosalberto! Also see #575 against having api dependency on grpc.Context.

[arminru]

@carlosalberto I don't see any issue for us in particular but I am wondering if adding a hard dependency to io.grpc.Context to the API layer is justified when only using a quite limited subset of its features.

[carlosalberto]

Hey all - the latest iteration does the following:

• DistributedContext becomes CorrelationContext (uses an implicit style, as no Context is explicitly passed around, but it is simply used behind the scenes through CorrelationContext and related classes).
• Added io.opentelemetry.baggage to contain BaggageManager (uses an explicit style, as Context is always explicitly provided, and the underlying container acts as an opaque object, which simply gets/sets/resets provided values).
• CorrelationContext and BaggageManager expose different styles as a way to exercise their different APIs (in the end result, we will stick to a single style).
• Added a few dummy propagators (testing purposes).
• An dummy example was added to test out a request (api/src/test/java/io/opentelemetry/contextprop/PropagatorsTest.java)

// Extract.
Context ctx = extractor.extract(Context.current(), inboundCarrier, new MapGetter());

try (Scope scope = Context.setCurrent(ctx)) {
  ...

Feel free to review it and comment the ongoing prototype/API.

Notes/questions

• I'm wondering more and more the benefits of separating a given propagator into Extractor and Injector, as they might share too many technical details. It would be nice to document a set of scenarios that may require this separation.
• Context.Key objects have to be defined publicly, so the slots in Context can be re-used by code/propagators. For the moment, each sub-component (trace, baggage, etc) has to define/contain them (no single place for all of them, that is).
• For chained propagators (if we decide to expose them ourselves, rather than in a contrib library), using a single list would be simpler.
• Span and SpanContext existence/usage in Context will have to be 'normalized' to properly handle both (i.e. whether a SpanContext in the current Context could be exposed as the current Span or not, and similar details).

PS - I still have the Context abstraction on top of grpc.Context, but as discussed, we might be removing it ;)

[arminru]

• I'm wondering more and more the benefits of separating a given propagator into Extractor and Injector, as they might share too many technical details. It would be nice to document a set of scenarios that may require this separation.

I don't know of any necessity either but it doesn't really hurt since one can just implement both interfaces in a single propagator class and share that instance for both injecting and extracting.

[carlosalberto]

Btw, as inspecting the dummy example in the actual code changes, I'm pasting it a link to a gist of it ;) https://gist.github.com/carlosalberto/1d91dc23967ff6442758d858b0d0fe25

[mwear]

Should there be a separate key for span context or is it actually being stored in baggage?

[carlosalberto]

Separate key for SpanContext ;)

[mwear]

I see, there is a getSpanContextKey for each context (key).

[tylerbenson]

As discussed in our meeting today, This interface should have another method that provides access to all the headers:

Iterable<String> headers();

[carlosalberto]

@bogdandrutu I've 'ported' a super small integration for web-servlet (from OT, as that's the one I was slightly familiar with), which shows how extraction would work (even if only SpanContext is handled). I can port an integration that does injection (such as the okhttp one) if you feel we should have that one too.

(I'm also wondering if you need something more specific, such as Thrift or gRPC - let me know if that's the case ;))

The important code lives in TracingFilter.java:

/* In a real world scenario, a single instance would be shared/used. */
HttpTraceContextExtractor extractor = new HttpTraceContextExtractor();

/**
* SpanContext *and* other members (such as correlationcontext) would be extracted here, and
* make it available in the returned Context object.
* 
* For further consumption, the returned Context object would need to be explicitly passed
* to DistributedContext/Baggage handlers, or else set it automatically as the current instance.
*/
Context ctx =
  extractor.extract(Context.current(), httpRequest, HttpServletRequestGetter.getInstance());
SpanContext extractedContext = ctx.getValue(ContextKeys.getSpanContextKey());
Span span =
  tracer
      .spanBuilder(httpRequest.getMethod())
      .setParent(extractedContext)
      .setSpanKind(Span.Kind.SERVER)
      .startSpan();

[carlosalberto]

Hey all - I've added the Global Propagators part from the referenced prototype. This is now implemented using a class (should we do interface?) called Propagators (to be renamed to something that sounds better).

public final class Propagators {
  public static Propagators create(HttpInjector injector, HttpExtractor extractor);
  public HttpExtractor getHttpExtractor();
  public HttpInjector getHttpInjector();
  // Add here binary propagators and other members.
}

Then it can registered in OpenTelemetry:

HttpInjector injector = ChainedPropagators.chain(...);
HttpExtractor extractor = ChainedPropagators.chain(...);
OpenTelemetry.setPropagators(Propagators.create(injector, extractor));
...
OpenTelemetry.getPropagators().extract(Context.current(), carrier, carrierGetter);

This is a very simple design that, I think, should work without many problems. Users would consume such Propagators instance, which would simply expose the extractor/injector, and that is all.

Putting all propagators in a single class could also be used to remove the chained registration if needed, as we could even have something like:

Propagators props = Propagators.newBuilder()
  .addHttpExtractor(TraceContextExtractor())
  .addHttpExtractor(DefaultCorrelationContextExtractor())
  .addHttpExtractor(DefaultBaggageExtractor())
  .build();

OpenTelemetry.setPropagators(props);

[carlosalberto]

Closing this on behalf of #720