Update Hanami to use Rack middleware

As part of #329, update the Hanami integration to use Rack middleware and the EventHandler to instrument requests made to Hanami apps. This standardizes the instrumentation as much as possible between Rack apps and minimizes our reliance on monkeypatches. The only monkeypatch that remains is setting the action name to the Action class name. I have found no other way yet to fetch this metadata from the request metadata, environment or the Hanami router. Part of #329 Mostly solves #911
appsignal · Jun 25, 2024 · e79d427 · e79d427
1 parent b65d667
commit e79d427
Show file tree

Hide file tree

Showing 5 changed files with 130 additions and 108 deletions.
diff --git a/.changesets/report-response_status-tag-and-metric-for-hanami-requests.md b/.changesets/report-response_status-tag-and-metric-for-hanami-requests.md
@@ -0,0 +1,7 @@
+---
+bump: patch
+type: add
+---
+
+Improve instrumentation of Hanami requests by making sure the transaction is always closed.
+It will also report a `response_status` tag and metric for Hanami requests.
diff --git a/lib/appsignal/integrations/hanami.rb b/lib/appsignal/integrations/hanami.rb
@@ -1,9 +1,11 @@
 # frozen_string_literal: true
 
 require "appsignal"
+require "appsignal/rack/hanami_middleware"
 
 module Appsignal
   module Integrations
+    # @api private
     module HanamiPlugin
       def self.init
         Appsignal.internal_logger.debug("Loading Hanami integration")
@@ -19,48 +21,25 @@ def self.init
 
         return unless Appsignal.active?
 
+        hanami_app_config.middleware.use(
+          ::Rack::Events,
+          [Appsignal::Rack::EventHandler.new]
+        )
+        hanami_app_config.middleware.use(Appsignal::Rack::HanamiMiddleware)
+
         ::Hanami::Action.prepend Appsignal::Integrations::HanamiIntegration
       end
     end
-  end
-end
-
-module Appsignal::Integrations::HanamiIntegration
-  def call(env)
-    params = ::Hanami::Action::BaseParams.new(env)
-    request = ::Hanami::Action::Request.new(
-      :env => env,
-      :params => params,
-      :sessions_enabled => true
-    )
 
-    transaction = Appsignal::Transaction.create(
-      SecureRandom.uuid,
-      Appsignal::Transaction::HTTP_REQUEST,
-      request
-    )
+    # @api private
+    module HanamiIntegration
+      def call(env)
+        super
+      ensure
+        transaction = env[::Appsignal::Rack::APPSIGNAL_TRANSACTION]
 
-    begin
-      Appsignal.instrument("process_action.hanami") do
-        super.tap do |response|
-          # TODO: update to response_status or remove:
-          # https://github.com/appsignal/appsignal-ruby/issues/183
-          transaction.set_metadata("status", response.status.to_s)
-        end
+        transaction&.set_action_if_nil(self.class.name)
       end
-    rescue Exception => error # rubocop:disable Lint/RescueException
-      transaction.set_error(error)
-      # TODO: update to response_status or remove:
-      # https://github.com/appsignal/appsignal-ruby/issues/183
-      transaction.set_metadata("status", "500")
-      raise error
-    ensure
-      transaction.set_params_if_nil(request.params.to_h)
-      transaction.set_action_if_nil(self.class.name)
-      transaction.set_metadata("path", request.path)
-      transaction.set_metadata("method", request.request_method)
-      transaction.set_http_or_background_queue_start
-      Appsignal::Transaction.complete_current!
     end
   end
 end

diff --git a/lib/appsignal/rack/hanami_middleware.rb b/lib/appsignal/rack/hanami_middleware.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module Appsignal
+  module Rack
+    # @api private
+    class HanamiMiddleware < AbstractMiddleware
+      def initialize(app, options = {})
+        options[:request_class] ||= ::Hanami::Action::Request
+        options[:params_method] ||= :params
+        options[:instrument_span_name] ||= "process_action.hanami"
+        super
+      end
+
+      private
+
+      def params_for(request)
+        super&.to_h
+      end
+
+      def request_for(env)
+        params = ::Hanami::Action.params_class.new(env)
+        @request_class.new(
+          :env => env,
+          :params => params,
+          :sessions_enabled => true
+        )
+      end
+    end
+  end
+end
diff --git a/spec/lib/appsignal/integrations/hanami_spec.rb b/spec/lib/appsignal/integrations/hanami_spec.rb
@@ -11,17 +11,28 @@
         Appsignal::Integrations::HanamiPlugin.init
       end
 
-      it "prepends the integration to Hanami" do
+      it "prepends the integration to Hanami::Action" do
         allow(Appsignal).to receive(:active?).and_return(true)
         Appsignal::Integrations::HanamiPlugin.init
         expect(::Hanami::Action.included_modules)
           .to include(Appsignal::Integrations::HanamiIntegration)
       end
 
+      it "adds middleware to the Hanami app" do
+        allow(Appsignal).to receive(:active?).and_return(true)
+        Appsignal::Integrations::HanamiPlugin.init
+
+        expect(::Hanami.app.config.middleware.stack[::Hanami::Router::DEFAULT_PREFIX])
+          .to include(
+            [Rack::Events, [[kind_of(Appsignal::Rack::EventHandler)]], nil],
+            [Appsignal::Rack::HanamiMiddleware, [], nil]
+          )
+      end
+
       context "when not active" do
         before { allow(Appsignal).to receive(:active?).and_return(false) }
 
