Properly encode baggage values and metadata#3740
Conversation
And, ensure we encode baggage metadata as well as the baggage value.
25e622a to
22de59e
Compare
Codecov Report
@@ Coverage Diff @@
## main #3740 +/- ##
==========================================
Coverage 89.24% 89.24%
- Complexity 3839 3873 +34
==========================================
Files 462 463 +1
Lines 11969 12109 +140
Branches 1163 1188 +25
==========================================
+ Hits 10682 10807 +125
- Misses 905 910 +5
- Partials 382 392 +10
Continue to review full report at Codecov.
|
| */ | ||
| private static boolean isValueValid(String value) { | ||
| return value != null && StringUtils.isPrintableString(value); | ||
| return value != null; |
There was a problem hiding this comment.
I think removing the isPrintableString check in this class and filtering in the propagator is more consistent with the specs. But can be another PR
There was a problem hiding this comment.
I wonder if the filtering is actually something that is even needed, given the encoding requirements. The otel spec doesn't mention anything about filtering, and I suspect that the W3C spec is just defining the wire format, not the input requirements. So, in the wire format, you URL-encode to make sure that you aren't using any non-ascii, delimiter characters, but the inputs can be whatever you want. At least, I think that's what's intended, even if it's quite poorly worded in the W3C spec.
There was a problem hiding this comment.
Or were you thinking about the key validation? That's another bit that I wish was better understood.
| import org.junit.runner.RunWith; | ||
|
|
||
| class PercentEscaperFuzzTest { | ||
| @RunWith(JQF.class) |
There was a problem hiding this comment.
Wonder if RunWith is needed given the manual driving. No big deal
There was a problem hiding this comment.
By the way if this feels too hacky, maybe can use easy-random + @RepeatedTest like I used for the random span stuff. It's pretty trivial to get a random string. Don't know if Fuzz does anything cool besides random though.
https://github.com/j-easy/easy-random
https://github.com/j-easy/easy-random/blob/master/easy-random-core/src/main/java/org/jeasy/random/randomizers/text/StringRandomizer.java
There was a problem hiding this comment.
I checked and @RunWith is needed. JQF does have some other interesting features that we could start leveraging in the future, but agreed that maybe we could use something else. In another PR. :)
This PR does several things:
Resolves #3442