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

Baggage API is not working, entangled with W3C Propagator #3685

Open
yurishkuro opened this issue Feb 7, 2023 · 6 comments · Fixed by #4804 · May be fixed by #3689
Open

Baggage API is not working, entangled with W3C Propagator #3685

yurishkuro opened this issue Feb 7, 2023 · 6 comments · Fixed by #4804 · May be fixed by #3689
Labels
area:baggage Part of OpenTelemetry baggage bug Something isn't working help wanted Extra attention is needed pkg:bridges Related to a bridge package

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Feb 7, 2023

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:

// via OpenTracing Bridge
span.SetBaggageItem("someKey", "any value I want /#%")

// via baggage package
mem, err := baggage.NewMember("someKey", "any value I want /#%")
require.NoError(t, err)
bag.SetMember(mem)

However, the NewMember function invokes url.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 same NewMember 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

@yurishkuro yurishkuro added the bug Something isn't working label Feb 7, 2023
@yurishkuro
Copy link
Member Author

cc @MadVikingGod

@yurishkuro
Copy link
Member Author

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.

@yurishkuro
Copy link
Member Author

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.

@yurishkuro yurishkuro linked a pull request Feb 7, 2023 that will close this issue
@yurishkuro
Copy link
Member Author

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)

@yurishkuro
Copy link
Member Author

@MrAlias can you shed some light on the original motivation?

From my point of view, this would be the ideal implementation:

  • Users of the API are not exposed to URL-encoding, meaning any string is valid to pass to NewMember, and the same string must be returned from Member.Value()
    • Motivation: baggage API is a general-purpose context propagation mechanism. It is supposed to be completely de-coupled from the W3C spec. The latter is only concerned with wire representation, and there are other possible wire representations, such as binary encoding proposed in the original paper from Jonathan Mace that introduced the term "baggage".
  • The internal storage of key/values in Member struct should also be raw, unencoded strings.
    • There is a possible optimization where the baggage propagators pass an accessor function to Member instead of the actual decoded values, so that if the value is never read in a service, it never needs to be decoded.
  • All URL encoding/decoding should move to the baggage propagator code, including the regex checks.
    • I am not 100% those checks are even needed since the encoding/decoding functions from stdlib presumably already ensure that the encoded strings satisfy the desired character set.

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.

@yurishkuro yurishkuro reopened this Feb 9, 2023
shubbham1215 pushed a commit to shubbham1215/jaeger that referenced this issue Feb 22, 2023
## Which problem is this PR solving?
- The baggage propagation was broken in HotROD after jaegertracing#4187
- This restores the baggage support by providing a few integration
points with OTEL
- [ ] This change will not work until OTEL Baggage & Bridge bugs are
fixed: open-telemetry/opentelemetry-go#3685

## Short description of the changes
- [`index.html`] The webapp used to send baggage via `jaeger-baggage`
header, this is now changed to W3C `baggage` header
- [`mux.go`] The Jaeger SDK was able to accept `jaeger-baggage` header
even for requests without am active trace. OTEL Bridge does not support
that, so we use OTEL's Baggage propagator to manually extract the
baggage into Context, and once the Bridge creates a Span, we copy OTEL
baggage from Context into the Span.
- All other changes are cosmetic / logging related

---------

Signed-off-by: Yuri Shkuro <[email protected]>
shubbham1215 pushed a commit to shubbham1215/jaeger that referenced this issue Mar 5, 2023
## Which problem is this PR solving?
- The baggage propagation was broken in HotROD after jaegertracing#4187
- This restores the baggage support by providing a few integration
points with OTEL
- [ ] This change will not work until OTEL Baggage & Bridge bugs are
fixed: open-telemetry/opentelemetry-go#3685

## Short description of the changes
- [`index.html`] The webapp used to send baggage via `jaeger-baggage`
header, this is now changed to W3C `baggage` header
- [`mux.go`] The Jaeger SDK was able to accept `jaeger-baggage` header
even for requests without am active trace. OTEL Bridge does not support
that, so we use OTEL's Baggage propagator to manually extract the
baggage into Context, and once the Bridge creates a Span, we copy OTEL
baggage from Context into the Span.
- All other changes are cosmetic / logging related

---------

Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: shubbham1215 <[email protected]>
@pellared pellared added pkg:bridges Related to a bridge package area:baggage Part of OpenTelemetry baggage labels Oct 24, 2023
@yurishkuro yurishkuro changed the title Baggage not working with OpenTracing Bridge Baggage API is not working, entangled with W3C Propagator Dec 21, 2023
@pellared pellared self-assigned this Dec 29, 2023
@pellared pellared reopened this Jan 10, 2024
@pellared
Copy link
Member

#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: #4804 (comment)

@pellared pellared removed their assignment Jan 10, 2024
@pellared pellared added the good first issue Good for newcomers label Jan 10, 2024
@pellared pellared added help wanted Extra attention is needed and removed good first issue Good for newcomers labels Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:baggage Part of OpenTelemetry baggage bug Something isn't working help wanted Extra attention is needed pkg:bridges Related to a bridge package
Projects
None yet
2 participants