fix: action_pack always assuming sdk spans (#1562)

fix: action_pack assuming sdk span

Author: robertlaurin

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

@@ -23,6 +23,9 @@ 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 = OpenTelemetry::Instrumentation::Rack.current_span
+            return unless span.recording?
+
             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.
@@ -36,22 +39,18 @@ def start(_name, _id, payload)
             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
+            if @span_naming == :semconv
+              span.name = 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
+            # 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 = "#{payload[:controller]}##{payload[:action]}"
+            end
 
-            span.name = span_name
             span.add_attributes(attributes)
           rescue StandardError => e
             OpenTelemetry.handle_error(exception: e)

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

@@ -236,6 +236,12 @@
       _(span.attributes['code.namespace']).must_equal 'ExceptionsController'
       _(span.attributes['code.function']).must_equal 'show'
     end
+
+    it 'does not raise with api/non recording spans' do
+      with_sampler(OpenTelemetry::SDK::Trace::Samplers::ALWAYS_OFF) do
+        get 'internal_server_error'
+      end
+    end
   end
 
   it 'sets filters `http.target`' do

instrumentation/action_pack/test/test_helper.rb

@@ -24,3 +24,11 @@
   c.use 'OpenTelemetry::Instrumentation::ActionPack'
   c.add_span_processor span_processor
 end
+
+def with_sampler(sampler)
+  previous_sampler = OpenTelemetry.tracer_provider.sampler
+  OpenTelemetry.tracer_provider.sampler = sampler
+  yield
+ensure
+  OpenTelemetry.tracer_provider.sampler = previous_sampler
+end