-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: Member.String encodes only when necessary #4775
baggage: Member.String encodes only when necessary #4775
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4775 +/- ##
=====================================
Coverage 82.2% 82.2%
=====================================
Files 226 226
Lines 18344 18383 +39
=====================================
+ Hits 15087 15126 +39
Misses 2975 2975
Partials 282 282
|
Should you run benchstat to compare the benchmarks between main and your branch? |
I can do that today in a few hours or tomorrow morning. |
@dmathieu Benchstat results added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objection to this as an optimization of existing incorrect behavior - its not supposed to encode anything at all, only Propagator should be doing the encoding.
This was merged without two approvals from the Go Approvals team. |
@MrAlias Sorry, I have missed that @yurishkuro is not an approver 😬 |
Fixes #4773
I also added Add UTF-8 test cases and some benchmarks.
Benchmark:
Benchstat (in
old.txt
the benchmarks useurl.PathEscape
instead ofvalueEscape
):The results of
String
andValueEscape
show improvement.