Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Allow common base64 encoding in tracestate values #551

Open
feldentm-SAP opened this issue Nov 2, 2023 · 5 comments
Open

Proposal: Allow common base64 encoding in tracestate values #551

feldentm-SAP opened this issue Nov 2, 2023 · 5 comments

Comments

@feldentm-SAP
Copy link

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
Copy link
Member

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

@feldentm-SAP
Copy link
Author

feldentm-SAP commented Nov 2, 2023

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
Copy link
Contributor

basti1302 commented Dec 5, 2023

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
Copy link
Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants