feat: Strip Rails `(.:format)` suffix from `http.route` #1375

akahn · 2025-01-29T17:30:16Z

In our OpenTelemetry spans, all http.route values end in (.:format). This is unsightly and does not help when diagnosing an issue using traces. It also stands out as different from the http.route produced by other language's OTel SDKs, which only include parts of the URL path itself.

With this change, we strip this suffix from http.route, just like we are already doing for the span name.

linux-foundation-easycla · 2025-01-29T17:30:22Z

The committers listed above are authorized under a signed CLA.

✅ login: akahn / name: Alex Kahn (1ea7157, cd9fe4b, 4fffe3a)
✅ login: arielvalentin / name: Ariel Valentin (8d9e0ba)

arielvalentin

I have left a request to use chomp as opposed to gsub to achieve this.

Please also make sure to review the contributing guide that requests you use conventional commit format for the PR title and individual commits.

I am also surprised that tests passed for this!

If you are able to help us track down why there were no test failures or updates accompanied with this change that would be grately appreciated.

...tion/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb

akahn · 2025-01-29T18:48:09Z

Thanks for the review, @arielvalentin!

Happy to make that change, and make sure there is test coverage for attributes. I tried to run the tests according to the instructions in the README but got the error below. Is there a particular version of Ruby I need to be running? Any help would be appreciated.

❯ bundle exec rake
/Users/alex.kahn/Codes/opentelemetry-ruby-contrib/instrumentation/action_pack/test/test_helper.rb:7: warning: logger was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add logger to your Gemfile or gemspec to silence this warning.
/Users/alex.kahn/.gem/ruby/3.4.1/gems/pry-0.14.2/lib/pry/command_state.rb:3: warning: ostruct was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add ostruct to your Gemfile or gemspec to silence this warning.
/Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/3.4.0/readline.rb:4: warning: reline was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add reline to your Gemfile or gemspec to silence this warning.
/Users/alex.kahn/.gem/ruby/3.4.1/gems/byebug-11.1.3/lib/byebug/commands/irb.rb:4: warning: irb was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add irb to your Gemfile or gemspec to silence this warning.
/Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/3.4.0/bundled_gems.rb:82:in 'Kernel.require': cannot load such file -- rack/test (LoadError)
	from /Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/3.4.0/bundled_gems.rb:82:in 'block (2 levels) in Kernel#replace_require'
	from /Users/alex.kahn/Codes/opentelemetry-ruby-contrib/instrumentation/action_pack/test/test_helper.rb:13:in '<top (required)>'
	from /Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/3.4.0/bundled_gems.rb:82:in 'Kernel.require'
	from /Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/3.4.0/bundled_gems.rb:82:in 'block (2 levels) in Kernel#replace_require'
	from /Users/alex.kahn/Codes/opentelemetry-ruby-contrib/instrumentation/action_pack/test/opentelemetry/instrumentation/action_pack/handlers/action_controller_test.rb:7:in '<top (required)>'
	from /Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/3.4.0/bundled_gems.rb:82:in 'Kernel.require'
	from /Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/3.4.0/bundled_gems.rb:82:in 'block (2 levels) in Kernel#replace_require'
	from /Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/gems/3.4.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:21:in 'block in <main>'
	from /Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/gems/3.4.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in 'Array#select'
	from /Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/gems/3.4.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in '<main>'
rake aborted!
Command failed with status (1)
/Users/alex.kahn/.gem/ruby/3.4.1/bin/bundle:25:in 'Kernel#load'
/Users/alex.kahn/.gem/ruby/3.4.1/bin/bundle:25:in '<main>'
Tasks: TOP => default => test
(See full trace by running task with --trace)

arielvalentin · 2025-01-29T18:53:04Z

Time to update the README! 😿

The gems are executed using appraisal so try this instead:

bundle exec appraisal rake test

That will end up running several versions of Rails that we aim to support. If you want to focus on a specific version check out the appraisal docs for more info.

Thanks again for your help.

akahn · 2025-01-29T19:15:47Z

After running bundle exec appraisal to install the various Rails versions, I ran bundle exec appraisal rake test and got this error:

❯ bundle exec appraisal rake
>> BUNDLE_GEMFILE=/Users/alex.kahn/Codes/opentelemetry-ruby-contrib/instrumentation/action_pack/gemfiles/action_pack_7.0.0.gemfile bundle exec rake
/Users/alex.kahn/Codes/opentelemetry-ruby-contrib/instrumentation/action_pack/test/test_helper.rb:7: warning: logger was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add logger to your Gemfile or gemspec to silence this warning.
/Users/alex.kahn/.gem/ruby/3.4.1/gems/activesupport-7.0.8.7/lib/active_support/core_ext/object/json.rb:229: warning: method redefined; discarding old as_json
/Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/3.4.0/ipaddr.rb:231: warning: previous definition of as_json was here
/Users/alex.kahn/.gem/ruby/3.4.1/gems/activesupport-7.0.8.7/lib/active_support/notifications/fanout.rb:3: warning: mutex_m was loaded from the standard library, but is not part of the default gems starting from Ruby 3.4.0.
You can add mutex_m to your Gemfile or gemspec to silence this warning.
/Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/3.4.0/bundled_gems.rb:82:in 'Kernel.require': cannot load such file -- mutex_m (LoadError)

