From 5c647b56c2794c137c77a28c17b4bac1ec82a5bc Mon Sep 17 00:00:00 2001
From: Jean-Philippe Doyle <jeanphilippe.doyle@gmail.com>
Date: Wed, 23 Oct 2024 15:34:37 -0400
Subject: [PATCH] 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 <arielvalentin@users.noreply.github.com>
---
 .../rack/middlewares/event_handler.rb               |  3 +--
 .../rack/middlewares/tracer_middleware.rb           |  2 +-
 .../rack/middlewares/event_handler_test.rb          | 13 +++++++++++++
 .../rack/middlewares/tracer_middleware_test.rb      | 13 +++++++++++++
 4 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb
index 5475a4fc5..a77448cb7 100644
--- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb
+++ b/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
diff --git a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb b/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb
index 608728ac6..e6aceba7d 100644
--- a/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/tracer_middleware.rb
+++ b/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:
diff --git a/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/event_handler_test.rb b/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/event_handler_test.rb
index b106a5d0d..afb254bca 100644
--- a/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/event_handler_test.rb
+++ b/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(
diff --git a/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/tracer_middleware_test.rb b/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/tracer_middleware_test.rb
index 89c68c328..b17f60ed9 100644
--- a/instrumentation/rack/test/opentelemetry/instrumentation/rack/middlewares/tracer_middleware_test.rb
+++ b/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