Initial proposal for binary protocol

[SergeyKanzhelev]

This PR was originally submitted here: w3c/trace-context#215

I moved discussions here.

CC: @watson @yurishkuro @bogdandrutu

[SergeyKanzhelev]

Comments from original PR:

From @yurishkuro:

Why not define the binary format using protobuf IDL instead of inventing some custom encoding?

Reply from @SergeyKanzhelev:

Why not define the binary format using protobuf IDL instead of inventing some custom encoding?

I got a recommendation to not define it as protobuf as it's proprietary protocol/tech. That's said proposed format is basically how it will look in protobuf.

Reply from @yurishkuro

I got a recommendation to not define it as protobuf as it's proprietary protocol/tech. That's said proposed format is basically how it will look in protobuf.

You just got another recommendation :-) Should we have a vote then?

There are many binary format specs out there. I think it would be best if we pick one rather than come up with a custom n+1 spec. There's nothing wrong with having the format defined by a widely used spec imo, it does not force the implementation in any way.

Reply from @SergeyKanzhelev

You just got another recommendation :-) Should we have a vote then?

Before voting - do you agree that we should provide binary encoding rules that basically copies proto algorithm as not everybody can take dependency on proto?

Also I tried to do it with proto and it feels like an overkill. For instance, instead of byte length that is easy to read we will get varint with complex algorithm of serializing/deserializing.

What other binary format specs you can suggest?

[SergeyKanzhelev]

@yurishkuro I understand your biggest concern was about custom format vs. some standard protocol like protobuf. Do you still have this concern?

I believe I addressed all the rest of your comments.

[yurishkuro]

Another meta-question: what are the use cases that require binary protocol?

[SergeyKanzhelev]

Another meta-question: what are the use cases that require binary protocol?

Primarily GRPC and AMQP. Do you have an idea how to eliminate the need for binary?

[yurishkuro]

Primarily GRPC and AMQP

Afaik gRPC works fine with text format, it does not require binary. Don't know about AMQP, doesn't it also support "headers" as key/value pairs?

The only time binary format is needed is in transports that only support a binary blob specifically reserved for tracing, which is quite rare (why single out tracing out of all metadata?). Even in those rare cases we don't explicitly need a new binary format, we can simply use HTTP encoding for headers, i.e. a multi-line utf-8 string

traceparent: ... \n
tracestate: ... \n

encoded as bytes. Parsing this blob into map[string]string is trivial, and after that the normal trace context parsing can be used.

[SergeyKanzhelev]

@yurishkuro can you please clarify. Are you advocating for NOT having binary or you are asking to provide a good rationale in the doc to have binary?

[yurishkuro]

I'm questioning if we need binary. OpenTracing always defined it as a standard format, but only one Jaeger client supports it (as a poc), yet we never had requests to implement it in other clients, which makes me suspicious of why it's needed.

[SergeyKanzhelev]

@yurishkuro we are already using it in Open Census for GRPC. Given the choice of text vs. binary in AMQP - I'm getting feedback that binary would be preferrable.

So we need the protocol. Next question - will we be better served by simply compressing text representatation or creating a new way to serialize. I think better way to serialize will address perf and size concers from people who needs binary in first place

[yurishkuro]

Fair enough. I think it's worth adding an explanation at the top of the document why this spec is needed, what problem it is meant to address. Otherwise the reader can ask "why should I care"?

[nikmd23]

From a technical perspective, I don't see any large problems. Before publishing we will probably want to do a pass to clean up grammar.

[discostu105]

A couple of notes:

Field identifiers in traceparent raise a couple of questions:

• Can the order of trace-id, parent-id, trace-flags be changed?
• Is it possible to leave some one of these identifiers out?
• Can I add my personal custom identifiers?
• Do I have to error-check a tag to assert the identifiers are correct or should I just ignore them?

In text-format of TraceContext, none one these fields are really optional, even in future versions we defined to only "expand". What's the benefit of field identifiers, vs. defining fixed-size fields in specified order?

tracestate ASCII entries: For our Dynatrace tracestate entry we would technically be able to generate a binary version too. We already use it today for tracing in various binary protocols and it has size/perf benefits. When using binary TraceContext we would be forced to use the ASCII version of our tag. Of course that's possible, but we'd lose the size/perf benefits. This is a tricky one to solve though. If a tracer needs to convert TraceContext with foreign tracestate entries from binary to text or vice-versa, it needs to know how to do that. We'd need to add a type (text or binary) to the tracestate entry.

[discostu105]

Concerning validity of binary-spec: We use binary tagging for a couple of technologies, but GRPC and AMQP are not among them.

GRPC: If using GRPC Metadata, AFAIK it only supports strings. Blobs can be used, but will be base64 encoded. (edit: not entirely true, it depends. If HTTP2 is used, blobs are base64 encoded, but otherwise true binary is used) https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md#storing-binary-data-in-metadata

AMQP: We use a string header on the message property. Not entirely sure, maybe binary header-values are possible in amqp.

Some examples where we use binary: Service Fabric Remoting, RMI, Akka, Kafka, CTG, .NET Remoting

[bogdandrutu]

Propose to have only one binary value for the encoding result, by unifying traceparent and tracestate.

{0,  // version
  0 /* id */ , 75, 249, 47, 53, 119, 179, 77, 166, 163, 206, 146, 157, 0, 14, 71, 54,  // trace-id
  1 /* id */, 52, 240, 103, 170, 11, 169, 2, 183,  // span-id
  2 /* id */, 1,  // trace-options
  3 /* id */, ...  // tracestate (probably prefixed with the len to allow easy parsing)
}

This will simplify integrations with grpc or other binary protocols. Also from the tracing library perspective this will simplify the propagation framework (accepts byte[] and returns byte[]).

[SergeyKanzhelev]

There are too many conversations on a single PR. I'm merging PR as an editor draft and will create issues for every comment to discuss