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

fix(baggage): Update baggage parsing and encoding in vendored otel package #568

Merged
merged 12 commits into from Feb 6, 2023

Conversation

tonyo
Copy link
Member

@tonyo tonyo commented Feb 3, 2023

These changes to baggage.go fix a few issues that are happening now in violation of the baggage spec (or at least my understanding of it):

  1. Baggage string value one%20two should be properly parsed as "one two"
  2. Baggage string value one+two should be parsed as "one+two"
  3. Go string value "one two" should be encoded as one%20two (percent encoding), and NOT as one+two (URL query encoding).
  4. Go string value "1=1" might be encoded as 1=1 (the spec says: "Note, value MAY contain any number of the equal sign (=) characters. Parsers MUST NOT assume that the equal sign is only used to separate key and value."). 1%3D1 is also valid, but to simplify the implementation we're not doing it.

Notes

  • The original baggage_test.go was copied from opentelemetry-go, commit 5e8eb855bf3c6507715edd12ded5c6a950dd6104.
  • Case number 2 is not fully fixed right now: baggage string value one+two will be parsed as "one two", and not as "one+two". To fix that, url.QueryUnescape() should be replaced by a function with the logic compatible with percentEncodeValue(), where the plus (and other allowed characters) are not treated as something special.

Fixes #554.

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Base: 74.72% // Head: 79.13% // Increases project coverage by +4.40% 🎉

Coverage data is based on head (9efd2cd) compared to base (ef3a838).
Patch coverage: 89.28% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #568      +/-   ##
==========================================
+ Coverage   74.72%   79.13%   +4.40%     
==========================================
  Files          38       38              
  Lines        3838     3853      +15     
==========================================
+ Hits         2868     3049     +181     
+ Misses        850      703     -147     
+ Partials      120      101      -19     
Impacted Files Coverage Δ
internal/otel/baggage/baggage.go 97.54% <89.28%> (+53.49%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -12,6 +12,7 @@ require (
github.com/pingcap/errors v0.11.4
github.com/pkg/errors v0.9.1
github.com/sirupsen/logrus v1.9.0
github.com/stretchr/testify v1.8.0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testify is currently used only in baggage_test.go, and also used by a bunch of dependencies.

@tonyo tonyo changed the title fix(baggage): Use alternative implementations of baggage parsing and encoding fix(baggage): Update baggage parsing and encoding in vendored otel package Feb 3, 2023
@tonyo tonyo marked this pull request as ready for review February 3, 2023 14:58
@tonyo tonyo requested a review from cleptric February 3, 2023 14:58
@tonyo tonyo self-assigned this Feb 3, 2023
Copy link
Member

@cleptric cleptric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Maybe we can give it another try by opening this as a PR on the Otel repo.

@tonyo tonyo merged commit 3964ece into master Feb 6, 2023
@tonyo tonyo deleted the tonyo/otel-baggage-attempt-fix-1 branch February 6, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OTel] Baggage parsed incorrectly if it contains encoded whitespaces ("%20")
2 participants