Span activation should activate parent context

[zeitlinger]

Is your feature request related to a problem? Please describe.

Baggage propagation is not intuitive.

Describe the solution you'd like

I would like to be able to access the baggage, even if I omit the extractedContext.makeCurrent() statement below:

        Context extractedContext = textMapPropagator.extract(Context.current(), req, EXTRACT_PROPERTIES_FROM_HEADER);
        try (Scope ignore = extractedContext.makeCurrent()) {
            Span serverSpan = tracer.spanBuilder(handlerMethodName(req, handlerMethodPerInfo))
                                    .setParent(extractedContext)
                                    .setSpanKind(SERVER)
                                    .startSpan();

            try (final Scope scope = serverSpan.makeCurrent()) {
               //here I have access to baggage using Baggage.current()
            } finally {
                serverSpan.end();
            }
        }

Describe the solution you'd like

I already passed extractedContext to the span and activated it.

[jkwatson]

You can always explicitly extract the Baggage from any given Context instance via Baggage.fromContext(context). Does that work for you?

[zeitlinger]

You can always explicitly extract the Baggage from any given Context instance via Baggage.fromContext(context). Does that work for you?

Yes, that works (System.out.println(Baggage.fromContext(extractedContext));) - but it doesn't help me in another method where I don't have access to the context.

[anuraaga]

Hi @zeitlinger - because of how import it is to close when activating a new context, we do need the core API to separate the concepts of creating a span and activating a context. We've seen some APIs that share the two and it becomes quite common to forget to clean up context leading to bugs or data corruption.

We do have some very primitive helpers in the incubator here

https://github.com/open-telemetry/opentelemetry-java/blob/main/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedTracer.java#L29

A pattern like run(spanCreationParams, () -> { myCode() }) is safe and may be an API closer to what you are imagining. If you have any thoughts on a useful utility for span creation would you be interested in contributing it to that incubator module?

[zeitlinger]

That would be a nice utility function on top - but this is not what this ticket is about.

In the example, I would suggest that serverSpan.makeCurrent() should activate extractedContext first - or include the baggage of extractedContext for a similar effect.

[zeitlinger]

Another solution would be to let the extract method activate the context - or make it the the extractAsCurrent and deprecate extract.

[jkwatson]

Can you explain what precisely you mean by "activate" the context?

Also, any baggage in a parent context will be available on a child context, automatically.

[trask]

IIUC, the confusion/request is that Span.makeCurrent() doesn't use the Context that was passed into SpanBuilder.setParent(Context).

@zeitlinger, I would write the above example as

        Context extractedContext = textMapPropagator.extract(Context.current(), req, EXTRACT_PROPERTIES_FROM_HEADER);
        Span serverSpan = tracer.spanBuilder(handlerMethodName(req, handlerMethodPerInfo))
                                .setParent(extractedContext)
                                .setSpanKind(SERVER)
                                .startSpan();

        try (final Scope scope = extractedContext.with(serverSpan)) {
           //here I have access to baggage using Baggage.current()
        } finally {
            serverSpan.end();
        }

but it doesn't help me in another method where I don't have access to the context

Can you show what this example looks like?

[jkwatson]

IIUC, the confusion/request is that Span.makeCurrent() doesn't use the Context that was passed into SpanBuilder.setParent(Context).

Ah! I had totally missed that as what was desired. Thanks for clarifying, @trask.

It would be very odd to have the parent Context being the one that is in the put into the current Scope...since it wouldn't include the current Span! I think if you want something like this, it would be best to have it be something external to the core API, since having this be the default behavior would be pretty confusing.

[trask]

I think the ask was for span.makeCurrent() to put the span into the Context that was passed into SpanBuilder.setParent(Context) and then make that resulting Context active.

The confusion is that this looks like it should work but it doesn't:

        Context extractedContext = textMapPropagator.extract(Context.current(), req, EXTRACT_PROPERTIES_FROM_HEADER);
        Span serverSpan = tracer.spanBuilder(handlerMethodName(req, handlerMethodPerInfo))
                                .setParent(extractedContext)
                                .setSpanKind(SERVER)
                                .startSpan();

        try (final Scope scope = serverSpan.makeCurrent()) {
-->        // Baggage.current() doesn't work here
        } finally {
            serverSpan.end();
        }

[anuraaga]

Sorry didn't understand the question at first, now following. This is a tricky issue. Let's first remember (or define for the first time maybe :P) the definition of an active context, it is arbitrary state associated with a thread of execution, or more abstractly a line of code. Let's extend the example to show this

    Context baseContext = Context.current().with(BASE_BAGGAGE);
    try (Scope scope = baseContext.makeCurrent()) {
      Context extractedContext =
          textMapPropagator.extract(Context.current(), req, EXTRACT_PROPERTIES_FROM_HEADER);
      Span serverSpan =
          tracer
              .spanBuilder(handlerMethodName(req, handlerMethodPerInfo))
              .setParent(extractedContext)
              .setSpanKind(SERVER)
              .startSpan();

      // baseContext is active for this line of code
      try (final Scope scope = serverSpan.makeCurrent()) {
      } finally {
        serverSpan.end();
      }
    }

There are two contexts with different baggage, baseContext and extractedContext. Per our definition of active context, the baggage in baseContext is the baggage associated with the line of code serverSpan.makeCurrent(), so it is getting used. I agree this is highly unintuitive given we set a parent context for serverSpan, but it would be a significant break from context semantics if we used extractedContext when calling span.makeCurrent().

Also just for reference, one thing to remember is span.makeCurrent() exists mostly for internal spans - we didn't really want that method but it would lose too much convenience for internal spans, e.g.

Span span = tracer.spanBuilder(foo).startSpan();
try (Scope scope = span.makeCurrent()) {
}

With internal spans, it's very uncommon to pass in an explicit parent, and it is nice to not have to worry about Context at all when creating them. The method does cause the current confusion when actually extracting a parent context though.

One important note is that I think we expect 99% of use cases that need to extract context are writing instrumentation, as opposed to using instrumentation. We expect instrumentation authors to almost always use the instrumentation API, not the opentelemetry API. The instrumentation API does not have this problem as it only operates on Context, not per-signal

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java#L133

We do need to get this stabilized, but @zeitlinger is this something you would be able to use in the future?

I suspect we don't want to change the semantics of Span.makeCurrent as it does seem like a significant behavior change semver-wise, though I'm not totally against considering it a bugfix, but the instrumentation API option makes me thing we would tend to avoid the risk. We could easily add a debug log though when the parent has been explicitly set and the span is being activated into a context which is not that parent. Would that be useful? We would also want to add to the javadoc for setParent and Span.makeCurrent the caveat of the latter not using the former.

[zeitlinger]

Thanks for all the feedback - and sorry for not making a better example.

I really liked this example to demonstrate the problem with the current API.

As pointed out, this is probably an edge case because users are mostly not using it.

This proposal would avoid the confusion - because it would link context extraction and context activation:

            try (Scope extractedScope = textMapPropagator.extractAndMakeCurrent(Context.current(), req, EXTRACT_PROPERTIES_FROM_HEADER)) {
                //current context has the baggage already
                Span serverSpan =
                        tracer
                                .spanBuilder(handlerMethodName(req, handlerMethodPerInfo))
                                .setSpanKind(SERVER)
                                .startSpan();
            }

I'm not sure if a new method is needed - or if we can change the existing extract method.