Improve support for nested Rack middlewares

.changesets/support-nested-genericinstrumentation-middleware.md

@@ -0,0 +1,6 @@
+---
+bump: patch
+type: change
+---
+
+Improve support for instrumentation of nested pure Rack and Sinatra apps. It will now report more of the request's duration and events. This also improves support for apps that have multiple Rack GenericInstrumentation or SinatraInstrumentation middlewares.

lib/appsignal.rb

@@ -326,6 +326,7 @@ def const_missing(name)
 require "appsignal/integrations/railtie" if defined?(::Rails)
 require "appsignal/transaction"
 require "appsignal/version"
+require "appsignal/rack/abstract_middleware"
 require "appsignal/rack/generic_instrumentation"
 require "appsignal/rack/event_handler"
 require "appsignal/transmitter"

lib/appsignal/rack/abstract_middleware.rb

@@ -0,0 +1,127 @@
+# frozen_string_literal: true
+
+require "rack"
+
+module Appsignal
+  # @api private
+  module Rack
+    class AbstractMiddleware
+      def initialize(app, options = {})
+        Appsignal.internal_logger.debug "Initializing #{self.class}"
+        @app = app
+        @options = options
+        @request_class = options.fetch(:request_class, ::Rack::Request)
+        @params_method = options.fetch(:params_method, :params)
+        @instrument_span_name = options.fetch(:instrument_span_name, "process.abstract")
+      end
+
+      def call(env)
+        if Appsignal.active?
+          request = request_for(env)
+          # Supported nested instrumentation middlewares by checking if there's
+          # already a transaction active for this request.
+          wrapped_instrumentation = env.key?(Appsignal::Rack::APPSIGNAL_TRANSACTION)
+          transaction =
+            if wrapped_instrumentation
+              env[Appsignal::Rack::APPSIGNAL_TRANSACTION]
+            else
+              Appsignal::Transaction.create(
+                SecureRandom.uuid,
+                Appsignal::Transaction::HTTP_REQUEST,
+                request
+              )
+            end
+
+          add_transaction_metadata_before(transaction, request)
+          if wrapped_instrumentation
+            instrument_wrapped_request(request, transaction)
+          else
+            # Set transaction on the request environment so other nested
+            # middleware can detect if there is parent instrumentation
+            # middleware active.
+            env[Appsignal::Rack::APPSIGNAL_TRANSACTION] = transaction
+            instrument_request(request, transaction)
+          end
+        else
+          @app.call(env)
+        end
+      end
+
+      private
+
+      # Another instrumentation middleware is active earlier in the stack, so
+      # don't report any exceptions here, the top instrumentation middleware
+      # will be the one reporting the exception.
+      #
+      # Either another {GenericInstrumentation} or {EventHandler} is higher in
+      # the stack and will report the exception and complete the transaction.
+      #
+      # @see {#instrument_request}
+      def instrument_wrapped_request(request, transaction)
+        instrument_app_call(request.env)
+      ensure
+        add_transaction_metadata_after(transaction, request)
+      end
+
+      # Instrument the request fully. This is used by the top instrumentation
+      # middleware in the middleware stack. Unlike
+      # {#instrument_wrapped_request} this will report any exceptions being
+      # raised.
+      #
+      # @see {#instrument_wrapped_request}
+      def instrument_request(request, transaction)
+        instrument_app_call(request.env)
+      rescue Exception => error # rubocop:disable Lint/RescueException
+        transaction.set_error(error)
+        raise error
+      ensure
+        add_transaction_metadata_after(transaction, request)
+
+        # Complete transaction because this is the top instrumentation middleware.
+        Appsignal::Transaction.complete_current!
+      end
+
+      def instrument_app_call(env)
+        Appsignal.instrument(@instrument_span_name) do
+          @app.call(env)
+        end
+      end
+
+      # Add metadata to the transaction based on the request environment.
+      # Override this method to set metadata before the app is called.
+      # Call `super` to also include the default set metadata.
+      def add_transaction_metadata_before(transaction, request)
+        params = params_for(request)
+        transaction.params = params if params
+      end
+
+      # Add metadata to the transaction based on the request environment.
+      # Override this method to set metadata after the app is called.
+      # Call `super` to also include the default set metadata.
+      def add_transaction_metadata_after(transaction, request)
+        default_action =
+          request.env["appsignal.route"] || request.env["appsignal.action"]
+        transaction.set_action_if_nil(default_action)
+        transaction.set_metadata("path", request.path)
+        transaction.set_metadata("method", request.request_method)
+        transaction.set_http_or_background_queue_start
+      end
+
+      def params_for(request)
+        return unless request.respond_to?(@params_method)
+
+        request.send(@params_method)
+      rescue => error
+        # Getting params from the request has been know to fail.
+        Appsignal.internal_logger.debug(
+          "Exception while getting params in #{self.class} from '#{@params_method}': #{error}"
+        )
+        nil
+      end
+
+      def request_for(env)
+        @request_class.new(env)
+      end
+    end
+  end
+end

