fix: patch the issue for frozen string on ruby < 3.0 with string interpolation

[xuan-cao-swi]

Description

Although Ruby 2.7.5 is EOL, this change would make it easier for people still working on < 3.0.0 by not having to pin to lower version of opentelemetry-common

Other two string interpolation would work for both > 3 and < 3 (that string is mutable) are:

foobar = "foo%s" % ["bar"]
foobar = sprintf("foo%s","bar")

But not sure if the format is preferred

Fixes part of #1463

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Tested with common, api and propagator jaeger gem using bundle exec rake test

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

• Yes. - Link to PR:
• No.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[dazuma]

I think it would be simpler to avoid the need for mutation in the first place:

def to_rack_key(key)
  "HTTP_#{key}".tr('-', '_').upcase
end

Note that non-mutating methods like tr and upcase return unfrozen strings in both Ruby < 3.0 and Ruby >= 3.0, so this method still safely returns an unfrozen string.

[fbogsany]

The original goal here was to avoid unnecessary allocations.

# frozen_string_literal: true

require "benchmark/ipsa"

key = "content-type"

Benchmark.ipsa do |x|
  x.report("interpolate-tr-upcase") do
    "HTTP_#{key}".tr('-', '_').upcase
  end
  x.report("concat-tr!-upcase!") do
    ret = 'HTTP_' + key
    ret.tr!('-', '_')
    ret.upcase!
    ret
  end
end

bogsanyi@franciss-mbp ~ % chruby 2.7.2
bogsanyi@franciss-mbp ~ % ruby to_rack_key_bench.rb
Allocations -------------------------------------
interpolate-tr-upcase
                           3/0  alloc/ret        3/0  strings/ret
  concat-tr!-upcase!       1/0  alloc/ret        1/0  strings/ret
Warming up --------------------------------------
interpolate-tr-upcase
                       142.175k i/100ms
  concat-tr!-upcase!   251.416k i/100ms
Calculating -------------------------------------
interpolate-tr-upcase
                          2.137M (± 1.1%) i/s -     10.805M
  concat-tr!-upcase!      5.156M (± 1.0%) i/s -     25.896M
bogsanyi@franciss-mbp ~ % chruby 3.2.2
bogsanyi@franciss-mbp ~ % ruby to_rack_key_bench.rb
Allocations -------------------------------------
interpolate-tr-upcase
                           4/0  alloc/ret        3/0  strings/ret
  concat-tr!-upcase!       1/0  alloc/ret        1/0  strings/ret
Warming up --------------------------------------
interpolate-tr-upcase
                       182.614k i/100ms
  concat-tr!-upcase!   259.884k i/100ms
Calculating -------------------------------------
interpolate-tr-upcase
                          3.023M (± 1.1%) i/s -     15.157M
  concat-tr!-upcase!      4.692M (± 1.0%) i/s -     23.649M

[robbkidd]

I was unable to find anything in the code archaeology about the choice for a local variable in the to/from methods. Was there some optimization (memory? GC work?) intended in allocating and mutating a single string object?

(UPDATE: @fbogsany snuck in and dropped the Lore while I had this comment editor open. My comments below have been Overcome By Benchmarks.)

If not, I'm a fan of transform chains like @dazuma suggests.

def to_rack_key(key)
  key
    .prepend("HTTP_") # prepend is another style option, but interpolation's fine, too
    .tr('-', '_')
    .upcase
end

def from_rack_key(key)
  start = key.start_with?('HTTP_') ? 5 : 0
  key[start..]
    .tr('_', '-')
    .downcase
end

I am not in favor of adding branching logic based on RUBY_VERSION, particularly for unsupported versions. I don't mind changing this transform to a single implementation that works in Ruby 2.7 and 3+, but without Ruby 2.7 in the test matrix, there's nothing that will catch us breaking 2.7 use in the future.

[xuan-cao-swi]

Thanks for the suggestions. I also tried other interpolation method. Percentage and sprintf seems works (not immutable) for both ruby < 3 and > 3

# frozen_string_literal: true

require "benchmark/ipsa"

key = "content-type"

Benchmark.ipsa do |x|
  x.report("interpolate-tr-upcase") do
    "HTTP_#{key}".tr('-', '_').upcase
  end
  x.report("interpolate-tr!-upcase!") do
    ret = "HTTP_#{key}"
    ret.tr!('-', '_')
    ret.upcase!
    ret
  end
  x.report("concat-tr!-upcase!") do
    ret = 'HTTP_' + key
    ret.tr!('-', '_')
    ret.upcase!
    ret
  end
  x.report("percentage-interpolate") do
    ret = "HTTP_%s" % key
    ret.tr!('-', '_')
    ret.upcase!
    ret
  end
  x.report("sprintf-interpolate") do
    ret = sprintf("HTTP_%s",key)
    ret.tr!('-', '_')
    ret.upcase!
    ret
  end
end

bash-5.1$ ruby to_rack_key_bench.rb 
Allocations -------------------------------------
interpolate-tr-upcase
                           3/0  alloc/ret        3/0  strings/ret
interpolate-tr!-upcase!
                           1/0  alloc/ret        1/0  strings/ret
  concat-tr!-upcase!       1/0  alloc/ret        1/0  strings/ret
percentage-interpolate
                           2/0  alloc/ret        1/0  strings/ret
 sprintf-interpolate       2/0  alloc/ret        1/0  strings/ret
Warming up --------------------------------------
interpolate-tr-upcase
                       205.501k i/100ms
interpolate-tr!-upcase!
                       212.834k i/100ms
  concat-tr!-upcase!   220.677k i/100ms
percentage-interpolate
                       103.801k i/100ms
 sprintf-interpolate   110.715k i/100ms
Calculating -------------------------------------
interpolate-tr-upcase
                          3.949M (± 2.0%) i/s -     19.934M
interpolate-tr!-upcase!
                          4.283M (± 3.9%) i/s -     21.496M
  concat-tr!-upcase!      4.314M (± 1.6%) i/s -     21.626M
percentage-interpolate
                          1.451M (± 1.8%) i/s -      7.266M
 sprintf-interpolate      1.492M (± 1.9%) i/s -      7.529M

[fbogsany]

Suggested change

	ret = RUBY_VERSION < '3.0.0'? 'HTTP_' + key : "HTTP_#{key}" # rubocop:disable Style/StringConcatenation
	ret = +"HTTP_#{key}"

This produces an unfrozen string. It works in Ruby 2.7.2 and Ruby 3.2.2 in my testing. It appears to minimize allocations as well. It is slightly slower than the original code.

[xuan-cao-swi]

Thanks @fbogsany for the suggestions. I have updated it.

[cheempz]

@dazuma @fbogsany @robbkidd could we get your attention again please? This would remove one major hurdle for our organization to migrate to OTel Ruby and I'd really appreciate any help you can give :)

[mwear]

LGTM. @fbogsany did you want to confirm your comments have been addressed?

[mwear]

I'm inclined to merge this if there are not any objections. I'd feel a little better with a ✅ from @fbogsany or @robertlaurin.

[mwear]

@dazuma, it looks like your request for changes is blocking this PR. Do you mind taking a look and resolving if everything looks ok to you?

The author went with @fbogsany's suggestions

[9toon]

This changes, added for Ruby under 3.0, was released as v1.2.2 which requires Ruby 3.0 or higher. Is there any plan to port it as v1.1.1, for example, to be also available for Ruby under 3.0?