Add new Rack instrumentation middleware

.changesets/add-new-recommended-rack-instrumentation-middleware.md

@@ -0,0 +1,18 @@
+---
+bump: minor
+type: add
+---
+
+Add our new recommended Rack instrumentation middleware. If an app is using the `Appsignal::Rack::GenericInstrumentation` middleware, please update it to use `Appsignal::Rack::InstrumentationMiddleware` instead.
+
+This new middleware will not report all requests under the "unknown" action if no action name is set. To set an action name, call the `Appsignal.set_action` helper from the app.
+
+```ruby
+# config.ru
+
+# Setup AppSignal
+
+use Appsignal::Rack::InstrumentationMiddleware
+
+# Run app
+```

.changesets/deprecate-appsignal--rack--genericinstrumentation-middleware.md

@@ -0,0 +1,6 @@
+---
+bump: patch
+type: deprecate
+---
+
+Deprecate the `Appsignal::Rack::GenericInstrumentation` middleware. Use the `Appsignal::Rack::InstrumentationMiddleware` middleware instead. See changelog entry about the `InstrumentationMiddleware`.

lib/appsignal.rb

@@ -324,8 +324,9 @@ def const_missing(name)
 require "appsignal/probes"
 require "appsignal/marker"
 require "appsignal/garbage_collection"
+require "appsignal/rack"
 require "appsignal/rack/abstract_middleware"
-require "appsignal/rack/generic_instrumentation"
+require "appsignal/rack/instrumentation_middleware"
 require "appsignal/rack/event_handler"
 require "appsignal/integrations/railtie" if defined?(::Rails)
 require "appsignal/transaction"

lib/appsignal/rack.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+module Appsignal
+  # @api private
+  module Rack
+    # Alias constants that have moved with a warning message that points to the
+    # place to update the reference.
+    def self.const_missing(name)
+      case name
+      when :GenericInstrumentation
+        require "appsignal/rack/generic_instrumentation"
+
+        callers = caller
+        Appsignal::Utils::StdoutAndLoggerMessage.warning \
+          "The constant Appsignal::Rack::GenericInstrumentation has been deprecated. " \
+            "Please use the new Appsignal::Rack::InstrumentationMiddleware middleware. " \
+            "This new middleware does not default the action name to 'unknown'. " \
+            "Set the action name for the endpoint using the Appsignal.set_action helper. " \
+            "Read our Rack docs for more information " \
+            "https://docs.appsignal.com/ruby/integrations/rack.html " \
+            "Update the constant name to " \
+            "Appsignal::Rack::InstrumentationMiddleware in the following file to " \
+            "remove this message.\n#{callers.first}"
+        # Return the alias so it can't ever get stuck in a recursive loop
+        Appsignal::Rack::GenericInstrumentationAlias
+      else
+        super
+      end
+    end
+  end
+end

lib/appsignal/rack/abstract_middleware.rb

@@ -72,8 +72,8 @@ def call(env)
       # 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.
+      # Either another {AbstractMiddleware} or {EventHandler} is higher in the
+      # stack and will report the exception and complete the transaction.
       #
       # @see {#instrument_app_call_with_exception_handling}
       def instrument_app_call(env)

lib/appsignal/rack/generic_instrumentation.rb

@@ -1,10 +1,8 @@
 # frozen_string_literal: true
 
-require "rack"
-
 module Appsignal
-  # @api private
   module Rack
+    # @api private
     class GenericInstrumentation < AbstractMiddleware
       def initialize(app, options = {})
         options[:instrument_span_name] ||= "process_action.generic"
@@ -16,5 +14,8 @@ def add_transaction_metadata_after(transaction, request)
         transaction.set_action_if_nil("unknown")
       end
     end
+
+    # @api private
+    class GenericInstrumentationAlias < GenericInstrumentation; end
   end
 end

lib/appsignal/rack/instrumentation_middleware.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module Appsignal
+  module Rack
+    # @api public
+    class InstrumentationMiddleware < AbstractMiddleware
+      def initialize(app, options = {})
+        options[:instrument_span_name] ||= "process_request_middleware.rack"
+        super
+      end
+    end
+  end
+end

lib/appsignal/rack/rails_instrumentation.rb

@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-require "rack"
-
 module Appsignal
   module Rack
     # @api private

lib/appsignal/rack/sinatra_instrumentation.rb

@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-require "rack"
-
 module Appsignal
   module Rack
     # Stub old middleware. Prevents Sinatra middleware being loaded twice.

spec/lib/appsignal/rack/generic_instrumentation_spec.rb

