feat: replace cgi with uri for encode and decode#2028
feat: replace cgi with uri for encode and decode#2028xuan-cao-swi wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
xuan-cao-swi
changed the title
xuan-cao-swi
requested review from
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd and
robertlaurin
as code owners
|
|
||
| raw_value = getter.get(carrier, carrier_key) | ||
| value = CGI.unescape(raw_value) | ||
| value = URI.decode_uri_component(raw_value) |
There was a problem hiding this comment.
Do the same changes need to happen for B3 headers?
There was a problem hiding this comment.
It seems B3 propagator headers should only contain trace ID, span ID, and sampling decision, which have well-defined headers that don't require decoding.
There was a problem hiding this comment.
Am I missing something? Then where these baggage headers encoded incorrectly when using the b3 propagators?
There was a problem hiding this comment.
It's not decode before, also I don't see any other language decode these key and value
I can create an issue to discuss about decode b3 baggage headers
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| require 'cgi' |
There was a problem hiding this comment.
Are the CGI methods we're calling in the propagators the same in 3.x and 4.x, but the only difference is what needs to be required?
To help me better understand the issue, would something like this also solve the problem?
if RUBY_VERSION >= '4.0'
require 'cgi/escape'
else
require 'cgi'
endThere was a problem hiding this comment.
ah, I just saw the conversation on #2016. It sounds like URI is compatible with the spec.
There was a problem hiding this comment.
Yes, other language use their stdlib to decode
arielvalentin
changed the title
3 participants
This PR tries to use
URI.encode/decode_uri_componentto replaceCGI.escape/unescape.Although
cgi/escapeis present in Ruby 4.0.0, it is not available in Ruby versions less than 4.0.0. We can use cgi/escape once all Ruby 3 versions are deprecated, if required.The difference between these two implementation is the treatment for asterisk (*), which CGI encode to
%2A, but URI preserve * (see more in #2016).CGI will silence the invalid char encode/decode, while URI will raise the error.