Open yurishkuro opened 1 year ago
cc @MadVikingGod
The change was introduced in #3226. It did not provide a good justification why it needs to be this way. Referencing W3C doesn't make sense since it does not dictate in-memory representation, only wire format. From an API design of baggage, it makes more sense to allow the users to use any strings they want, and just take care of encoding them on the wire.
If people disagree (I can see a performance argument for encoding), then the API must be clearly documented to require URL-encoded values. Then the OpenTracing Bridge could be fixed to encode them when passing back to OTEL Baggage.
Looking more into this, I would certainly advocate for reverting #3226. It introduced weird asymmetry in the baggage.Member
type where its constructor requires URL-encoded value, but member.Value()
function returns raw value (because it's un-encoded in the constructor). So the performance argument I mentioned above doesn't even apply.
Another issue: in #2529 a decoding was added to parseMember, right before the value is checked against regexp to validate that it is "safe" per W3C. After #2529 it's the decoded value that's checked, which is incorrect, and valid strings now fail (the unit test that was added kind-of cheated by making decoded string look like URL-encoded)
@MrAlias can you shed some light on the original motivation?
From my point of view, this would be the ideal implementation:
NewMember
, and the same string must be returned from Member.Value()
Unfortunately, switching to this approach at this time is a breaking change. Fortunately, it's going to be breaking for already broken state, which was created after #3226 / #2529.
https://github.com/open-telemetry/opentelemetry-go/pull/4804 was applied to fix OpenTracing Bridge and add functions that are not entangled with W3C Propagator.
I am not closing the issue as we can still consider deprecating all the functions and methods that are entangled with W3C Propagator. Reference: https://github.com/open-telemetry/opentelemetry-go/pull/4804#issuecomment-1876601271
Description
As a user of Baggage, whether it's via OTEL Baggage API or OpenTracing Bridge, the value of a baggage entry is not supposed to be restricted, i.e. the following should be valid code:
However, the
NewMember
function invokesurl.QueryUnescape(value)
and fails if that returns an error. And even if I URL-escape the value first, when baggage is used via OpenTracing Bridge, at some point the sameNewMember
function is called with unescaped value and fails again.I think this is the wrong behavior. There may be reasons to store the value internally as url-encoded so that serialization is more efficient (since baggage is generally accessed less frequently than it is passed around and serialized). However, that is the internal implementation detail, which is between Baggage and Propagator, it should not be spilling out to the API that the user is exposed to. A user should only deal with unencoded strings that makes sense in the application.
Unit test tat illustrates the issue: #3689
This seems similar to the issue #2523.
cc @williamgcampbell @NewReStarter