From 661b8e08de962e8f95326f0bbc9c0061b8cc0a62 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Thu, 20 Jun 2024 18:27:09 +0200 Subject: [PATCH] Supported nested Rack EventHandlers (#1101) --- .../support-nested-rack-eventhandlers.md | 6 +++ lib/appsignal/rack/event_handler.rb | 21 ++++++++++ spec/lib/appsignal/rack/event_handler_spec.rb | 40 +++++++++++++++++-- 3 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 .changesets/support-nested-rack-eventhandlers.md diff --git a/.changesets/support-nested-rack-eventhandlers.md b/.changesets/support-nested-rack-eventhandlers.md new file mode 100644 index 000000000..3b4b3117b --- /dev/null +++ b/.changesets/support-nested-rack-eventhandlers.md @@ -0,0 +1,6 @@ +--- +bump: patch +type: change +--- + +Support apps that have multiple Appsignal::Rack::EventHandler-s in the middleware stack. diff --git a/lib/appsignal/rack/event_handler.rb b/lib/appsignal/rack/event_handler.rb index 5ed08d549..a403068af 100644 --- a/lib/appsignal/rack/event_handler.rb +++ b/lib/appsignal/rack/event_handler.rb @@ -3,6 +3,7 @@ module Appsignal module Rack APPSIGNAL_TRANSACTION = "appsignal.transaction" + APPSIGNAL_EVENT_HANDLER_ID = "appsignal.event_handler_id" RACK_AFTER_REPLY = "rack.after_reply" class EventHandler @@ -16,8 +17,22 @@ def self.safe_execution(name) ) end + attr_reader :id + + def initialize + @id = SecureRandom.uuid + end + + def request_handler?(given_id) + id == given_id + end + def on_start(request, _response) + event_handler = self self.class.safe_execution("Appsignal::Rack::EventHandler#on_start") do + request.env[APPSIGNAL_EVENT_HANDLER_ID] ||= id + return unless request_handler?(request.env[APPSIGNAL_EVENT_HANDLER_ID]) + transaction = Appsignal::Transaction.create( SecureRandom.uuid, Appsignal::Transaction::HTTP_REQUEST, @@ -29,6 +44,8 @@ def on_start(request, _response) request.env[RACK_AFTER_REPLY] << proc do Appsignal::Rack::EventHandler .safe_execution("Appsignal::Rack::EventHandler's after_reply") do + next unless event_handler.request_handler?(request.env[APPSIGNAL_EVENT_HANDLER_ID]) + transaction.finish_event("process_request.rack", "", "") transaction.set_http_or_background_queue_start @@ -48,6 +65,8 @@ def on_start(request, _response) def on_error(request, _response, error) self.class.safe_execution("Appsignal::Rack::EventHandler#on_error") do + return unless request_handler?(request.env[APPSIGNAL_EVENT_HANDLER_ID]) + transaction = request.env[APPSIGNAL_TRANSACTION] return unless transaction @@ -57,6 +76,8 @@ def on_error(request, _response, error) def on_finish(request, response) self.class.safe_execution("Appsignal::Rack::EventHandler#on_finish") do + return unless request_handler?(request.env[APPSIGNAL_EVENT_HANDLER_ID]) + transaction = request.env[APPSIGNAL_TRANSACTION] return unless transaction diff --git a/spec/lib/appsignal/rack/event_handler_spec.rb b/spec/lib/appsignal/rack/event_handler_spec.rb index beb42627c..81851377c 100644 --- a/spec/lib/appsignal/rack/event_handler_spec.rb +++ b/spec/lib/appsignal/rack/event_handler_spec.rb @@ -11,6 +11,7 @@ let(:response) { nil } let(:log_stream) { StringIO.new } let(:log) { log_contents(log_stream) } + let(:event_handler_instance) { described_class.new } before do start_agent Appsignal.internal_logger = test_logger(log_stream) @@ -18,7 +19,7 @@ around { |example| keep_transactions { example.run } } def on_start - described_class.new.on_start(request, response) + event_handler_instance.on_start(request, response) end describe "#on_start" do @@ -34,6 +35,14 @@ def on_start expect(Appsignal::Transaction.current).to eq(last_transaction) end + context "when the handler is nested in another EventHandler" do + it "does not create a new transaction in the nested EventHandler" do + on_start + expect { described_class.new.on_start(request, response) } + .to_not(change { created_transactions.length }) + end + end + it "registers transaction on the request environment" do on_start @@ -87,7 +96,7 @@ def on_start describe "#on_error" do def on_error(error) - described_class.new.on_error(request, response, error) + event_handler_instance.on_error(request, response, error) end it "reports the error" do @@ -103,6 +112,15 @@ def on_error(error) ) end + context "when the handler is nested in another EventHandler" do + it "does not report the error on the transaction" do + on_start + described_class.new.on_error(request, response, ExampleStandardError.new("the error")) + + expect(last_transaction.to_h).to include("error" => nil) + end + end + it "logs an error in case of an internal error" do on_start @@ -122,7 +140,7 @@ def on_error(error) let(:response) { Rack::Events::BufferedResponse.new(200, {}, ["body"]) } def on_finish - described_class.new.on_finish(request, response) + event_handler_instance.on_finish(request, response) end it "doesn't do anything without a transaction" do @@ -155,6 +173,22 @@ def on_finish ) ) expect(last_transaction.ext.queue_start).to eq(queue_start_time) + expect(last_transaction).to be_completed + end + + context "when the handler is nested in another EventHandler" do + it "does not complete the transaction" do + on_start + described_class.new.on_finish(request, response) + + expect(last_transaction.to_h).to include( + "action" => nil, + "metadata" => {}, + "sample_data" => {}, + "events" => [] + ) + expect(last_transaction).to_not be_completed + end end it "doesn't set the action name if already set" do