w3c / trace-context

Trace Context
https://w3c.github.io/trace-context/
Other
469 stars 76 forks source link

Proposal: Allow common base64 encoding in tracestate values #551

Open feldentm-SAP opened 11 months ago

feldentm-SAP commented 11 months ago

Problem

SAP would like to encode context information from existing widely-used correlators into tracestate header parts. These correlators use data that is a structured record of raw binary data. We tried to use a compact representation of required data, but had to learn that we cannot encode them in common base64 encodings. Using hex encoding instead would be unpleasant because we would exceed the tracestate length limits.

Proposal in tracestate Values

In 3.3.2.2.2 Value we would need to remove the except '=' clause to allow the '=' padding characters used by common base64 encodings.

Note: This would reflect the rules currently used in the baggage standard: https://www.w3.org/TR/baggage/#value

yurishkuro commented 11 months ago

Padding with = is not required by base64 and can be skipped in many libraries.

feldentm-SAP commented 11 months ago

Some fields have variable length, hence, it is required.

EDIT: I think both sides used the wrong arguments here. The point against the current tracestate definition is that it encouraged outright idiotic implementation of string handling. The point I should have made is that some implementation rely on the padding which is why we would send it and not assume implementation to insert correct padding. In this situation, we would be able to identify the correct payload length and would, hence, be able to decode it. However, we'd like to do it with the smallest possible friction and having services implement proper decoding might be an issue, here.

basti1302 commented 10 months ago

I don't see any possibility to make this change in a backwards compatible way. Yes, maybe not disallowing the = character in tracestate values would have been better. But this ship has sailed now. Changing it now might break existing parsers. The only option would be to move to a different header name (like tracestate2 or similar). In my personal opinion this is not worth it.

Edit: Let me add a real world scenario of a simple parsing strategy that would potentially break if we started to allow = in values. Let's say vendor foo is only interested in their own tracestate key-value pair, foo=.... One very simple way to get the tracestate value would be to scan the full tracestate header until you find foo=... followed by either , or end of string. Implementations like this likely exist today. This parser would break if we allowed = and some other vendor by coincidence adds a key-value pair like bar=abcdefoo=ghi.

feldentm-SAP commented 10 months ago

And you really consider such processing valid? I would have expected that implementations either handle the header transparently or view it as a data structure parsing it correctly with code that deserves to be called parser. Just looking at obscure ways to use something is never a good guidance. Clarity and symmetry are.

Seriously, such implementation would fail anyway if they start using baggage.

SergeyKanzhelev commented 10 months ago

it correctly with code that deserves to be called parser... Seriously, such implementation would fail anyway if they start using baggage.

They will likely use different parser for baggage as it also supports unicode for example. And those parsers are valid and we cannot disregard them.