From 9d4ec30145d195168ab95376af978485eafa2da5 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Tue, 5 Nov 2024 13:59:11 +0100 Subject: [PATCH 1/5] Add Hanami 2.2 to the CI build Add Hanami 2.2 to the build matrix so we test against it on the CI. --- .github/workflows/ci.yml | 83 ++++++++++++++++++++++- build_matrix.yml | 6 ++ gemfiles/hanami-2.2.gemfile | 7 ++ spec/support/helpers/dependency_helper.rb | 13 ++-- 4 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 gemfiles/hanami-2.2.gemfile diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c7dbf0ac..a7c084bc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,7 +2,7 @@ # This is a generated file by the `rake build_matrix:github:generate` task. # See `build_matrix.yml` for the build matrix. # Generate this file with `rake build_matrix:github:generate`. -# Generated job count: 124 +# Generated job count: 127 --- name: Ruby gem CI 'on': @@ -264,6 +264,33 @@ jobs: JRUBY_OPTS: '' COV: '1' BUNDLE_GEMFILE: gemfiles/hanami-2.1.gemfile + ruby_3-3-4__hanami-2-2_ubuntu-latest: + name: Ruby 3.3.4 - hanami-2.2 + needs: ruby_3-3-4_ubuntu-latest + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + - name: Install Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: 3.3.4 + bundler-cache: true + - name: Install gem extension + run: "./script/bundler_wrapper exec rake extension:install" + - name: Print extension install report + run: "[ -e ext/install.report ] && cat ext/install.report || echo 'No ext/install.report + file found'" + - name: Print Makefile log file + run: "[ -f ext/mkmf.log ] && cat ext/mkmf.log || echo 'No ext/mkmf.log file + found'" + - name: Run tests + run: "./script/bundler_wrapper exec rake test" + env: + RAILS_ENV: test + JRUBY_OPTS: '' + COV: '1' + BUNDLE_GEMFILE: gemfiles/hanami-2.2.gemfile ruby_3-3-4__http5_ubuntu-latest: name: Ruby 3.3.4 - http5 needs: ruby_3-3-4_ubuntu-latest @@ -916,6 +943,33 @@ jobs: JRUBY_OPTS: '' COV: '1' BUNDLE_GEMFILE: gemfiles/hanami-2.1.gemfile + ruby_3-2-5__hanami-2-2_ubuntu-latest: + name: Ruby 3.2.5 - hanami-2.2 + needs: ruby_3-2-5_ubuntu-latest + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + - name: Install Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: 3.2.5 + bundler-cache: true + - name: Install gem extension + run: "./script/bundler_wrapper exec rake extension:install" + - name: Print extension install report + run: "[ -e ext/install.report ] && cat ext/install.report || echo 'No ext/install.report + file found'" + - name: Print Makefile log file + run: "[ -f ext/mkmf.log ] && cat ext/mkmf.log || echo 'No ext/mkmf.log file + found'" + - name: Run tests + run: "./script/bundler_wrapper exec rake test" + env: + RAILS_ENV: test + JRUBY_OPTS: '' + COV: '1' + BUNDLE_GEMFILE: gemfiles/hanami-2.2.gemfile ruby_3-2-5__http5_ubuntu-latest: name: Ruby 3.2.5 - http5 needs: ruby_3-2-5_ubuntu-latest @@ -1568,6 +1622,33 @@ jobs: JRUBY_OPTS: '' COV: '1' BUNDLE_GEMFILE: gemfiles/hanami-2.1.gemfile + ruby_3-1-6__hanami-2-2_ubuntu-latest: + name: Ruby 3.1.6 - hanami-2.2 + needs: ruby_3-1-6_ubuntu-latest + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + - name: Install Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: 3.1.6 + bundler-cache: true + - name: Install gem extension + run: "./script/bundler_wrapper exec rake extension:install" + - name: Print extension install report + run: "[ -e ext/install.report ] && cat ext/install.report || echo 'No ext/install.report + file found'" + - name: Print Makefile log file + run: "[ -f ext/mkmf.log ] && cat ext/mkmf.log || echo 'No ext/mkmf.log file + found'" + - name: Run tests + run: "./script/bundler_wrapper exec rake test" + env: + RAILS_ENV: test + JRUBY_OPTS: '' + COV: '1' + BUNDLE_GEMFILE: gemfiles/hanami-2.2.gemfile ruby_3-1-6__http5_ubuntu-latest: name: Ruby 3.1.6 - http5 needs: ruby_3-1-6_ubuntu-latest diff --git a/build_matrix.yml b/build_matrix.yml index 8b984c04..ac02c129 100644 --- a/build_matrix.yml +++ b/build_matrix.yml @@ -121,6 +121,12 @@ matrix: - "3.2.5" - "3.1.6" - "3.0.7" + - gem: "hanami-2.2" + only: + ruby: + - "3.3.4" + - "3.2.5" + - "3.1.6" - gem: "http5" - gem: "padrino" - gem: "psych-3" diff --git a/gemfiles/hanami-2.2.gemfile b/gemfiles/hanami-2.2.gemfile new file mode 100644 index 00000000..ce08093d --- /dev/null +++ b/gemfiles/hanami-2.2.gemfile @@ -0,0 +1,7 @@ +source "https://rubygems.org" + +gem "hanami", "~> 2.2.0" +gem "hanami-controller", "~> 2.2.0" +gem "hanami-router", "~> 2.2.0" + +gemspec :path => "../" diff --git a/spec/support/helpers/dependency_helper.rb b/spec/support/helpers/dependency_helper.rb index 429b83a0..ed2bd821 100644 --- a/spec/support/helpers/dependency_helper.rb +++ b/spec/support/helpers/dependency_helper.rb @@ -136,17 +136,22 @@ def hanami_present? dependency_present? "hanami" end + def hanami2_present? + hanami_present? && Gem.loaded_specs["hanami"].version >= Gem::Version.new("2.0") + end + def hanami2_1_present? hanami_present? && Gem::Version.new(::Hanami::VERSION) >= Gem::Version.new("2.1.0") end - def dry_monitor_present? - dependency_present? "dry-monitor" + def hanami2_2_present? + hanami_present? && + Gem::Version.new(::Hanami::VERSION) >= Gem::Version.new("2.2.0") end - def hanami2_present? - hanami_present? && Gem.loaded_specs["hanami"].version >= Gem::Version.new("2.0") + def dry_monitor_present? + dependency_present? "dry-monitor" end def dependency_present?(dependency_file) From c68485043feee13a49400f57c9a77d48f670f0f2 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Tue, 5 Nov 2024 14:10:32 +0100 Subject: [PATCH 2/5] Set Hanami action name from middleware Remove the need for the monkeypatch in the Hanami loader and fetch the Hanami action instance from the request env to determine the action name. I'll remove the monkeypatch in the next commit. This only works on Hanami 2.2 and newer. Part of #911 --- ...ad-hanami-action-name-without-monkeypatch.md | 6 ++++++ lib/appsignal/rack/hanami_middleware.rb | 12 ++++++++++++ .../appsignal/rack/hanami_middleware_spec.rb | 17 +++++++++++++++++ 3 files changed, 35 insertions(+) create mode 100644 .changesets/read-hanami-action-name-without-monkeypatch.md diff --git a/.changesets/read-hanami-action-name-without-monkeypatch.md b/.changesets/read-hanami-action-name-without-monkeypatch.md new file mode 100644 index 00000000..4db8350f --- /dev/null +++ b/.changesets/read-hanami-action-name-without-monkeypatch.md @@ -0,0 +1,6 @@ +--- +bump: patch +type: change +--- + +Read the Hanami Action name without metaprogramming in Hanami 2.2 and newer. This makes our instrumentation more stable whenever something changes in future Hanami releases. diff --git a/lib/appsignal/rack/hanami_middleware.rb b/lib/appsignal/rack/hanami_middleware.rb index 1cad0abd..d5178069 100644 --- a/lib/appsignal/rack/hanami_middleware.rb +++ b/lib/appsignal/rack/hanami_middleware.rb @@ -12,13 +12,25 @@ def initialize(app, options = {}) private + HANAMI_ACTION_INSTANCE = "hanami.action_instance" + def add_transaction_metadata_after(transaction, request) + action_name = fetch_hanami_action(request.env) + transaction.set_action_if_nil(action_name) if action_name transaction.add_params { params_for(request) } end def params_for(request) ::Hanami::Action.params_class.new(request.env).to_h end + + def fetch_hanami_action(env) + # This env key is available in Hanami 2.2+ + action_instance = env.fetch(HANAMI_ACTION_INSTANCE, nil) + return unless action_instance + + action_instance.class.name + end end end end diff --git a/spec/lib/appsignal/rack/hanami_middleware_spec.rb b/spec/lib/appsignal/rack/hanami_middleware_spec.rb index 2a9687ee..5283a12f 100644 --- a/spec/lib/appsignal/rack/hanami_middleware_spec.rb +++ b/spec/lib/appsignal/rack/hanami_middleware_spec.rb @@ -16,6 +16,15 @@ around { |example| keep_transactions { example.run } } def make_request(env) + if DependencyHelper.hanami2_2_present? + instance = + Class.new do + def self.name + "HanamiApp::Actions::Books::Index" + end + end.new + env["hanami.action_instance"] = instance + end middleware.call(env) end @@ -32,5 +41,13 @@ def make_request(env) expect(last_transaction).to include_event("name" => "process_action.hanami") end + + if DependencyHelper.hanami2_2_present? + it "sets action name on the transaction" do + make_request(env) + + expect(last_transaction).to have_action("HanamiApp::Actions::Books::Index") + end + end end end From dee86155599e800c12dff87a2763395623786464 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Tue, 5 Nov 2024 14:11:59 +0100 Subject: [PATCH 3/5] Disable Hanami monkeypatch on Hanami 2.2 In the previous commit we started reading the action name from the Hanami Action instance on the request env. Disable the monkeypatch on Hanami 2.2+ so it's not doing the same thing twice. We'll leave the monkeypatch for now, but eventually we should be able to remove it. Like on the next major Hanami version or after some years. Part of #911 --- lib/appsignal/loaders/hanami.rb | 3 +++ spec/lib/appsignal/loaders/hanami_spec.rb | 28 ++++++++++++++++++----- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/lib/appsignal/loaders/hanami.rb b/lib/appsignal/loaders/hanami.rb index e0862ad3..6356bbd6 100644 --- a/lib/appsignal/loaders/hanami.rb +++ b/lib/appsignal/loaders/hanami.rb @@ -23,9 +23,12 @@ def on_start ) hanami_app_config.middleware.use(Appsignal::Rack::HanamiMiddleware) + return unless Gem::Version.new(Hanami::VERSION) < Gem::Version.new("2.2.0") + ::Hanami::Action.prepend Appsignal::Loaders::HanamiLoader::HanamiIntegration end + # Legacy instrumentation to set the action name in Hanami apps older than Hanami 2.2 module HanamiIntegration def call(env) super diff --git a/spec/lib/appsignal/loaders/hanami_spec.rb b/spec/lib/appsignal/loaders/hanami_spec.rb index fc794214..de14fde2 100644 --- a/spec/lib/appsignal/loaders/hanami_spec.rb +++ b/spec/lib/appsignal/loaders/hanami_spec.rb @@ -37,9 +37,16 @@ def uninstall_hanami_middleware ) end - it "prepends the integration to Hanami::Action" do - expect(::Hanami::Action) - .to have_received(:prepend).with(Appsignal::Loaders::HanamiLoader::HanamiIntegration) + if DependencyHelper.hanami2_2_present? + it "does not prepend a monkeypatch integration to Hanami::Action" do + expect(::Hanami::Action).to_not have_received(:prepend) + .with(Appsignal::Loaders::HanamiLoader::HanamiIntegration) + end + else + it "prepends the integration to Hanami::Action" do + expect(::Hanami::Action).to have_received(:prepend) + .with(Appsignal::Loaders::HanamiLoader::HanamiIntegration) + end end def hanami_middleware_options @@ -80,10 +87,19 @@ def make_request(env) context "with an active transaction" do let(:env) { { Appsignal::Rack::APPSIGNAL_TRANSACTION => transaction } } - it "sets action name on the transaction" do - make_request(env) + if DependencyHelper.hanami2_2_present? + it "does not set an action name on the transaction" do + # This is done by the middleware instead + make_request(env) + + expect(transaction).to_not have_action + end + else + it "sets action name on the transaction" do + make_request(env) - expect(transaction).to have_action("HanamiApp::Actions::Books::Index") + expect(transaction).to have_action("HanamiApp::Actions::Books::Index") + end end end end From ca22ed54de513773c5ee387f28ad98ee8f301627 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Tue, 5 Nov 2024 14:48:26 +0100 Subject: [PATCH 4/5] Fix request parameter reporting for Hanami 2.2 In Hanami 2.2 they removed the params class. We can't access it that way anymore. This "router.params" env key contains both query params and POST request payload params. This approach seems to be least intrusive and less prone to breaking in future version of Hanami. --- ...request-parameter-reporting-for-hanami-2-2.md | 6 ++++++ lib/appsignal/rack/hanami_middleware.rb | 3 ++- .../lib/appsignal/rack/hanami_middleware_spec.rb | 16 ++++++++++++++-- 3 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 .changesets/fix-request-parameter-reporting-for-hanami-2-2.md diff --git a/.changesets/fix-request-parameter-reporting-for-hanami-2-2.md b/.changesets/fix-request-parameter-reporting-for-hanami-2-2.md new file mode 100644 index 00000000..dcec61a7 --- /dev/null +++ b/.changesets/fix-request-parameter-reporting-for-hanami-2-2.md @@ -0,0 +1,6 @@ +--- +bump: patch +type: fix +--- + +Fix request parameter reporting for Hanami 2.2. diff --git a/lib/appsignal/rack/hanami_middleware.rb b/lib/appsignal/rack/hanami_middleware.rb index d5178069..f3ab8c5c 100644 --- a/lib/appsignal/rack/hanami_middleware.rb +++ b/lib/appsignal/rack/hanami_middleware.rb @@ -13,6 +13,7 @@ def initialize(app, options = {}) private HANAMI_ACTION_INSTANCE = "hanami.action_instance" + ROUTER_PARAMS = "router.params" def add_transaction_metadata_after(transaction, request) action_name = fetch_hanami_action(request.env) @@ -21,7 +22,7 @@ def add_transaction_metadata_after(transaction, request) end def params_for(request) - ::Hanami::Action.params_class.new(request.env).to_h + request.env.fetch(ROUTER_PARAMS, nil) end def fetch_hanami_action(env) diff --git a/spec/lib/appsignal/rack/hanami_middleware_spec.rb b/spec/lib/appsignal/rack/hanami_middleware_spec.rb index 5283a12f..eccd441f 100644 --- a/spec/lib/appsignal/rack/hanami_middleware_spec.rb +++ b/spec/lib/appsignal/rack/hanami_middleware_spec.rb @@ -3,11 +3,13 @@ if DependencyHelper.hanami2_present? describe Appsignal::Rack::HanamiMiddleware do let(:app) { double(:call => true) } - let(:router_params) { { "param1" => "value1", "param2" => "value2" } } + let(:router_params) { nil } let(:env) do + options = {} + options["router.params"] = router_params if router_params Rack::MockRequest.env_for( "/some/path", - "router.params" => router_params + options ) end let(:middleware) { Appsignal::Rack::HanamiMiddleware.new(app, {}) } @@ -28,7 +30,17 @@ def self.name middleware.call(env) end + context "without params" do + it "sets no request parameters on the transaction" do + make_request(env) + + expect(last_transaction).to_not include_params + end + end + context "with params" do + let(:router_params) { { "param1" => "value1", "param2" => "value2" } } + it "sets request parameters on the transaction" do make_request(env) From f1500987b7b73ab2873202ea7301f2cfb19acdc7 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Tue, 5 Nov 2024 15:13:37 +0100 Subject: [PATCH 5/5] Ignore Hanami errors by default Configure the loader to ignore some Hanami built-in errors by default so they don't confuse our users. --- .changesets/ignore-hanami-errors-by-default.md | 13 +++++++++++++ lib/appsignal/loaders/hanami.rb | 6 +++++- spec/lib/appsignal/loaders/hanami_spec.rb | 7 ++++++- 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 .changesets/ignore-hanami-errors-by-default.md diff --git a/.changesets/ignore-hanami-errors-by-default.md b/.changesets/ignore-hanami-errors-by-default.md new file mode 100644 index 00000000..6df18970 --- /dev/null +++ b/.changesets/ignore-hanami-errors-by-default.md @@ -0,0 +1,13 @@ +--- +bump: patch +type: change +--- + +Ignore these Hanami errors by default: + +- Hanami::Router::NotAllowedError (for example: sending a GET request to POST endpoint) +- Hanami::Router::NotFoundError + +They are usually errors you don't want to be notified about, so we ignore them by default now. + +Customize the `ignore_errors` config option to continue receiving these errors. diff --git a/lib/appsignal/loaders/hanami.rb b/lib/appsignal/loaders/hanami.rb index 6356bbd6..976b642f 100644 --- a/lib/appsignal/loaders/hanami.rb +++ b/lib/appsignal/loaders/hanami.rb @@ -9,7 +9,11 @@ def on_load hanami_app_config = ::Hanami.app.config register_config_defaults( :root_path => hanami_app_config.root.to_s, - :env => hanami_app_config.env + :env => hanami_app_config.env, + :ignore_errors => [ + "Hanami::Router::NotAllowedError", + "Hanami::Router::NotFoundError" + ] ) end diff --git a/spec/lib/appsignal/loaders/hanami_spec.rb b/spec/lib/appsignal/loaders/hanami_spec.rb index de14fde2..4a09dc72 100644 --- a/spec/lib/appsignal/loaders/hanami_spec.rb +++ b/spec/lib/appsignal/loaders/hanami_spec.rb @@ -8,7 +8,12 @@ :name => :hanami, :root_path => Dir.pwd, :env => :test, - :options => {} + :options => { + :ignore_errors => [ + "Hanami::Router::NotAllowedError", + "Hanami::Router::NotFoundError" + ] + } ) end end