Proposing a single formatter for all contexts #37
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| # Single Formatters for all Contexts | ||
|
|
||
| **Status:** `proposed` | ||
|
|
||
| Defining a single formatter which (de)serializes all contexts, rather than separate ones. | ||
|
|
||
| ## Motivation | ||
|
|
||
| The existing propagators specification specifies that formatters should exist for each type of context (today this is SpanContext and DistributedContext). Requiring SDK authors to have to author one formatter per context type can result in error-prone and backwards-incompatible consumption and usage. | ||
|
|
||
| The error-prone aspect comes from the configuration of said formatters. Let's say that a user is setting up their propagators by hand: in order to do so they will need to configure both their SpanContext propagators and their DistributedContext propagators. If a user wants to use a SaaS product that requires both a custom SpanContext propagator as well as a DistributedContext propagator, they will have to know to attach both. If they forget to do so, they will end up with missing context that is hard to debug (since the user of a SaaS product would probably have little internal knowledge about how propagation should occur). | ||
|
|
||
| The backwards-incompatible changes come from the need to extract or inject context into new propagators. If a SaaS vendor or open source protocol previously had no need for on of the two propagators, but started to need it, they would need to notify all consumers that, as of this new version, the consumer must modify their code to add the additional formatter for the new context they require. This will probably come up often in SaaS offerings that previously only needed to propagate data in SpanContext, but then added their own fields that will then require custom DistributedContext formatters. | ||
|
|
||
| ## Explanation | ||
|
|
||
| The new proposed API schema looks like: | ||
|
|
||
| formatters (namespace) | ||
| UnifiedContext.class | ||
| BinaryAPI.class | ||
| HTTPTextFormat.class | ||
|
|
||
| With the following APIs: | ||
|
|
||
| ### UnifiedContext | ||
|
|
||
| UnifiedContext will be a composite object containing all contexts that are present in the OpenTelemetry implementation. As of the authoring of this RFC, this object will contain a DistributedContext and SpanContext. | ||
|
|
||
| ### BinaryAPI | ||
|
|
||
| The BinaryAPI will support two methods: | ||
|
|
||
| * fromBytes: convert raw bytes to a UnifiedContext or Null | ||
| * toBytes: convert a UnifiedContext to raw bytes | ||
|
|
||
| ### HTTPTextFormat | ||
|
|
||
| The HTTPTextFormat will support two methods: | ||
|
|
||
| * extract: from a carrier object and getter, return a UnifiedContext | ||
| * inject: from a UnifiedContext, a carrier object, and a setter, return Null. populate the carrier object with values from the UnifiedContext. | ||
|
|
||
| ## Internal details | ||
|
|
||
| This will require medium-sized refactoring on existing propagators and integrations. | ||
|
|
||
| * integrations will have to switch to extracting the SpanContext and DistributedContext from the UnifiedContext. | ||
| * all existing HTTPTextFormat and BinaryAPI APIs and implementations would have to refactored to match the unified HTTPTextFormat and BinaryAPIs. | ||
|
|
||
| An example of the UnifiedContext at work (although not following the code organization above) exists at: | ||
| * PR: https://github.com/open-telemetry/opentelemetry-python/pull/89/files | ||
| * file: https://github.com/open-telemetry/opentelemetry-python/blob/f1f8bb884fdcfe4ac257225e1636e1960495a51c/opentelemetry-api/src/opentelemetry/context/unified_context.py | ||
|
|
||
|
|
||
| ## Trade-offs and mitigations | ||
|
|
||
| One drawback is this makes error cases a bit more ambiguous. For example, what happens if a format can extract a span properly, but not a distributed context? or visa versa? | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. Sounds good. I can move that to an RFC form. |
||
|
|
||
| This could be mitigated by defining some clear behavior around these cases. Also the current choice left to formatters is to return a full object, partial object, or none. The same could be provided here and it would be up to the consumer to error handle (although that again is prone to errors and different expectations). | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. I don't have it well thought out, but I'm happy to think through and amend with proposed behavior as a starting point. |
||
|
|
||
| ## Open questions | ||
|
|
||
| Deeper and partial error handling as descrbied in trade-offs. For example, what if a formatter succeeds in deserializing a DistributedContext, but not a SpanContext? | ||
|
|
||
| ## Future possibilities | ||
|
|
||
| This enables seamless integration of additional context objects, if that becomes a necessity. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context object is not part of the wire propagators layer. E.g.
context.Contextin Go orio.grpc.Contextin Java are the containers of all types of contexts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't great from a terminology standpoint, but I think there's a distinction between:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.