Skip to content

Conversation

@xuan-cao-swi
Copy link
Contributor

Added cgi and ostruct gem since they are removed from ruby 4.
Also fixed some warning that is about constants reinitialization.

@arielvalentin
Copy link
Contributor

@xuan-cao-swi I know this is not ready for review but I think that cgi is a runtime dependency of the API and SDK:

https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-ruby%20cgi&type=code

If we do not want to add a transitive dependency on the cgi gem then we should find a stdlib alternative to CGI.unescape or CGI.escape.

@arielvalentin
Copy link
Contributor

ostruct on the other hand is a test only dependency:

If we can find an alternative for OpenStruct I think that we should go for it.

@xuan-cao-swi
Copy link
Contributor Author

For replacement of cgi escape and unescape maybe using URI.decode_www_form_component/encode_www_form_component?

I just did test and found out if rake is 13.3, then no OpenStruct is required. I think old rake (13.1.0 and before) use OpenStruct. Currently most gemspec using rake ~> 12.0 (except log_sdk), maybe they can change to newer rake.

@xuan-cao-swi
Copy link
Contributor Author

xuan-cao-swi commented Jan 13, 2026

Updated all rake to 13.3 for ruby 4.
Using URI to replace CGI for url encoding/decoding.
Avoid use openstruct for test case.

@xuan-cao-swi xuan-cao-swi changed the title test: add cgi and ostruct in gemspec dev dependency test: remove cgi and ostruct dependency for ruby 4 Jan 13, 2026
@arielvalentin
Copy link
Contributor

I do not know if they are functionally equivalent, e.g. of en

irb(main):006> CGI.escape("*")
=> "%2A"
irb(main):007> URI.encode_www_form_component("*")
=> "*"
irb(main):008> URI.decode_www_form_component("%2A")
=> "*"
irb(main):010> CGI.unescape("*")
=> "*"

@xuan-cao-swi
Copy link
Contributor Author

It seems URI encode/decode is for newer URL encoding rules (RFC 3986), and CGI is for legacy rules.

Decode seems aligned for asterisk.

irb(main):011> URI.decode_www_form_component("%2A")
=> "*"
irb(main):014> CGI.unescape("%2A")
=> "*"

If we are ok with adding cgi in dependency then I can revert back the changes.
I don't see any other stdlib have uri encode and decode support.
We can also copy over the function from cgi library and integrate them in sdk/exporter codebase.

@arielvalentin
Copy link
Contributor

I think what we should do is consult the specification.

Does it specify which to use?

In inclined to keep this change but report it as potentially a breaking change.

@xuan-cao-swi
Copy link
Contributor Author

xuan-cao-swi commented Jan 14, 2026

Based on the spec, baggage propagator and exporter protocol both use w3c baggage specification. Jaeger propagator will be deprecated. RFC 3986 is compatible with W3C Baggage specification. I think the most important consideration is to following Character Encoding guide

Difference about space

# Ruby encode_uri_component/encode_www_form_component
URI.encode_uri_component(" ")       # Space Encoding %20
URI.encode_www_form_component(" ")  # Space Encoding +
CGI.escape(" ")                     # Space Encoding +

# Python is using unquote/quote and unquote_plus/quote_plus (RFC 3986)
urllib.parse.quote(" ")        # Space Encoding	%20
urllib.parse.quote_plus(" ")   # Space Encoding	+

# JS uses default encodeURIComponent
encodeURIComponent(" ")        # Space Encoding	%20

Difference about * (W3C Baggage allows both * and %2A for encoding *)

python quote/quote_plus encode to %2A
js encodeURIComponent doesn't encode to %2A
ruby encode_uri_component/encode_www_form_component doesn't encode %2A

@arielvalentin
Copy link
Contributor

Per our slack convo I think we agree that uri is the most correct per the spec. Let's use that and set this as a fix:

@arielvalentin
Copy link
Contributor

Actually how do you feel about splitting this or into separate ones.

Have the URI fix in a separate PR from ostruct so it's more explicit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants