feat: Set span error only for 5xx response range (#1196)

feat: Sets span error only for 5xx response range

This also avoids setting span in error when a Rails ActionCable hijacked response returns a -1 status code

Co-authored-by: Ariel Valentin <[email protected]>

Author: j15e

instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb

@@ -43,7 +43,6 @@ class EventHandler
           include ::Rack::Events::Abstract
 
           OTEL_TOKEN_AND_SPAN = 'otel.rack.token_and_span'
-          GOOD_HTTP_STATUSES = (100..499)
 
           # Creates a server span for this current request using the incoming parent context
           # and registers them as the {current_span}
@@ -208,7 +207,7 @@ def detach_context(request)
           end
 
           def add_response_attributes(span, response)
-            span.status = OpenTelemetry::Trace::Status.error unless GOOD_HTTP_STATUSES.include?(response.status.to_i)
+            span.status = OpenTelemetry::Trace::Status.error if response.server_error?
             attributes = extract_response_attributes(response)
             span.add_attributes(attributes)
           rescue StandardError => e

instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb

@@ -152,7 +152,7 @@ def create_request_span_name(request_uri_or_path_info, env)
           end
 
           def set_attributes_after_request(span, status, headers, _response)
-            span.status = OpenTelemetry::Trace::Status.error unless (100..499).cover?(status.to_i)
+            span.status = OpenTelemetry::Trace::Status.error if (500..599).cover?(status.to_i)
             span.set_attribute('http.status_code', status)
 
             # NOTE: if data is available, it would be good to do this:

instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/event_handler_test.rb

@@ -105,6 +105,19 @@
       _(proxy_event).must_be_nil
     end
 
+    describe 'with a hijacked response' do
+      let(:service) do
+        lambda do |env|
+          env['rack.hijack?'] = true
+          [-1, {}, []]
+        end
+      end
+
+      it 'sets the span status to "unset"' do
+        _(rack_span.status.code).must_equal OpenTelemetry::Trace::Status::UNSET
+      end
+    end
+
     describe 'when baggage is set' do
       let(:headers) do
         Hash(

instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/tracer_middleware_test.rb

@@ -72,6 +72,19 @@
       _(first_span.status.code).must_equal OpenTelemetry::Trace::Status::UNSET
     end
 
+    describe 'with a hijacked response' do
+      let(:app) do
+        lambda do |env|
+          env['rack.hijack?'] = true
+          [-1, {}, []]
+        end
+      end
+
+      it 'sets the span status to "unset"' do
+        _(first_span.status.code).must_equal OpenTelemetry::Trace::Status::UNSET
+      end
+    end
+
     it 'has no parent' do
       _(first_span.parent_span_id).must_equal OpenTelemetry::Trace::INVALID_SPAN_ID
     end