Skip to content

feat: replace cgi with uri for encode and decode #2028

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.

Sign up for GitHub

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

Open
xuan-cao-swi wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: replace cgi with uri for encode and decode #2028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a4643e
fix!: replace cgi
xuan-cao-swi Jan 15, 2026
c87ca7d
update test assert for new rfc spec
xuan-cao-swi Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions api/lib/opentelemetry/baggage/propagation/text_map_propagator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# SPDX-License-Identifier: Apache-2.0

require 'cgi'
Copy link
Contributor

@kaylareopelle kaylareopelle Jan 30, 2026

Choose a reason for hiding this comment

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

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'
end

Copy link
Contributor

@kaylareopelle kaylareopelle Jan 30, 2026

Choose a reason for hiding this comment

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

ah, I just saw the conversation on #2016. It sounds like URI is compatible with the spec.

Copy link
Contributor Author

@xuan-cao-swi xuan-cao-swi Feb 4, 2026

Choose a reason for hiding this comment

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

Yes, other language use their stdlib to decode

require 'uri'

module OpenTelemetry
module Baggage
Expand Down Expand Up @@ -63,7 +63,7 @@ def extract(carrier, context: Context.current, getter: Context::Propagation.text
# the W3C spec where it's referred to as properties. We preserve
# the properties (as-is) so that they can be propagated elsewhere.
kv, meta = entry.split(';', 2)
k, v = kv.split('=').map!(&CGI.method(:unescape))
k, v = kv.split('=').map!(&URI.method(:decode_uri_component))
builder.set_value(k, v, metadata: meta)
end
end
Expand Down Expand Up @@ -99,7 +99,7 @@ def encode(baggage)
end

def encode_value(key, entry)
result = +"#{CGI.escape(key.to_s)}=#{CGI.escape(entry.value.to_s)}"
result = +"#{URI.encode_uri_component(key.to_s)}=#{URI.encode_uri_component(entry.value.to_s)}"
# We preserve metadata received on extract and assume it's already formatted
# for transport. It's sent as-is without further processing.
result << ";#{entry.metadata}" if entry.metadata
Expand Down
2 changes: 1 addition & 1 deletion exporter/otlp-http/lib/opentelemetry/exporter/otlp/http/trace_exporter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ def parse_headers(raw)
raise ArgumentError, ERROR_MESSAGE_INVALID_HEADERS if entries.empty?

entries.each_with_object({}) do |entry, headers|
k, v = entry.split('=', 2).map(&CGI.method(:unescape))
k, v = entry.split('=', 2).map(&URI.method(:decode_uri_component))
begin
k = k.to_s.strip
v = v.to_s.strip
Expand Down
2 changes: 1 addition & 1 deletion exporter/otlp-logs/lib/opentelemetry/exporter/otlp/logs/logs_exporter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ def parse_headers(raw)
raise ArgumentError, ERROR_MESSAGE_INVALID_HEADERS if entries.empty?

entries.each_with_object({}) do |entry, headers|
k, v = entry.split('=', 2).map(&CGI.method(:unescape))
k, v = entry.split('=', 2).map(&URI.method(:decode_uri_component))
begin
k = k.to_s.strip
v = v.to_s.strip
Expand Down
2 changes: 1 addition & 1 deletion exporter/otlp-metrics/lib/opentelemetry/exporter/otlp/metrics/util.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def parse_headers(raw)
raise ArgumentError, ERROR_MESSAGE_INVALID_HEADERS if entries.empty?

entries.each_with_object({}) do |entry, headers|
k, v = entry.split('=', 2).map(&CGI.method(:unescape))
k, v = entry.split('=', 2).map(&URI.method(:decode_uri_component))
begin
k = k.to_s.strip
v = v.to_s.strip
Expand Down
2 changes: 1 addition & 1 deletion exporter/otlp/lib/opentelemetry/exporter/otlp/exporter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ def parse_headers(raw)
raise ArgumentError, ERROR_MESSAGE_INVALID_HEADERS if entries.empty?

entries.each_with_object({}) do |entry, headers|
k, v = entry.split('=', 2).map(&CGI.method(:unescape))
k, v = entry.split('=', 2).map(&URI.method(:decode_uri_component))
begin
k = k.to_s.strip
v = v.to_s.strip
Expand Down
6 changes: 3 additions & 3 deletions propagator/jaeger/lib/opentelemetry/propagator/jaeger/text_map_propagator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# SPDX-License-Identifier: Apache-2.0

require 'cgi'
require 'uri'

# OpenTelemetry is an open source observability framework, providing a
# general-purpose API, SDK, and related tools required for the instrumentation
Expand Down Expand Up @@ -75,7 +75,7 @@ def inject(carrier, context: Context.current, setter: Context::Propagation.text_
setter.set(carrier, IDENTITY_KEY, trace_span_identity_value)
OpenTelemetry::Baggage.values(context: context).each do |key, value|
baggage_key = 'uberctx-' + key
encoded_value = CGI.escape(value)
encoded_value = URI.encode_uri_component(value)
setter.set(carrier, baggage_key, encoded_value)
end
carrier
Expand Down Expand Up @@ -110,7 +110,7 @@ def context_with_extracted_baggage(carrier, context, getter)
next unless baggage_key

raw_value = getter.get(carrier, carrier_key)
value = CGI.unescape(raw_value)
value = URI.decode_uri_component(raw_value)
Copy link
Contributor

@arielvalentin arielvalentin Jan 20, 2026

Choose a reason for hiding this comment

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

Do the same changes need to happen for B3 headers?

Copy link
Contributor Author

@xuan-cao-swi xuan-cao-swi Jan 20, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@arielvalentin arielvalentin Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Am I missing something? Then where these baggage headers encoded incorrectly when using the b3 propagators?

Copy link
Contributor Author

@xuan-cao-swi xuan-cao-swi Feb 4, 2026

Choose a reason for hiding this comment

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

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

b.set_value(baggage_key, value)
end
end
Expand Down
2 changes: 1 addition & 1 deletion propagator/jaeger/test/text_map_propagator_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ def create_context(trace_id:,
end
carrier = {}
propagator.inject(carrier, context: context)
_(carrier['uberctx-key1']).must_equal('value+1+%2F+blah')
_(carrier['uberctx-key1']).must_equal('value%201%20%2F%20blah')
end

it 'injects to rack keys' do
Expand Down