I tried adding mutex_m as a dependency in the test section of the Gemfile, but got the same error. Any ideas?

arielvalentin · 2025-01-29T19:26:55Z

@akahn I have to admit, I am a little perplexed.

What version of Ruby and Bundler are you running?

akahn · 2025-01-29T20:33:41Z

❯ ruby -v
ruby 3.4.1 (2024-12-25 revision 48d4efcb85) +PRISM [arm64-darwin24]

❯ bundler -v
Bundler version 2.6.2

❯ env | grep RUBY
RUBY_ROOT=/Users/alex.kahn/.rubies/ruby-3.4.1
RUBYOPT=
RUBY_ENGINE=ruby
RUBY_VERSION=3.4.1

❯ env | grep GEM
GEM_ROOT=/Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/gems/3.4.0
GEM_HOME=/Users/alex.kahn/.gem/ruby/3.4.1
GEM_PATH=/Users/alex.kahn/.gem/ruby/3.4.1:/Users/alex.kahn/.rubies/ruby-3.4.1/lib/ruby/gems/3.4.0

Edit: I've downgraded to Ruby 3.3.4, and now have this working.

akahn · 2025-01-29T21:14:26Z

If you are able to help us track down why there were no test failures or updates accompanied with this change that would be grately appreciated.

This is because there are no tests for the http.route attribute.

I've tried adding them but it seems that bundle exec appraisal rake test is only running tests with Rails 7.0.8.7, so the request.respond_to?(:route_uri_pattern) check never passes (and thus http.route isn't added at all). I'm printing Rails.version as the tests run in order to confirm this. Is it just an issue on my end, or do you get the same result?

akahn · 2025-01-29T22:58:40Z

