feat: Use Semconv Naming For ActionPack (#1224)

* feat: Use Semconv Naming For ActionPack

Aligns the span name closer to the specification instead of using Ruby class/method naming syntax

Fixes #961

* squash: remove conditional logic on rails version

* squash: add leading slash

* squash: fix gem version references for 3.0

* squash: relax the test a little

* squash: added some docs and refactored the code a little

* squash: tests and more docs

* squash: spelling errors

* squash: Re-add assertions

Author: arielvalentin

instrumentation/action_pack/README.md

@@ -42,6 +42,25 @@ See the table below for details of what [Rails Framework Hook Events](https://gu
 | - | - | - | - |
 | `process_action.action_controller` | :white_check_mark: | :x: | It modifies the existing Rack span |
 
+## Semantic Conventions
+
+This instrumentation generally uses [HTTP server semantic conventions](https://opentelemetry.io/docs/specs/semconv/http/http-spans/) to update the existing Rack span.
+
+For Rails 7.1+, the span name is updated to match the HTTP method and route that was matched for the request using [`ActionDispatch::Request#route_uri_pattern`](https://api.rubyonrails.org/classes/ActionDispatch/Request.html#method-i-route_uri_pattern), e.g.: `GET /users/:id`
+
+For older versions of Rails the span name is updated to match the HTTP method, controller, and action name that was the target of the request, e.g.: `GET /example/index`
+
+> ![NOTE]: Users may override the `span_naming` option to default to Legacy Span Naming Behavior that uses the controller's class name and action in Ruby documentation syntax, e.g. `ExampleController#index`.
+
+This instrumentation does not emit any custom attributes.
+
+| Attribute Name | Type | Notes |
+| - | - | - |
+| `code.namespace` | String | `ActionController` class name |
+| `code.function` | String | `ActionController` action name e.g. `index`, `show`, `edit`, etc... |
+| `http.route` | String | (Rails 7.1+) the route that was matched for the request |
+| `http.target` | String | The `request.filtered_path` |
+
 ### Error Handling for Action Controller
 
 If an error is triggered by Action Controller (such as a 500 internal server error), Action Pack will typically employ the default `ActionDispatch::PublicExceptions.new(Rails.public_path)` as the `exceptions_app`, as detailed in the [documentation](https://guides.rubyonrails.org/configuring.html#config-exceptions-app).

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

@@ -13,6 +13,7 @@ class ActionController
           # @param config [Hash] of instrumentation options
           def initialize(config)
             @config = config
+            @span_naming = config.fetch(:span_naming)
           end
 
           # Invoked by ActiveSupport::Notifications at the start of the instrumentation block
@@ -22,20 +23,11 @@ def initialize(config)
           # @param payload [Hash] the payload passed as a method argument
           # @return [Hash] the payload passed as a method argument
           def start(_name, _id, payload)
-            rack_span = OpenTelemetry::Instrumentation::Rack.current_span
+            span_name, attributes = to_span_name_and_attributes(payload)
 
-            request = payload[:request]
-
-            rack_span.name = "#{payload[:controller]}##{payload[:action]}" unless request.env['action_dispatch.exception']
-
-            attributes_to_append = {
-              OpenTelemetry::SemanticConventions::Trace::CODE_NAMESPACE => String(payload[:controller]),
-              OpenTelemetry::SemanticConventions::Trace::CODE_FUNCTION => String(payload[:action])
-            }
-
-            attributes_to_append[OpenTelemetry::SemanticConventions::Trace::HTTP_TARGET] = request.filtered_path if request.filtered_path != request.fullpath
-
-            rack_span.add_attributes(attributes_to_append)
+            span = OpenTelemetry::Instrumentation::Rack.current_span
+            span.name = span_name
+            span.add_attributes(attributes)
           rescue StandardError => e
             OpenTelemetry.handle_error(exception: e)
           end
@@ -47,11 +39,37 @@ def start(_name, _id, payload)
           # @param payload [Hash] the payload passed as a method argument
           # @return [Hash] the payload passed as a method argument
           def finish(_name, _id, payload)
-            rack_span = OpenTelemetry::Instrumentation::Rack.current_span
-            rack_span.record_exception(payload[:exception_object]) if payload[:exception_object]
+            span = OpenTelemetry::Instrumentation::Rack.current_span
+            span.record_exception(payload[:exception_object]) if payload[:exception_object]
           rescue StandardError => e
             OpenTelemetry.handle_error(exception: e)
           end
+
+          private
+
+          # Extracts the span name and attributes from the payload
+          #
+          # @param payload [Hash] the payload passed from ActiveSupport::Notifications
+          # @return [Array<String, Hash>] the span name and attributes
+          def to_span_name_and_attributes(payload)
+            request = payload[:request]
+            http_route = request.route_uri_pattern if request.respond_to?(:route_uri_pattern)
+
+            attributes = {
+              OpenTelemetry::SemanticConventions::Trace::CODE_NAMESPACE => String(payload[:controller]),
+              OpenTelemetry::SemanticConventions::Trace::CODE_FUNCTION => String(payload[:action])
+            }
+            attributes[OpenTelemetry::SemanticConventions::Trace::HTTP_ROUTE] = http_route if http_route
+            attributes[OpenTelemetry::SemanticConventions::Trace::HTTP_TARGET] = request.filtered_path if request.filtered_path != request.fullpath
+
+            if @span_naming == :semconv
+              return ["#{request.method} #{http_route.gsub('(.:format)', '')}", attributes] if http_route
+
+              return ["#{request.method} /#{payload.dig(:params, :controller)}/#{payload.dig(:params, :action)}", attributes]
+            end
+
+            ["#{payload[:controller]}##{payload[:action]}", attributes]
+          end
         end
       end
     end

instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/instrumentation.rb

@@ -7,7 +7,31 @@
 module OpenTelemetry
   module Instrumentation
     module ActionPack
-      # The Instrumentation class contains logic to detect and install the ActionPack instrumentation
+      # The {OpenTelemetry::Instrumentation::ActionPack::Instrumentation} class contains logic to detect and install the ActionPack instrumentation
+      #
+      # Installation and configuration of this instrumentation is done within the
+      # {https://www.rubydoc.info/gems/opentelemetry-sdk/OpenTelemetry/SDK#configure-instance_method OpenTelemetry::SDK#configure}
+      # block, calling {https://www.rubydoc.info/gems/opentelemetry-sdk/OpenTelemetry%2FSDK%2FConfigurator:use use()}
+      # or {https://www.rubydoc.info/gems/opentelemetry-sdk/OpenTelemetry%2FSDK%2FConfigurator:use_all use_all()}.
+      #
+      # ## Configuration keys and options
+      #
+      # ### `:span_naming`
+      #
+      # Specifies how the span names are set. Can be one of:
+      #
+      # - `:semconv` **(default)** - The span name will use HTTP semantic conventions '{method http.route}', for example `GET /users/:id`
+      # - `:class` - The span name will appear as '<ActionController class name>#<action>',
+      #   for example `UsersController#show`.
+      #
+      # @example An explicit default configuration
+      #   OpenTelemetry::SDK.configure do |c|
+      #     c.use_all({
+      #       'OpenTelemetry::Instrumentation::ActionPack' => {
+      #         span_naming: :class
+      #       },
+      #     })
+      #   end
       class Instrumentation < OpenTelemetry::Instrumentation::Base
         MINIMUM_VERSION = Gem::Version.new('6.1.0')
 
@@ -17,6 +41,8 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base
           patch
         end
 
+        option :span_naming, default: :semconv, validate: %i[semconv class]
+
         present do
           defined?(::ActionController)
         end

instrumentation/action_pack/opentelemetry-instrumentation-action_pack.gemspec

@@ -34,7 +34,6 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency 'minitest', '~> 5.0'
   spec.add_development_dependency 'opentelemetry-sdk', '~> 1.1'
   spec.add_development_dependency 'opentelemetry-test-helpers', '~> 0.3'
-  spec.add_development_dependency 'rails', '>= 6.1'
   spec.add_development_dependency 'rake', '~> 13.0'
   spec.add_development_dependency 'rubocop', '~> 1.68.0'
   spec.add_development_dependency 'rubocop-performance', '~> 1.22.0'

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

@@ -36,7 +36,6 @@
 
     _(last_response.body).must_equal 'actually ok'
     _(last_response.ok?).must_equal true
-    _(span.name).must_equal 'ExampleController#ok'
     _(span.kind).must_equal :server
     _(span.status.ok?).must_equal true
 
@@ -65,7 +64,7 @@
 
     _(last_response.body).must_equal 'created new item'
     _(last_response.ok?).must_equal true
-    _(span.name).must_equal 'ExampleController#new_item'
+    _(span.name).must_match(/^GET/)
     _(span.kind).must_equal :server
     _(span.status.ok?).must_equal true
 
@@ -82,24 +81,25 @@
     _(span.attributes['code.function']).must_equal 'new_item'
   end
 
-  it 'sets the span name when the controller raises an exception' do
-    get 'internal_server_error'
+  describe 'when encountering server side errors' do
+    it 'sets semconv attributes' do
+      get 'internal_server_error'
 
-    _(span.name).must_equal 'ExampleController#internal_server_error'
-    _(span.kind).must_equal :server
-    _(span.status.ok?).must_equal false
+      _(span.kind).must_equal :server
+      _(span.status.ok?).must_equal false
 
-    _(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::Rack'
-    _(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::Rack::VERSION
+      _(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::Rack'
+      _(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::Rack::VERSION
 
-    _(span.attributes['http.method']).must_equal 'GET'
-    _(span.attributes['http.host']).must_equal 'example.org'
-    _(span.attributes['http.scheme']).must_equal 'http'
-    _(span.attributes['http.target']).must_equal '/internal_server_error'
-    _(span.attributes['http.status_code']).must_equal 500
-    _(span.attributes['http.user_agent']).must_be_nil
-    _(span.attributes['code.namespace']).must_equal 'ExampleController'
-    _(span.attributes['code.function']).must_equal 'internal_server_error'
+      _(span.attributes['http.method']).must_equal 'GET'
+      _(span.attributes['http.host']).must_equal 'example.org'
+      _(span.attributes['http.scheme']).must_equal 'http'
+      _(span.attributes['http.target']).must_equal '/internal_server_error'
+      _(span.attributes['http.status_code']).must_equal 500
+      _(span.attributes['http.user_agent']).must_be_nil
+      _(span.attributes['code.namespace']).must_equal 'ExampleController'
+      _(span.attributes['code.function']).must_equal 'internal_server_error'
+    end
   end
 
   it 'does not set the span name when an exception is raised in middleware' do
@@ -139,13 +139,79 @@
     _(span.attributes['code.function']).must_be_nil
   end
 
+  describe 'span naming' do
+    describe 'when using the default span_naming configuration' do
+      describe 'successful requests' do
+        describe 'Rails Version < 7.1' do
+          it 'uses the http method controller and action name' do
+            skip "Rails #{Rails.gem_version} uses ActionDispatch::Request#route_uri_pattern" if Rails.gem_version >= Gem::Version.new('7.1')
+            get '/ok'
+
+            _(span.name).must_equal 'GET /example/ok'
+          end
+
+          it 'excludes route params' do
+            skip "Rails #{Rails.gem_version} uses ActionDispatch::Request#route_uri_pattern" if Rails.gem_version >= Gem::Version.new('7.1')
+            get '/items/1234'
+
+            _(span.name).must_equal 'GET /example/item'
+          end
+        end
+
+        describe 'Rails Version >= 7.1' do
+          it 'uses the Rails route' do
+            skip "Rails #{Rails.gem_version} does not define ActionDispatch::Request#route_uri_pattern" if Rails.gem_version < Gem::Version.new('7.1')
+            get '/ok'
+
+            _(span.name).must_equal 'GET /ok'
+          end
+
+          it 'includes route params' do
+            skip "Rails #{Rails.gem_version} does not define ActionDispatch::Request#route_uri_pattern" if Rails.gem_version < Gem::Version.new('7.1')
+            get '/items/1234'
+
+            _(span.name).must_equal 'GET /items/:id'
+          end
+        end
+      end
+
+      describe 'server errors' do
+        it 'uses the http method controller and action name for server side errors' do
+          skip "Rails #{Rails.gem_version} uses ActionDispatch::Request#route_uri_pattern" if Rails.gem_version >= Gem::Version.new('7.1')
+
+          get 'internal_server_error'
+
+          _(span.name).must_equal 'GET /example/internal_server_error'
+        end
+
+        it 'uses the Rails route for server side errors' do
+          skip "Rails #{Rails.gem_version} uses ActionDispatch::Request#route_uri_pattern" if Rails.gem_version < Gem::Version.new('7.1')
+
+          get 'internal_server_error'
+
+          _(span.name).must_equal 'GET /internal_server_error'
+        end
+      end
+    end
+
+    describe 'when using the class span_naming' do
+      let(:config) { { span_naming: :class } }
+
+      it 'uses the http method and controller name' do
+        get '/ok'
+
+        _(span.name).must_equal 'ExampleController#ok'
+      end
+    end
+  end
+
   describe 'when the application has exceptions_app configured' do
     let(:rails_app) { AppConfig.initialize_app(use_exceptions_app: true) }
 
     it 'does not overwrite the span name from the controller that raised' do
       get 'internal_server_error'
 
-      _(span.name).must_equal 'ExampleController#internal_server_error'
+      _(span.name).must_match(/^GET/)
       _(span.kind).must_equal :server
       _(span.status.ok?).must_equal false
 

instrumentation/action_pack/test/test_helpers/controllers/example_controller.rb

@@ -20,6 +20,6 @@ def new_item
   end
 
   def internal_server_error
-    raise :internal_server_error
+    raise 'internal_server_error'
   end
 end

instrumentation/rails/test/instrumentation/opentelemetry/instrumentation/rails/patches/action_controller/metal_test.rb

@@ -17,12 +17,12 @@
   # Clear captured spans
   before { exporter.reset }
 
-  it 'sets the span name to the format: HTTP_METHOD /rails/route(.:format)' do
+  it 'sets the span name to the format: HTTP_METHOD /rails/route' do
     get '/ok'
 
     _(last_response.body).must_equal 'actually ok'
     _(last_response.ok?).must_equal true
-    _(span.name).must_equal 'ExampleController#ok'
+    _(span.name).must_match %r{GET.*/ok}
     _(span.kind).must_equal :server
     _(span.status.ok?).must_equal true