Proposing a single formatter for all contexts

[toumorokoshi]

No description provided.

[yurishkuro]

Context object is not part of the wire propagators layer. E.g. context.Context in Go or io.grpc.Context in Java are the containers of all types of contexts.

[toumorokoshi]

This isn't great from a terminology standpoint, but I think there's a distinction between:

1. a composed context that is there to help formatter authors return back only a single object
2. a context object that is designed to actually handle storage of the current context.

The UnifiedContext is not intended to be a long-lived container for all types of contex: as you mentioned there is a more general context component for that.

Ths UnifiedContext is strictly for formatters. A propagator would then take the unifiedcontext, retrieve the various context objects, then put them into the higher-level "Context" object.

The reason for the separation was to simplify and standardize the way that context is populated. For example, DistributedContext is the same throughout the process, while SpanContext can be switched out with a new span. As such, the way one may insert that value into the context may change.

I understand the desire to not attach the general context objects to the wire propagators. but something does need to eventually handle taking stuff from wire propagators and adding them to the general context.

[yurishkuro]

My preference would be to avoid an intermedia "unified context" object. The propagators themselves are still different depending on which type of context they are handling, e.g. a span context propagator really has very little in common with baggage propagator. So at a matter of the API they can both accept the underlying Context object and read the specific type of context they need.

I can see how there are some perf benefits that can be achieved on extracting contexts from the wire and combining them into the in-memory Context object if the latter supports some sort of bulk write mode (which, for example, Go context does not support). But that still means that propagators need access to such bulk write builder, not that they need to deal with an extra "unified context" container, which requires memory allocations.

[toumorokoshi]

I don't think that the span context and distributed context propagator are that separate. In terms of the functionality that they provide, yes, but I don't believe this is the case in terms of vendor integrations.

For example, let's say I own a telemetry SaaS company (name it MyTel), and I want to provide you a simple OTel integration. Let's say that MyTel has it's own specification for spans, and wants to also propagate it's own token app_key. Let's say that for MyTel's product to work properly, both must exist and be propagated.

As a vendor, I want to make it really easy to integrate MyTel's telemetry into an OTEL-based SDK. If the propagators were combined, I would just have to instruct the consumer to load my integration in as a propagator.

If they were separate, I would need to tell my users that they need to add both of those propagators. In addition, if one is not added correctly, then the user would have a very confusing issue on their hands where some of the data would be propagating correctly, and one would not.

My other reasoning is the fact that DistributedContext was designed to support CorrelationContext, which itself was designed to be an addition to the trace-context to have a more official way to supporting 3rd party labels. I think this implies that it will be very common for vendors to need to both integrate propagators with SpanContext and DistributedContext.

But regardless I think your proposal is calling to just eliminate the UnifiedContext entirely and instead use the existing Context object, which I think is fine. It does require the formatter implementer to know the getter / setter methods that are appropriate for each context, but it fulfills my primary concern.

[toumorokoshi]

The other thing that concerns me a little bit about context directly is the fact that propagators are also expected to honor the EntryPropgationFilter of values to propagate, which requires all formatter authors to be aware of that implementation detail:

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

But that could be a pretty easy to uphold convention, or logic added to the DistributedContext object itself.

[yurishkuro]

For MyTel to register one or all codecs it just needs to provide the user with a single registration function that will register all of them. Inside they can still be separate.

Don't know about the filters, it seems to have been added to the spec before the whole context framework is designed. We might change that.

[toumorokoshi]

I think the single registration function results in an inflexible framework. In otel-python we are having discussions around the ability to support multiple different configurations and propagator chains. I think if a vendor provides effectively a magic function that doesn't better expose the correct primitives, it will make it very difficult for consumers with more complex use cases.

But this is probably just a matter of preference. I've personally been bitten by "magic" functions that do everything for you: I'd much rather prefer concise primitives.

[yurishkuro]

You're now saying the exact opposite to what you said in your MyTel example where you did want a single registration. A single catch-all function does not mean that the vendor cannot also provide fine grained individual codecs.

[toumorokoshi]

