Remove binary format propagator

[MikeGoldsmith]

The Propagators API includes both binary and text propagators, however the binary format is not well defined and is without an agreed protocol. This has lead many languages to simply have a Noop implementation.

The W3C has binary protocols for different transports in progress but they have not been formally accept yet. It make senses to remove the binary propagator from the Open Telemetry spec for now and re-add once a protocol has been accepted.

In-progress W3C binary protocol proposals:
Trace Context Binary Spec: https://github.com/w3c/trace-context-binary
Trace Context AMQP: https://github.com/w3c/trace-context-amqp
Trace Context MQTT: https://github.com/w3c/trace-context-mqtt

[bogdandrutu]

This cannot be removed unless a backwards compatibility story is defined, we need it to be compatible with OpenCensus and gRPC integration.

[MikeGoldsmith]

Currently there is not a binary encoding defined in the spec so we're unlikely to have consistency across implementations and are stuck stuck creating injectors & extractors for an unknown format that we can't test. This PR doesn't propose to remove it permanently, just until we have an agreed encoding format in the spec - this could include the reasoning for backward compatibility with OpenCensus.

Couple of follow-up questions:

• Can OpenCensus convert from/to the proposed W3C traceparent context formats?
• What do you mean by gRPC integration? (My understanding is that the OTel-Go gRPC plugin just uses key-value pairs and not a binary encoding)

[MikeGoldsmith]

@bogdandrutu If we don't want to remove BinaryFormatter, we need instead properly define the spec & protocol (which could be the W3C implementation).

My concern is leaving it in this state where we have an API definition without a SDK implementation.

[carlosalberto]

@open-telemetry/specs-approvers Please review/approve this PR.

@bogdandrutu whenever you have a few free cycles, please reply to @MikeGoldsmith's latest notes ;)

[dyladan]

Opentelemetry JS is using the binary propagator from OpenCensus for gRPC. Should we be switching to use the text Trace Context propagator for that?

[carlosalberto]

Hey hey @dyladan

Oh interesting to know that. I think that the majority of languages/implementations were not using binary at all (for now).

I'd suggest to use the text format for now, while the binary support/format is updated ;)

[carlosalberto]

@bogdandrutu Was wondering if you would have enough time to champion an update for the Binary format yourself? Even if it's a prototype we can start from.

(Disclaimer: I think/feel most of the OTel community are not in a real need in Binary format right now, and thus I'd think we can get forget it for the time being, and focus on it on the 0.4.0 milestone)

[dyladan]

@markwolff since you wrote the grpc plugin, can you please weigh in on how difficult it would be to transition to text propagation?

[markwolff]

@markwolff since you wrote the grpc plugin, can you please weigh in on how difficult it would be to transition to text propagation?

It should be straightforward and only require a oneliner change on which formatter the plugin is using.

[Oberon00]

@markwolff @dyladan: How did this work when a grpc call was made to a language other than JS?

[dyladan]

@markwolff @dyladan: How did this work when a grpc call was made to a language other than JS?

It didn't. This brings up a good point that maybe we should have some document in the spec that says how we map http header propagation formats to various transports like gRPC.

[Flarna]

It didn't. This brings up a good point that maybe we should have some document in the spec that says how we map http header propagation formats to various transports like gRPC.

Isn't this the target of the various w3c standards mentioned in the beginning of this issue?

[dyladan]

It didn't. This brings up a good point that maybe we should have some document in the spec that says how we map http header propagation formats to various transports like gRPC.

Isn't this the target of the various w3c standards mentioned in the beginning of this issue?

Yes, but those standards don't always specify which one should be used. gRPC being a good example, do you use binary or text?

[carlosalberto]

@bogdandrutu I created #437 to make sure we re-add Binary support after 0.3.0 (initially assigned to @MikeGoldsmith). If this is ok with you, would you mind resolving your changes request?

[iredelmeier]

IME, the "HTTP" propagator is still usable with, e.g., gRPC and Kafka; that is, these and other transports typically expose something along the lines of a basic key/value map that can be used with the W3C headers. These may later be transformed into a string or byte array or whatever else - but that implementation detail isn't necessarily something we need to be concerned with.

Some transports have specific requirements around, e.g., capitalization or special characters; however, I don't believe that handling these requirements will be any different with or without the binary propagator. Indeed, some seemingly basic HTTP libraries have requirements like this (shaking my fist at you, Rack).

[dyladan]

@iredelmeier it was my understanding that the binary propagator exists not because the text version is impossible, but because it can be much more efficient. A trace context header value is 55 characters, but only represents 26 bytes of data. Including field identifiers, the binary TC spec (proposed) uses 29 bytes.

Another issue is that OpenCensus uses binary trace context for gRPC calls, so we will lose interop with OpenCensus instrumented services, which could potentially make adoption more difficult for existing OC users.

[Oberon00]

Also, how should the OpenCensus and OpenTracing bridges be implemented without a binary propagator?

[carlosalberto]

Also, how should the OpenCensus and OpenTracing bridges be implemented without a binary propagator?

We will put it back in 0.4.0 ;)

If you are asking about right now, I don't think this is supported in the majority of the OT compatibility layers, and I don't think we have OC adapters yet.

[MikeGoldsmith]

Yeah, just want to confirm this PR is to temporarily remove the binary propagator from the spec as we do not have an agreed on protocol. This tightens up the 0.3 spec so that SDKs do not implement inconsistently.

An issue for 0.4 has already been created which will re-introduce the binary protocol with defined protocol.

[dyladan]

Also, how should the OpenCensus and OpenTracing bridges be implemented without a binary propagator?

We will put it back in 0.4.0 ;)

If you are asking about right now, I don't think this is supported in the majority of the OT compatibility layers, and I don't think we have OC adapters yet.

As a maintainer of a SIG which has implemented an OT shim, we cannot remove the binary propagator, but I feel its ok to remove from the spec as long as it's temporary. We can remove it from the API and just keep it SDK only for now until the binary prop is brought back. cc @mayurkale22 WDYT?

[MikeGoldsmith]

Agreed - I don't think it's worthwhile for existing implementations to be removed. This is a spec level only change until we have a properly defined binary protocol that we can implement consistently.

[carlosalberto]

As a maintainer of a SIG which has implemented an OT shim, we cannot remove the binary propagator, but I feel its ok to remove from the spec as long as it's temporary.

Giving my two cents: yes, that sounds about right @dyladan :)

[bogdandrutu]

I am fine to remove temporary until the interface is re-designed.