Context usability improvements

[anuraaga]

Context is an object that is a global concern in OpenTelemetry, while the "current span" is a concept of tracing specifically. I'm hoping we can model this ownership correctly, while also provide users an intuitive way of accessing these objects when they need to.

Accessing Context

The Context is outside of OpenTelemetry and something we use statically to insert telemetry information. So we only expect static access like this

class Context
  static Context current() {
    return contextImplementation.current();
  }

Accessing Span

The active span is only available during tracing. The parallel to the above for a span is

class Span {
  static Span current() {
    return Context.current().getValue(CURRENT_SPAN);
  }
}

This is very important, since many users don't do any tracing at all, they're just using the agent, or added some library instrumentation. But adding attributes is common - Span.current allows them to enrich spans without worry about the concept of tracing itself.

Making context / span current

Because the context / span are by definition available if a user needs to make them current, instance methods are convenient try (Scope ignored = context.makeCurrent()), try (Scope ignored = span.makeCurrent()).

Adding span to context

The final operation is adding a span to a existing context, which instrumentation authors need. Because we only really expect instrumentation authors to use this, tracer.addToContext(Span span, Context context) or span.addToContext(Context context) both seem OK.

[Oberon00]

Context is an object that is a global concern in OpenTelemetry

class Context
  static Context current() {
    return OpenTelemetry.get().currentContext();
  }

It's the other way round: Context is not in itself related to telemetry, but telemetry uses Context. So I think the code snippet above violates the layering prescribed by the spec.

[anuraaga]

I guess that makes sense - in that case context storage will always be separate from OpenTelemetry and initialized / accessed statically I think. In that case, I think the parallel to Context.current() of Span.current() is important, and we may not even bother with tracer.currentSpan()?

[Oberon00]

+1 on removing current span access from tracer. EDIT: It also removes the confusion I have often seen, where users wanted to get access to a particular named tracer because they wanted the current span on that tracer (which is not a thing)

[iNikem]

tracer.getCurrentSpan() should die. It really makes an impression, that spans are somehow scoped/related to tracers and you have to have the correct tracer to get a span. I think this is a huge source of confusion.

[bogdandrutu]

I think that was the whole purpose of open-telemetry/opentelemetry-specification#1019 to remove the confusion and avoid duplicate shortcuts/functionality in one package. Currently after the latest PR @anuraaga merged we have 3 ways to get access to the current Span in the trace package :))) we must go deeper maybe add this to the TracerProvider to not require using a name for the Tracer :))) (joking)

[anuraaga]

I've reread #1019 about 10 times now to try to make sure to align with it but unfortunately still have a hard time understanding what it's saying. If the point is OTel must not provide an API to access the current Context, then things get more clear.

Currently after the latest PR @anuraaga merged we have 3 ways to get access to the current Span in the trace package

Which PR? Can't find adding a new way to access the span.

That being said, I think having multiple ways to do the same thing is not automatically a bad thing as long as it provides real value, which tracer.currentSpan wouldn't if it just delegates to a static lookup anyways. If something is useful to users we should do it - any time the spec blocks that, it failed.

[bogdandrutu]

Sorry, is not yet merged #1795

If the point is OTel must not provide an API to access the current Context, then things get more clear.

The points are very simple, but had to explain it in a very complicated way, because sometimes I feel simple things are not easy to explain:

1. Every package that we provide (and uses Context): trace, baggage - should be consistent on how do they expose the context interaction utils;
2. We need 2 main functionalities: 1. how to interact with a Context instance object; 2. how to interact with the CurrentContext; The second may be missing in languages like golang;
3. Avoid duplicate functionalities in the same package (trace, baggage). Right now even without Move static access of current span to Span. #1795 we still have 2 ways to access the current Span on the Tracer and in the TracingContextUtil.

Indeed initially I suggested that we should not provide the API to access the CurrentContext, with the idea that if that functionality needs to be injected, we complicate more things than simplify unless we do SPI as we do now and we have a global. Also I was thinking that it can be removed for the moment (because it is just a shortcut) and we can add later when we understand better the CurrentContext.

[bogdandrutu]

which tracer.currentSpan wouldn't if it just delegates to a static lookup anyways.

