Report response status when there is no response (#1125)

When there is no response object given to the EventHandler's `on_finish` callback, we can expect an unhandled error has occurred. When have also received an exception in the `on_error` callback, report the response status as 500. This solution is not perfect. It may actually be another response status if something else above the EventHandler handles the exception. A setup we don't recommend.
appsignal · Jun 28, 2024 · 0230ab4 · 0230ab4
1 parent 2147cd0
commit 0230ab4
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 20 deletions.
diff --git a/.changesets/track-error-response-status.md b/.changesets/track-error-response-status.md
@@ -0,0 +1,6 @@
+---
+bump: patch
+type: add
+---
+
+Track error response status for web requests. When an unhandled exception reaches the AppSignal EventHandler instrumentation, report the response status as `500` for the `response_status` tag on the transaction and on the `response_status` metric.
diff --git a/lib/appsignal/rack/event_handler.rb b/lib/appsignal/rack/event_handler.rb
@@ -4,6 +4,7 @@ module Appsignal
   module Rack
     APPSIGNAL_TRANSACTION = "appsignal.transaction"
     APPSIGNAL_EVENT_HANDLER_ID = "appsignal.event_handler_id"
+    APPSIGNAL_EVENT_HANDLER_HAS_ERROR = "appsignal.event_handler.error"
     RACK_AFTER_REPLY = "rack.after_reply"
 
     # @api private
@@ -71,6 +72,7 @@ def on_error(request, _response, error)
           transaction = request.env[APPSIGNAL_TRANSACTION]
           return unless transaction
 
+          request.env[APPSIGNAL_EVENT_HANDLER_HAS_ERROR] = true
           transaction.set_error(error)
         end
       end
@@ -84,12 +86,18 @@ def on_finish(request, response)
         self.class.safe_execution("Appsignal::Rack::EventHandler#on_finish") do
           transaction.finish_event("process_request.rack", "", "")
           transaction.set_http_or_background_queue_start
-          if response
-            transaction.set_tags(:response_status => response.status)
+          response_status =
+            if response
+              response.status
+            elsif request.env[APPSIGNAL_EVENT_HANDLER_HAS_ERROR] == true
+              500
+            end
+          if response_status
+            transaction.set_tags(:response_status => response_status)
             Appsignal.increment_counter(
               :response_status,
               1,
-              :status => response.status,
+              :status => response_status,
               :namespace => format_namespace(transaction.namespace)
             )
           end

diff --git a/spec/lib/appsignal/rack/event_handler_spec.rb b/spec/lib/appsignal/rack/event_handler_spec.rb
@@ -22,6 +22,10 @@ def on_start
     event_handler_instance.on_start(request, response)
   end
 
+  def on_error(error)
+    event_handler_instance.on_error(request, response, error)
+  end
+
   describe "#on_start" do
     it "creates a new transaction" do
       expect { on_start }.to change { created_transactions.length }.by(1)
@@ -119,10 +123,6 @@ def on_start
   end
 
   describe "#on_error" do
-    def on_error(error)
-      event_handler_instance.on_error(request, response, error)
-    end
-
     it "reports the error" do
       on_start
       on_error(ExampleStandardError.new("the error"))
@@ -235,6 +235,29 @@ def on_finish(given_request = request, given_response = response)
         on_start
         on_finish(request, nil)
       end
+
+      context "with an error previously recorded by on_error" do
+        it "sets response status 500 as a tag" do
+          on_start
+          on_error(ExampleStandardError.new("the error"))
+          on_finish(request, nil)
+
+          expect(last_transaction.to_h).to include(
+            "sample_data" => hash_including(
+              "tags" => { "response_status" => 500 }
+            )
+          )
+        end
+
+        it "increments the response status counter for response status 500" do
+          expect(Appsignal).to receive(:increment_counter)
+            .with(:response_status, 1, :status => 500, :namespace => :web)
+
+          on_start
+          on_error(ExampleStandardError.new("the error"))
+          on_finish(request, nil)
+        end
+      end
     end
 
     context "with error inside on_finish handler" do
@@ -296,23 +319,48 @@ def on_finish(given_request = request, given_response = response)
       )
     end
 
-    it "sets the response status as a tag" do
-      on_start
-      on_finish
+    context "with response" do
+      it "sets the response status as a tag" do
+        on_start
+        on_finish
 
-      expect(last_transaction.to_h).to include(
-        "sample_data" => hash_including(
-          "tags" => { "response_status" => 200 }
+        expect(last_transaction.to_h).to include(
+          "sample_data" => hash_including(
+            "tags" => { "response_status" => 200 }
+          )
         )
-      )
-    end
+      end
 
-    it "increments the response status counter for response status" do
-      expect(Appsignal).to receive(:increment_counter)
-        .with(:response_status, 1, :status => 200, :namespace => :web)
+      it "increments the response status counter for response status" do
+        expect(Appsignal).to receive(:increment_counter)
+          .with(:response_status, 1, :status => 200, :namespace => :web)
 
-      on_start
-      on_finish
+        on_start
+        on_finish
+      end
+
+      context "with an error previously recorded by on_error" do
+        it "sets response status from the response as a tag" do
+          on_start
+          on_error(ExampleStandardError.new("the error"))
+          on_finish
+
+          expect(last_transaction.to_h).to include(
+            "sample_data" => hash_including(
+              "tags" => { "response_status" => 200 }
+            )
+          )
+        end
+
+        it "increments the response status counter based on the response" do
+          expect(Appsignal).to receive(:increment_counter)
+            .with(:response_status, 1, :status => 200, :namespace => :web)
+
+          on_start
+          on_error(ExampleStandardError.new("the error"))
+          on_finish
+        end
+      end
     end
 
     it "logs an error in case of an error" do