Merge pull request #1113 from appsignal/hanami-refactor

Update Hanami to use Rack middleware

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

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

spec/lib/appsignal/integrations/hanami_spec.rb

@@ -4,28 +4,63 @@
   describe "Hanami integration" do
     require "appsignal/integrations/hanami"
 
+    before do
+      uninstall_hanami_middleware
+    end
+
+    def uninstall_hanami_middleware
+      middleware_stack = ::Hanami.app.config.middleware.stack[::Hanami::Router::DEFAULT_PREFIX]
+      middleware_stack.delete_if do |middleware|
+        middleware.first == Appsignal::Rack::HanamiMiddleware ||
+          middleware.first == Rack::Events
+      end
+    end
+
     describe Appsignal::Integrations::HanamiPlugin do
       it "starts AppSignal on init" do
         expect(Appsignal).to receive(:start)
         expect(Appsignal).to receive(:start_logger)
         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)
         end
+
+        it "does not add the middleware to the Hanami app" do
+          Appsignal::Integrations::HanamiPlugin.init
+
+          middleware_stack = ::Hanami.app.config.middleware.stack[::Hanami::Router::DEFAULT_PREFIX]
+          expect(middleware_stack).to_not include(
+            [Rack::Events, [[kind_of(Appsignal::Rack::EventHandler)]], nil]
+          )
+          expect(middleware_stack).to_not include(
+            [Appsignal::Rack::HanamiMiddleware, [], nil]
+          )
+        end
       end
 
       context "when APPSIGNAL_APP_ENV ENV var is provided" do
@@ -49,19 +84,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 +99,26 @@ def make_request(env, app: HanamiApp::Actions::Books::Index)
       end
 
       describe "#call" do
-        it "sets params" do
-          make_request(env)
+        context "without an active transaction" do
+          let(: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 "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