From 527754cdbcdaf80d58d51359017a3af15c86f736 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 19 Jun 2024 14:36:46 +0200 Subject: [PATCH] Supported nested Rack EventHandlers In the case that an app, for whatever reason, has nested Rack EventHandlers, handle this properly. This change makes it so that the EventHandler doesn't create multiple transactions if it's nested inside another EventHandler. It does this by assigning itself an ID (UUID), and adding it to the request environment. Then, on every callback it receives, it checks if it is the first EventHandler that handled the request. Only then does it do any instrumentation. This will help with accidental multiple registrations of the AppSignal Rack EventHandler in one app, like with nested apps. It also opens the door for automatically registering the EventHandler for other gems, like Sinatra. --- .../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