lib/appsignal/rack/generic_instrumentation.rb

@@ -5,42 +5,15 @@
 module Appsignal
   # @api private
   module Rack
-    class GenericInstrumentation
+    class GenericInstrumentation < AbstractMiddleware
       def initialize(app, options = {})
-        Appsignal.internal_logger.debug "Initializing Appsignal::Rack::GenericInstrumentation"
-        @app = app
-        @options = options
+        options[:instrument_span_name] ||= "process_action.generic"
+        super
       end
 
-      def call(env)
-        if Appsignal.active?
-          call_with_appsignal_monitoring(env)
-        else
-          @app.call(env)
-        end
-      end
-
-      def call_with_appsignal_monitoring(env)
-        request = ::Rack::Request.new(env)
-        transaction = Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::HTTP_REQUEST,
-          request
-        )
-        begin
-          Appsignal.instrument("process_action.generic") do
-            @app.call(env)
-          end
-        rescue Exception => error # rubocop:disable Lint/RescueException
-          transaction.set_error(error)
-          raise error
-        ensure
-          transaction.set_action_if_nil(env["appsignal.route"] || "unknown")
-          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
+      def add_transaction_metadata_after(transaction, request)
+        super
+        transaction.set_action_if_nil("unknown")
       end
     end
   end

lib/appsignal/rack/sinatra_instrumentation.rb

@@ -28,64 +28,29 @@ def settings
       end
     end
 
-    class SinatraBaseInstrumentation
+    class SinatraBaseInstrumentation < AbstractMiddleware
       attr_reader :raise_errors_on
 
       def initialize(app, options = {})
-        Appsignal.internal_logger.debug "Initializing Appsignal::Rack::SinatraBaseInstrumentation"
-        @app = app
-        @options = options
-        @raise_errors_on = raise_errors?(@app)
-      end
-
-      def call(env)
-        if Appsignal.active?
-          call_with_appsignal_monitoring(env)
-        else
-          @app.call(env)
-        end
+        options[:request_class] ||= Sinatra::Request
+        options[:params_method] ||= :params
+        options[:instrument_span_name] ||= "process_action.sinatra"
+        super
+        @raise_errors_on = raise_errors?(app)
       end
 
       private
 
-      def call_with_appsignal_monitoring(env)
-        request = @options.fetch(:request_class, Sinatra::Request).new(env)
-        has_parent_instrumentation = env.key?(Appsignal::Rack::APPSIGNAL_TRANSACTION)
-        transaction =
-          if has_parent_instrumentation
-            env[Appsignal::Rack::APPSIGNAL_TRANSACTION]
-          else
-            Appsignal::Transaction.create(
-              SecureRandom.uuid,
-              Appsignal::Transaction::HTTP_REQUEST,
-              request
-            )
-          end
-
-        begin
-          params = fetch_params(request, @options.fetch(:params_method, :params))
-          transaction.params = params if params
-
-          Appsignal.instrument("process_action.sinatra") do
-            @app.call(env)
-          end
-        rescue Exception => error # rubocop:disable Lint/RescueException
-          transaction.set_error(error)
-          raise error
-        ensure
-          # If raise_error is off versions of Sinatra don't raise errors, but store
-          # them in the sinatra.error env var.
-          if !raise_errors_on && env["sinatra.error"] && !env["sinatra.skip_appsignal_error"]
-            transaction.set_error(env["sinatra.error"])
-          end
-          transaction.set_action_if_nil(action_name(env))
-          transaction.set_metadata("path", request.path)
-          transaction.set_metadata("method", request.request_method)
-          transaction.set_http_or_background_queue_start
-
-          # Only close if this middleware created the instrumentation
-          Appsignal::Transaction.complete_current! unless has_parent_instrumentation
+      def add_transaction_metadata_after(transaction, request)
+        env = request.env
+        transaction.set_action_if_nil(action_name(env))
+        # If raise_error is off versions of Sinatra don't raise errors, but store
+        # them in the sinatra.error env var.
+        if !raise_errors_on && env["sinatra.error"] && !env["sinatra.skip_appsignal_error"]
+          transaction.set_error(env["sinatra.error"])
         end
+
+        super
       end
 
       def action_name(env)
@@ -99,16 +64,6 @@ def action_name(env)
         end
       end
 
-      def fetch_params(request, params_method)
-        return unless request.respond_to?(params_method)
-
-        request.send(params_method)
-      rescue => error
-        # Getting params from the request has been know to fail.
-        Appsignal.internal_logger.debug "Exception while getting Sinatra params: #{error}"
-        nil
-      end
-
       def raise_errors?(app)
         app.respond_to?(:settings) &&
           app.settings.respond_to?(:raise_errors) &&