Hey Yuri, I'm sorry if I'm not being clear. My proposal is for unifying the formatters such that a single formatter can act on all contexts. I was arguing in the case of MyTel that having multiple formatters can be error-prone, and we should minimize the number of boilerplate integrations that are needed to integrate with a specific SaaS provider.

Maybe my example is a poor one. I just wanted to illustrate the error-prone nature of having propagators that only work with a specific context. I believe it will be a common case to need to extract and inject from multiple contexts for integrations with SaaS providers or other backends to work as expected.

[yurishkuro]

I agree with the single formatter notion, because that's what the user code would be interacting with, which does not need to know how many actual formatters and contexts are configured behind the scene. But I am still not seeing the need for unified context because the default Context object is already acting as a unified container.

[yurishkuro]

Another concern is how propagators for different wire formats can be composed. For example, I may want a service to support both Zipkin B3 format and W3C TraceContext format. In addition, Jaeger codecs support dedicated headers like jaeger-debug-id that can be set manually to force sampling of individual requests. There needs to be a way to compose different codecs.

[toumorokoshi]

Yes! This is indeed an issue.

I have an open issue in the spec about it: open-telemetry/opentelemetry-specification#210. Happy to put this into an RFC as well, but there seems to be a line between what type of API changes can be proposed as an issue vs an RFC

[yurishkuro]

as a side note, I think booking an issue is appropriate when you have a problem but not yet a solution and want to discuss it, whereas RFC is when you have both a problem (which may have been agreed upon in an issue) AND a proposed solution.

[toumorokoshi]

Sounds good. I can move that to an RFC form.

[yurishkuro]

It seems like you have a certain solution in mind, why not describe it in the RFC? Clearly, this is a question that would need to be answered.

[toumorokoshi]

I don't have it well thought out, but I'm happy to think through and amend with proposed behavior as a starting point.

[toumorokoshi]

As an aside, I think this will also help the creation of a propagators object that handles extraction from formatters and injecting into context. Here's the ticket in otel-python: open-telemetry/opentelemetry-python#51

[toumorokoshi]

this PR (open-telemetry/opentelemetry-python#89) now accurately represents what I'm proposing.

[tedsuo]

This relates to the context propagation discussions last week; looking into where they dovetail and where they diverge:
https://docs.google.com/document/d/1rgXZik2_-2fU1KpVi7nze5ygWA8NhaLE8VmD20H4P8M/edit#heading=h.o5slz8bi8org

[Oberon00]

I made a few comments to open-telemetry/opentelemetry-python#89 and open-telemetry/opentelemetry-python#121 one of which I think is useful for this discussion: With this design, a context is now used in some scenarios where it was not used previously. One use-case that seems poorly supported by this is using "explicit context propagation" (IIRC that was the name), i.e. not setting a span as active but explicitly passing the parent span to Tracer.create_span. In that case a new temporary context would need to be created in order to inject the span context into a carrier. It seems that a temporary context is needed in all cases when extracting (the extracted span context should not become current context -- it might never become a parent span, maybe it will only be added as a link.

This concern (and others, see comments in the mentioned PRs) becomes even stronger when you remove the Unified Context layer proposed here , as the current design at https://docs.google.com/document/d/1fxnfChmZauNlUJ7zqGs31x7gH70s9s_q1BWpvYVHNqA/edit (is this the most recent one?). The Unified context could be implemented as a lightweight interface that provides controlled, type-safe access. The typical runtime-context has type-unsafe accessors and no support for validating values passed to set (e.g. what if the current span context was set to null instead of INVALID_CONTEXT by a buggy extractor?).

[yurishkuro]

@Oberon00 using generic context mechanisms like io.grpc.Context (Java) or context.Context (Go) is orthogonal to how those contexts are passed around. These contexts already are "unified contexts".

[Oberon00]

@yurishkuro Maybe I'm misunderstanding something but, yes, I agree with you that which context mechanism is used and how contexts are passed around are orthogonal things. You can consider the concern that generic contexts are type-unsafe and the concern that full contexts are now passed around where they were previously not, as orthogonal concerns.

[toumorokoshi]

@yurishkuro @Oberon00 at this point I've been contributing my ideas into @tedsuo's new OTEP here: #66. Please leave any further comments on there, although my cursory read is that it's more aligned with what both of you were hoping for.