-        it "does not prepend the integration" do
+        it "does not prepend the integration to Hanami::Action" do
           Appsignal::Integrations::HanamiPlugin.init
           expect(::Hanami::Action).to_not receive(:prepend)
             .with(Appsignal::Integrations::HanamiIntegration)
@@ -49,19 +60,10 @@
       end
     end
 
-    describe "Hanami Actions" do
-      let(:env) do
-        Rack::MockRequest.env_for(
-          "/books",
-          "router.params" => router_params,
-          :method => "GET"
-        )
-      end
-      let(:router_params) { { "foo" => "bar", "baz" => "qux" } }
+    describe Appsignal::Integrations::HanamiIntegration do
+      let(:transaction) { http_request_transaction }
       around { |example| keep_transactions { example.run } }
-      before :context do
-        start_agent
-      end
+      before(:context) { start_agent }
       before do
         allow(Appsignal).to receive(:active?).and_return(true)
         Appsignal::Integrations::HanamiPlugin.init
@@ -73,72 +75,26 @@ def make_request(env, app: HanamiApp::Actions::Books::Index)
       end
 
       describe "#call" do
-        it "sets params" do
-          make_request(env)
-
-          expect(last_transaction.to_h).to include(
-            "sample_data" => hash_including(
-              "params" => router_params
-            )
-          )
-        end
-
-        it "sets the namespace and action name" do
-          make_request(env)
-
-          expect(last_transaction.to_h).to include(
-            "namespace" => Appsignal::Transaction::HTTP_REQUEST,
-            "action" => "HanamiApp::Actions::Books::Index"
-          )
-        end
-
-        it "sets the metadata" do
-          make_request(env)
-
-          expect(last_transaction.to_h).to include(
-            "metadata" => hash_including(
-              "status" => "200",
-              "path" => "/books",
-              "method" => "GET"
-            )
-          )
-        end
+        context "without an active transaction" do
+          let(:env) { {} }
 
-        context "with queue start header" do
-          let(:queue_start_time) { fixed_time * 1_000 }
-          before do
-            env["HTTP_X_REQUEST_START"] = "t=#{queue_start_time.to_i}" # in milliseconds
-          end
-
-          it "sets the queue start" do
+          it "does not set the action name" do
             make_request(env)
 
-            expect(last_transaction.ext.queue_start).to eq(queue_start_time)
+            expect(transaction.to_h).to include(
+              "action" => nil
+            )
           end
         end
 
-        context "with error" do
-          before do
-            expect do
-              make_request(env, :app => HanamiApp::Actions::Books::Error)
-            end.to raise_error(ExampleException)
-          end
+        context "with an active transaction" do
+          let(:env) { { Appsignal::Rack::APPSIGNAL_TRANSACTION => transaction } }
 
-          it "records the exception" do
-            expect(last_transaction.to_h).to include(
-              "error" => {
-                "name" => "ExampleException",
-                "message" => "exception message",
-                "backtrace" => kind_of(String)
-              }
-            )
-          end
+          it "sets action name on the transaction" do
+            make_request(env)
 
-          it "sets the status to 500" do
-            expect(last_transaction.to_h).to include(
-              "metadata" => hash_including(
-                "status" => "500"
-              )
+            expect(transaction.to_h).to include(
+              "action" => "HanamiApp::Actions::Books::Index"
             )
           end
         end

diff --git a/spec/lib/appsignal/rack/hanami_middleware_spec.rb b/spec/lib/appsignal/rack/hanami_middleware_spec.rb
@@ -0,0 +1,50 @@
+require "appsignal/rack/hanami_middleware"
+
+if DependencyHelper.hanami2_present?
+  describe Appsignal::Rack::HanamiMiddleware do
+    let(:app) { double(:call => true) }
+    let(:router_params) { { "param1" => "value1", "param2" => "value2" } }
+    let(:env) do
+      Rack::MockRequest.env_for(
+        "/some/path",
+        "router.params" => router_params
+      )
+    end
+    let(:middleware) { Appsignal::Rack::HanamiMiddleware.new(app, {}) }
+
+    before(:context) { start_agent }
+    around { |example| keep_transactions { example.run } }
+
+    def make_request(env)
+      middleware.call(env)
+    end
+
+    context "with params" do
+      it "sets request parameters on the transaction" do
+        make_request(env)
+
+        expect(last_transaction.to_h).to include(
+          "sample_data" => hash_including(
+            "params" => { "param1" => "value1", "param2" => "value2" }
+          )
+        )
+      end
+    end
+
+    it "reports a process_action.hanami event" do
+      make_request(env)
+
+      expect(last_transaction.to_h).to include(
+        "events" => [
+          hash_including(
+            "body" => "",
+            "body_format" => Appsignal::EventFormatter::DEFAULT,
+            "count" => 1,
+            "name" => "process_action.hanami",
+            "title" => ""
+          )
+        ]
+      )
+    end
+  end
+end