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

Author: tombruijn

.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.

lib/appsignal/integrations/hanami.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "appsignal"
+require "appsignal/rack/hanami_middleware"
 
 module Appsignal
   module Integrations
@@ -19,48 +20,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
-    )
+    module HanamiIntegration
+      def call(env)
+        super
+      ensure
+        transaction = env[::Appsignal::Rack::APPSIGNAL_TRANSACTION]
+        return unless 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

lib/appsignal/rack/hanami_middleware.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+module Appsignal
+  module Rack
+    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

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

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