@@ -1,28 +1,81 @@
-describe Appsignal::Rack::GenericInstrumentation do
-  let(:app) { double(:call => true) }
-  let(:env) { Rack::MockRequest.env_for("/some/path") }
-  let(:middleware) { Appsignal::Rack::GenericInstrumentation.new(app, {}) }
+describe "Appsignal::Rack::GenericInstrumentation" do
+  describe "Appsignal::Rack::GenericInstrumentation constant" do
+    let(:err_stream) { std_stream }
+    let(:stderr) { err_stream.read }
+    before do
+      if Appsignal::Rack.const_defined?(:GenericInstrumentation)
+        hide_const "Appsignal::Rack::GenericInstrumentation"
+      end
+    end
+
+    it "returns the Rack::GenericInstrumentation constant" do
+      silence do
+        expect(Appsignal::Rack::GenericInstrumentation)
+          .to be(Appsignal::Rack::GenericInstrumentationAlias)
+      end
+    end
 
-  before(:context) { start_agent }
-  around { |example| keep_transactions { example.run } }
+    it "prints a deprecation warning to STDERR" do
+      capture_std_streams(std_stream, err_stream) do
+        Appsignal::Rack::GenericInstrumentation
+      end
 
-  def make_request(env)
-    middleware.call(env)
-  end
+      expect(stderr).to include(
+        "appsignal WARNING: The constant Appsignal::Rack::GenericInstrumentation " \
+          "has been deprecated."
+      )
+    end
 
-  context "without an exception" do
-    it "reports a process_action.generic event" do
-      make_request(env)
+    it "logs a warning" do
+      logs =
+        capture_logs do
+          silence do
+            Appsignal::Rack::GenericInstrumentation
+          end
+        end
 
-      expect(last_transaction).to include_event("name" => "process_action.generic")
+      expect(logs).to contains_log(
+        :warn,
+        "The constant Appsignal::Rack::GenericInstrumentation has been deprecated."
+      )
     end
   end
 
-  context "without action name metadata" do
-    it "reports 'unknown' as the action name" do
-      make_request(env)
+  describe "middleware" do
+    let(:app) { double(:call => true) }
+    let(:env) { Rack::MockRequest.env_for("/some/path") }
+    let(:middleware) { Appsignal::Rack::GenericInstrumentation.new(app, {}) }
+
+    before(:context) { start_agent }
+    around { |example| keep_transactions { example.run } }
+
+    def make_request(env)
+      middleware.call(env)
+    end
+
+    context "without an exception" do
+      it "reports a process_action.generic event" do
+        make_request(env)
+
+        expect(last_transaction).to include_event("name" => "process_action.generic")
+      end
+    end
+
+    context "with action name env" do
+      it "reports the appsignal.action env as the action name" do
+        env["appsignal.action"] = "MyAction"
+        make_request(env)
+
+        expect(last_transaction).to have_action("MyAction")
+      end
+    end
+
+    context "without action name metadata" do
+      it "reports 'unknown' as the action name" do
+        make_request(env)
 
-      expect(last_transaction).to have_action("unknown")
+        expect(last_transaction).to have_action("unknown")
+      end
     end
   end
 end

spec/lib/appsignal/rack/grape_middleware_spec.rb

@@ -5,7 +5,7 @@
     let(:err_stream) { std_stream }
     let(:stderr) { err_stream.read }
 
-    it "returns the Probes constant calling the Minutely constant" do
+    it "returns the Rack::GrapeMiddleware constant calling the Grape::Middleware constant" do
       silence { expect(Appsignal::Grape::Middleware).to be(Appsignal::Rack::GrapeMiddleware) }
     end
 

spec/lib/appsignal/rack/instrumentation_middleware_spec.rb

@@ -0,0 +1,38 @@
+describe Appsignal::Rack::InstrumentationMiddleware do
+  let(:app) { DummyApp.new }
+  let(:env) { Rack::MockRequest.env_for("/some/path") }
+  let(:middleware) { described_class.new(app, {}) }
+
+  before { start_agent }
+  around { |example| keep_transactions { example.run } }
+
+  def make_request(env)
+    middleware.call(env)
+  end
+
+  context "without an exception" do
+    it "reports a process_request_middleware.rack event" do
+      make_request(env)
+
+      expect(last_transaction).to include_event("name" => "process_request_middleware.rack")
+    end
+  end
+
+  context "with custom action name" do
+    let(:app) { DummyApp.new { |_env| Appsignal.set_action("MyAction") } }
+
+    it "reports the custom action name" do
+      make_request(env)
+
+      expect(last_transaction).to have_action("MyAction")
+    end
+  end
+
+  context "without action name metadata" do
+    it "reports no action name" do
+      make_request(env)
+
+      expect(last_transaction).to_not have_action
+    end
+  end
+end