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 new file mode 100644 index 000000000..8b3608d55 --- /dev/null +++ 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 index fc96935a9..989ea8fdb 100644 --- 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 new file mode 100644 index 000000000..9def18ff4 --- /dev/null +++ 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 index 90a99538b..142efb71b 100644 --- a/spec/lib/appsignal/integrations/hanami_spec.rb +++ b/spec/lib/appsignal/integrations/hanami_spec.rb @@ -4,6 +4,18 @@ 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) @@ -11,21 +23,44 @@ 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 diff --git a/spec/lib/appsignal/rack/hanami_middleware_spec.rb b/spec/lib/appsignal/rack/hanami_middleware_spec.rb new file mode 100644 index 000000000..83834e0ae --- /dev/null +++ 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