Improve supported for nested Rack middlewares (#1100)

When more of our Rack, Rails and Sinatra middlewares are nested in one
app, improve reporting for those requests. This follows PRs #1089 for
Rails and #1097 for Sinatra. This change improves this reporting for
Rack apps using the GenericInstrumentation middleware. Other than
supporting this scenario better, no one should notice this change.

## Refactor details

I've refactored the SinatraInstrumentation middleware to inherit from a
new AbstractMiddleware which includes much of the behavior from the
GenericInstrumentation and previous SinatraInstrumentation
implementations.
All the logic for nested instrumentation middlewares are now handled in
this AbstractMiddleware.

Subclasses of AbstractMiddleware can set their library's specific
metadata using the `add_transaction_metadata_before` and
`add_transaction_metadata_after` methods, along with specifying the
`request_class`, `params_method` and `instrument_span_name` settings.

This already works with the EventHandler, as both set the transaction on
the request env to detect nested instrumentation. An app could add both
the EventHandler and a subclass of the AbstractMiddleware, and it would
report the request transaction properly.

## GenericInstrumentation

I've kept the GenericInstrumentation middleware. The only thing it
really does different that we don't want to put in the
AbstractMiddleware is the fallback to the "unknown" action name. I
didn't want to break existing behavior, so that is all it still does.

If we move the GenericInstrumentation action name fallback to the
AbstractMiddleware, we may be reporting more requests than we want for
other middlewares that inherit from it. For example, for Rails app, I
also want to use this AbstractMiddleware, and it relies on asset
requests having no action name so the extension can drop them. That way
we don't report transactions for asset requests.

## Next steps

If this change is approved, I will update the other Rack
instrumentations, like Rails, Grape and Padrino.

.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) &&