I took a look at what the GitHub actions were doing and did the same by manually setting BUNDLE_GEMFILE before running the tests. (I had assumed that bundle exec appraisal rake test would run the test suite across all the relevant Rails versions, but maybe that's not quite right.) Anyways, I added a simple test case for http.route removing the format suffix.

kaylareopelle · 2025-01-30T00:03:30Z

I had assumed that bundle exec appraisal rake test would run the test suite across all the relevant Rails versions, but maybe that's not quite right.

@akahn - I'm surprised that bundle exec appraisal rake test isn't running all the appraisals for you. That's working for me locally on Ruby 3.3.0.

All I ran from inside instrumentation/rails was:

bundle # to install the gemspec dependencies
bundle exec appraisal install
bundle exec appraisal rake test

I think it might be an issue on your machine, but I'm curious if others are running into the same problem.

kaylareopelle

@arielvalentin @akahn, since this PR changes the value of an attribute, I'm leaning toward the conventional commit prefix being a feat instead of a chore. What do you think? Should this initiate a release?

arielvalentin · 2025-01-30T00:33:35Z

@arielvalentin @akahn, since this PR changes the value of an attribute, I'm leaning toward the conventional commit prefix being a feat instead of a chore. What do you think? Should this initiate a release?

I agree.

arielvalentin · 2025-01-30T00:35:39Z

I took a look at what the GitHub actions were doing and did the same by manually setting BUNDLE_GEMFILE before running the tests. (I had assumed that bundle exec appraisal rake test would run the test suite across all the relevant Rails versions, but maybe that's not quite right.) Anyways, I added a simple test case for http.route removing the format suffix.

The error is happening in Ruby 3.4 but our test matrix isn't up to date yet.

I have a separate PR to add test coverage here: #1347

For now I recommend running the tests with Ruby 3.3 if possible to get them running in your development environment.

akahn · 2025-01-30T01:22:44Z

Thanks for all the guidance. I've renamed the commit and PR to be feat.

arielvalentin · 2025-01-30T02:26:18Z

...ction_pack/test/opentelemetry/instrumentation/action_pack/handlers/action_controller_test.rb

@@ -58,6 +58,14 @@
    _(span.attributes['code.function']).must_equal 'ok'
  end

+  it 'strips (:format) from http.route' do


this test can probably be consolidated with another one but I see why being explicit here is helpful.

Thanks. I noticed the tests were a bit disjointed, but I thought I would leave that to the maintainers or someone more familiar with the project, and to a future PR.

There's an existing describe block for span naming, broken down by Rails version. Maybe there could be another one for span attributes, broken down by Rails version, that this could go in. Alternatively, there could be separate top-level blocks for Rails versions, each with naming and span sections within.

kaylareopelle

Thanks, @akahn!

akahn · 2025-01-30T19:55:32Z

My pleasure @kaylareopelle. What is the release schedule for this gem? My team would love to get this running in our production app.

kaylareopelle · 2025-02-01T00:04:25Z

Hi @akahn! We generally release on Tuesdays near the time of our SIG meeting. Unless someone can fit it in sooner, expect a new release then.

kaylareopelle · 2025-02-04T21:16:36Z

Hi @akahn - This code has been released! You can access it through:

opentelemetry-instrumentation-action_pack 0.12.0
opentelemetry-instrumentation-rails 0.36.0
opentelemetry-instrumentation-all 0.73.0

akahn requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, arielvalentin, ahayworth, plantfansam, robbkidd, simi, kaylareopelle, xuan-cao-swi and a team as code owners January 29, 2025 17:30

akahn changed the title ~~Strip Rails (.:format) suffix from http.route~~ Strip Rails (.:format) suffix from http.route Jan 29, 2025

arielvalentin reviewed Jan 29, 2025

View reviewed changes

...tion/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb Outdated Show resolved Hide resolved

akahn changed the title ~~Strip Rails (.:format) suffix from http.route~~ chore: Strip Rails (.:format) suffix from http.route Jan 29, 2025

akahn force-pushed the strip-rails-format-suffix branch from ee144f7 to 7b796a6 Compare January 29, 2025 19:26

chore: Strip Rails (.:format) suffix from http.route

cd9fe4b

akahn force-pushed the strip-rails-format-suffix branch from 91472bc to e89fe75 Compare January 29, 2025 23:40

kaylareopelle reviewed Jan 30, 2025

View reviewed changes

akahn force-pushed the strip-rails-format-suffix branch from e89fe75 to ed04a56 Compare January 30, 2025 01:20

akahn changed the title ~~chore: Strip Rails (.:format) suffix from http.route~~ feat: Strip Rails (.:format) suffix from http.route Jan 30, 2025

test: Add test for http.route cleaning

4fffe3a

akahn force-pushed the strip-rails-format-suffix branch from ed04a56 to 4fffe3a Compare January 30, 2025 01:22

akahn and others added 2 commits January 29, 2025 20:22

Merge branch 'main' into strip-rails-format-suffix

1ea7157

Merge branch 'main' into strip-rails-format-suffix

8d9e0ba

arielvalentin approved these changes Jan 30, 2025

View reviewed changes

arielvalentin requested a review from kaylareopelle January 30, 2025 04:52

kaylareopelle approved these changes Jan 30, 2025

View reviewed changes

kaylareopelle merged commit 1e9853a into open-telemetry:main Jan 30, 2025
59 checks passed

github-actions bot mentioned this pull request Jan 30, 2025

chore: release main #1276

Draft

akahn deleted the strip-rails-format-suffix branch January 30, 2025 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Strip Rails `(.:format)` suffix from `http.route` #1375

feat: Strip Rails `(.:format)` suffix from `http.route` #1375

akahn commented Jan 29, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 29, 2025 •

edited

Loading

arielvalentin left a comment

akahn commented Jan 29, 2025

arielvalentin commented Jan 29, 2025

akahn commented Jan 29, 2025

arielvalentin commented Jan 29, 2025

akahn commented Jan 29, 2025 •

edited

Loading

akahn commented Jan 29, 2025 •

edited

Loading

akahn commented Jan 29, 2025

kaylareopelle commented Jan 30, 2025

kaylareopelle left a comment

arielvalentin commented Jan 30, 2025

arielvalentin commented Jan 30, 2025

akahn commented Jan 30, 2025

arielvalentin Jan 30, 2025

akahn Jan 30, 2025

kaylareopelle left a comment

akahn commented Jan 30, 2025

kaylareopelle commented Feb 1, 2025

kaylareopelle commented Feb 4, 2025

feat: Strip Rails (.:format) suffix from http.route #1375

feat: Strip Rails (.:format) suffix from http.route #1375

Conversation

akahn commented Jan 29, 2025 • edited Loading

linux-foundation-easycla bot commented Jan 29, 2025 • edited Loading

arielvalentin left a comment

Choose a reason for hiding this comment

akahn commented Jan 29, 2025

arielvalentin commented Jan 29, 2025

akahn commented Jan 29, 2025

arielvalentin commented Jan 29, 2025

akahn commented Jan 29, 2025 • edited Loading

akahn commented Jan 29, 2025 • edited Loading

akahn commented Jan 29, 2025

kaylareopelle commented Jan 30, 2025

kaylareopelle left a comment

Choose a reason for hiding this comment

arielvalentin commented Jan 30, 2025

arielvalentin commented Jan 30, 2025

akahn commented Jan 30, 2025

arielvalentin Jan 30, 2025

Choose a reason for hiding this comment

akahn Jan 30, 2025

Choose a reason for hiding this comment

kaylareopelle left a comment

Choose a reason for hiding this comment

akahn commented Jan 30, 2025

kaylareopelle commented Feb 1, 2025

kaylareopelle commented Feb 4, 2025

feat: Strip Rails `(.:format)` suffix from `http.route` #1375

feat: Strip Rails `(.:format)` suffix from `http.route` #1375

akahn commented Jan 29, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 29, 2025 •

edited

Loading

akahn commented Jan 29, 2025 •

edited

Loading

akahn commented Jan 29, 2025 •

edited

Loading