Exactly my point, maybe you call "multiple ways to do the same thing" differently than me and you have examples in mind where that can help, but I cannot think of an example where that is good. Maybe I am limiting the definition to just "duplicate code" like the example above, and that's why I think is always bad.

[anuraaga]

Sorry, is not yet merged #1795

One way is made private so don't think so. Hope it makes clear we are generally on the same page :)

The main point is whether context is owned by OTel or not. For telemetry APIs so far except OC, it was - in that pattern, I think openTelemetry.currentContext(), tracer.currentSpan() still do make sense as in my original proposal. But if context is totally separate, than no, the only way to access in a practical way is static, so we definitely don't need multiple static ways. That makes sense to me so updated this issue.

[carlosalberto]

But if context is totally separate, than no, the only way to access in a practical way is static, so we definitely don't need multiple static ways.

+1 for this one. I still feel that, because of how we do things in Java, static all the way is the only way to go.

[jkwatson]

So, are we converging on something like this?

Baggage baggage = Baggage.getCurrent()
Scope s = baggage.addToCurrentContext()

Context withBaggage = baggage.addToContext(context)

Span span = Span.getCurrent()
Scope s = span.addToCurrentContext()

Context withSpan = span.addToContext(context)

This will keep the Tracer a very anemic factory for spans, with just one method on it for creating Span.Builders, but at least this will get us out of TracingContextUtils, which is a giant step in the right direction.

Any thoughts on what to do with the Tracer, now that it's nothing but a span factory? I guess the Meter is only an instrument factory, so maybe that parallelism is the right way to go, at least for consistency?

[Oberon00]

That looks quite sensible to me. I think we also need Span.fromContext(context), etc though.

Some naming suggestions:

• If we add Concern.fromContext(context), it might be better to rename getCurrent to fromContext too.
• Maybe instead of addToContext, we should use withContext. "add" is a bit problematic IMHO since we more often than not, replace a previous span instead of just adding a new value. Other ideas: setInContext, inContext, in, with.

[jkwatson]

That looks quite sensible to me. I think we also need Span.fromContext(context), etc though.

Some naming suggestions:

• If we add Concern.fromContext(context), it might be better to rename getCurrent to fromContext too.
• Maybe instead of addToContext, we should use withContext. "add" is a bit problematic IMHO since we more often than not, replace a previous span instead of just adding a new value. Other ideas: setInContext, inContext, in, with.

I like fromContext for the extraction from the context, and inContext for the one that returns a Scope for that context. How about appendToContext for the one that returns you a context with the item appended to the context?

So, updated:

Baggage baggage = Baggage.fromContext()
Scope s = baggage.inContext()

Context withBaggage = baggage.appendToContext(context)

Span span = Span.fromContext()
Scope s = span.inContext()

Context withSpan = span.appendToContext(context)

[Oberon00]

I like append even less than add. It suggests that the first item is still there, and it also suggests that there is some ordered list in the context.

[jkwatson]

I like append even less than add. It suggests that the first item is still there, and it also suggests that there is some ordered list in the context.

"withContext" doesn't make any sense, though, since you're putting the item into the provided context and returning the updated context.

[carlosalberto]

Hey @jkwatson

This will keep the Tracer a very anemic factory for spans, with just one method on it for creating Span.Builders, but at least this will get us out of TracingContextUtils, which is a giant step in the right direction.

We (very) briefly mentioned the possibility of having Tracer.getCurrentSpan() etc, i.e. all this functionality as static methods on the Tracer. Any opinion on that?

[Oberon00]

"withContext" doesn't make any sense, though, since you're putting the item into the provided context and returning the updated context.

So you get the the item with the provided Context merged into a new context. To me it makes sense. "a with b" is the same as "b with a" in this case.

[jkwatson]

"withContext" doesn't make any sense, though, since you're putting the item into the provided context and returning the updated context.

So you get the the item with the provided Context merged into a new context. To me it makes sense. "a with b" is the same as "b with a" in this case.

but, you're definitely putting the span/baggage into the context, and not the other way around. If we don't make that clear from the API, users will be confused.

[jkwatson]

"withContext" doesn't make any sense, though, since you're putting the item into the provided context and returning the updated context.

