fix: rack span class naming (#1555)

Author: robertlaurin

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

@@ -23,9 +23,34 @@ 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)
-            span_name, attributes = to_span_name_and_attributes(payload)
+            request = payload[:request]
+            # It seems that there are cases in Rails functional tests where it bypasses the routing system and the `action_dispatch.route_uri_pattern` header not being set.
+            # Our Test suite executes the routing system so we are unable to recreate this error case.
+            # https://github.com/rails/rails/blob/747f85f200e7bb2c1a31b4e26e5a5655e2dc0cdc/actionpack/lib/action_dispatch/http/request.rb#L160
+            http_route = request.route_uri_pattern&.chomp('(.:format)') 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
 
             span = OpenTelemetry::Instrumentation::Rack.current_span
+            span_name = if @span_naming == :semconv
+                          if http_route
+                            "#{request.method} #{http_route}"
+                          else
+                            "#{request.method} /#{payload.dig(:params, :controller)}/#{payload.dig(:params, :action)}"
+                          end
+                        # If there is an exception we want to keep the original span name
+                        # so it is easier to see where the request was routed to.
+                        elsif request.env['action_dispatch.exception']
+                          span.name
+                        else
+                          "#{payload[:controller]}##{payload[:action]}"
+                        end
+
             span.name = span_name
             span.add_attributes(attributes)
           rescue StandardError => e
@@ -44,35 +69,6 @@ def finish(_name, _id, payload)
           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]
-            # It seems that there are cases in Rails functional tests where it bypasses the routing system and the `action_dispatch.route_uri_pattern` header not being set.
-            # Our Test suite executes the routing system so we are unable to recreate this error case.
-            # https://github.com/rails/rails/blob/747f85f200e7bb2c1a31b4e26e5a5655e2dc0cdc/actionpack/lib/action_dispatch/http/request.rb#L160
-            http_route = request.route_uri_pattern&.chomp('(.:format)') 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}", 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/test/opentelemetry/instrumentation/action_pack/handlers/action_controller_test.rb

@@ -215,11 +215,12 @@
 
   describe 'when the application has exceptions_app configured' do
     let(:rails_app) { AppConfig.initialize_app(use_exceptions_app: true) }
+    let(:config) { { span_naming: :class } }
 
     it 'does not overwrite the span name from the controller that raised' do
       get 'internal_server_error'
 
-      _(span.name).must_match(/^GET/)
+      _(span.name).must_equal 'ExampleController#internal_server_error'
       _(span.kind).must_equal :server
       _(span.status.ok?).must_equal false