Fix Rails error on unknown request method (#873)

If `ActionDispatch::Request#request_method` is called when the request method is not an known request method by Rails it will raise an `ActionController::UnknownHttpMethod`. We do not expect this method to raise an error, so it broke our Rails instrumentation, didn't close the transaction which would cause it be reused by the next request handled by Rails. Add exception handling to the method calls so it doesn't break our instrumentation anymore. Fixes #851 Fixes appsignal/support#201
appsignal · Aug 4, 2022 · 7cfed98 · 7cfed98
1 parent bdf1cdc
commit 7cfed98
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 4 deletions.
diff --git a/.changesets/fix-error-on-unknown-http-request-method.md b/.changesets/fix-error-on-unknown-http-request-method.md
@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "fix"
+---
+
+Fix error on unknown HTTP request method. When a request is made with an unknown request method, triggering and `ActionController::UnknownHttpMethod`, it will no longer break the AppSignal instrumentation but omit the request method in the sample data.
diff --git a/lib/appsignal/rack/rails_instrumentation.rb b/lib/appsignal/rack/rails_instrumentation.rb
@@ -40,7 +40,11 @@ def call_with_appsignal_monitoring(env)
           end
           transaction.set_http_or_background_queue_start
           transaction.set_metadata("path", request.path)
-          transaction.set_metadata("method", request.request_method)
+          begin
+            transaction.set_metadata("method", request.request_method)
+          rescue => error
+            Appsignal.logger.error("Unable to report HTTP request method: '#{error}'")
+          end
           Appsignal::Transaction.complete_current!
         end
       end

diff --git a/spec/lib/appsignal/rack/rails_instrumentation_spec.rb b/spec/lib/appsignal/rack/rails_instrumentation_spec.rb
@@ -3,8 +3,10 @@ class MockController
   end
 
   describe Appsignal::Rack::RailsInstrumentation do
-    before :context do
+    let(:log) { StringIO.new }
+    before do
       start_agent
+      Appsignal.logger = test_logger(log)
     end
 
     let(:params) do
@@ -16,9 +18,10 @@ class MockController
         "password" => "super secret"
       }
     end
+    let(:env_extra) { {} }
     let(:app) { double(:call => true) }
     let(:env) do
-      http_request_env_with_data(
+      http_request_env_with_data({
         :params => params,
         :with_queue_start => true,
         "action_dispatch.request_id" => "1",
@@ -27,7 +30,7 @@ class MockController
           :class => MockController,
           :action_name => "index"
         )
-      )
+      }.merge(env_extra))
     end
     let(:middleware) { Appsignal::Rack::RailsInstrumentation.new(app, {}) }
     around { |example| keep_transactions { example.run } }
@@ -102,6 +105,18 @@ def run
         )
       end
 
+      context "with an invalid HTTP request method" do
+        let(:env_extra) { { :request_method => "FOO", "REQUEST_METHOD" => "FOO" } }
+
+        it "does not store the HTTP request method" do
+          run
+
+          transaction_hash = last_transaction.to_h
+          expect(transaction_hash["metadata"]).to_not have_key("method")
+          expect(log_contents(log)).to contains_log(:error, "Unable to report HTTP request method: '")
+        end
+      end
+
       context "with an exception" do
         let(:error) { ExampleException.new("ExampleException message") }
         let(:app) do