So you get the the item with the provided Context merged into a new context. To me it makes sense. "a with b" is the same as "b with a" in this case.

but, you're definitely putting the span/baggage into the context, and not the other way around. If we don't make that clear from the API, users will be confused.

Which is why I don't like the "with" word. :)

[jkwatson]

Hey @jkwatson

This will keep the Tracer a very anemic factory for spans, with just one method on it for creating Span.Builders, but at least this will get us out of TracingContextUtils, which is a giant step in the right direction.

We (very) briefly mentioned the possibility of having Tracer.getCurrentSpan() etc, i.e. all this functionality as static methods on the Tracer. Any opinion on that?

It wasn't clear to me that there's much appetite to doing that, since you wouldn't do the same with baggage. If we want parallel APIs, then we need to make baggage and spans work the same way.

[carlosalberto]

since you wouldn't do the same with baggage

Wouldn't it be the 'same' having a static BaggageManager.getCurrentBaggage()?

(Not trying to defend this, just trying to get an overview of our options ;) )

[jkwatson]

since you wouldn't do the same with baggage

Wouldn't it be the 'same' having a static BaggageManager.getCurrentBaggage()?

(Not trying to defend this, just trying to get an overview of our options ;) )

Possibly, but I'm assuming at this point that the BaggageManager is going away very shortly due to open-telemetry/opentelemetry-specification#1070.

[anuraaga]

First, about static methods on Tracer, I do feel strongly that we should not do this because of my original point of

many users don't do any tracing at all, they're just using the agent, or added some library instrumentation. But adding attributes is common - Span.current allows them to enrich spans without worry about the concept of tracing itself.

If we can provide all the functionality such users need in the Span with a seemingly idiomatic API, I think we should go for it, manipulating spans becomes extremely simple. I agree with @jkwatson's sentiment that Tracer is a boring class that is just a holder of instrumentation library information at this point, but I don't think that means we need to shoehorn context interaction just to give it some more methods.

So for methods on a Span, I also like @Oberon00's from suggestions. So for accessing span, it becomes

Span Span.fromCurrentContext()
Span Span.fromContext(context)

This has a downside of not having a parallel with Context, e.g. Context can really only be Context.current() since there's nothing to from it with (well, Context.fromCurrentStorage is conceivable but doesn't seem good in any way). This seems OK to me though. At the same time,

Span Span.current()
Span Span.fromContext(context)

also seems OK mostly since the former is shorter and will likely be the most common for end users. I think I lean towards it with less risk that a user will static-import current than statically importing fromCurrentContext - it's pretty long and there will be a tendency to do so, ruining the readability of their code.

For inserting span there seems to be less consensus. add / append having problems sounding like they don't overwrite makes sense. So if focusing on set, we may have

Context Span.setInContext(context)

Unfortunately, Span.setInCurrentContext() returning a Context seems weird, but if it returned a Scope, there would be a lack of symmetry which currently hurts my eyes any time I read the TracingContextUtils class. Really, what we want is context.withSpan since usually the thing you are mutating is on the left side in Java.

So how about this idea?

interface ContextValue {
  Context storeInContext(Context context);
}

interface Span extends ContextValue {

  default Context storeInContext(Context context);
    // Any package private class is fine to hold the key
    return context.withValues(PropagatedSpan.SPAN_KEY, this);
}

  default Span getContextValue() {
    return this;
  }
}

interface Context {
  Context withValues(ContextValue value) {
    return value.storeInContext(this);
  }
}

try (Scope ignored = Context.current().withValues(span).makeCurrent()) {
}

// In practice, instrumentation needs a context before creating a span, then new context, almost all the time.
// So usually
Context context = Context.current();
Span span = tracer.spanBuilder().setParent(context).start();
try (Scope ignored = context.withValues(span).makeCurrent()) {
}

I think it solves the desire of keeping the context key private to tracing while also giving us an idiomatic Java API where we would usually mutate this with that rather than adding this to that.

[jkwatson]

I really like the idea of the Context exposing an interface that makes it easy for it to interact with, across any of the items that need propagation.

Your proposal seems pretty good, although I would s/withValues/withValue/ personally, since it's only putting one value into the context.