From 2ad6fc14adaa17e99c5f2d0dcf65cdde994bfab0 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Tue, 2 Jul 2024 17:10:20 +0200 Subject: [PATCH] Deprecate our GenericInstrumentation middleware Deprecate the Appsignal::Rack::GenericInstrumentation in favor of the Appsignal::Rack::InstrumentationMiddleware. This other middleware doesn't set the action name to "unknown" by default. The GenericInstrumentation is no longer loaded automatically. This helps with printing a deprecation warning when it is used. Update the Grape specs which I copied this from to not mention the Minutely constant, which I copied it from there. --- ...rack--genericinstrumentation-middleware.md | 6 ++ lib/appsignal.rb | 2 +- lib/appsignal/rack.rb | 26 ++++++ lib/appsignal/rack/generic_instrumentation.rb | 5 +- .../rack/generic_instrumentation_spec.rb | 90 ++++++++++++++----- .../appsignal/rack/grape_middleware_spec.rb | 2 +- 6 files changed, 105 insertions(+), 26 deletions(-) create mode 100644 .changesets/deprecate-appsignal--rack--genericinstrumentation-middleware.md create mode 100644 lib/appsignal/rack.rb diff --git a/.changesets/deprecate-appsignal--rack--genericinstrumentation-middleware.md b/.changesets/deprecate-appsignal--rack--genericinstrumentation-middleware.md new file mode 100644 index 000000000..5fceb9803 --- /dev/null +++ b/.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`. diff --git a/lib/appsignal.rb b/lib/appsignal.rb index 3b9da9a50..4e81e6222 100644 --- a/lib/appsignal.rb +++ b/lib/appsignal.rb @@ -324,9 +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/instrumentation_middleware" -require "appsignal/rack/generic_instrumentation" require "appsignal/rack/event_handler" require "appsignal/integrations/railtie" if defined?(::Rails) require "appsignal/transaction" diff --git a/lib/appsignal/rack.rb b/lib/appsignal/rack.rb new file mode 100644 index 000000000..43c101ce2 --- /dev/null +++ b/lib/appsignal/rack.rb @@ -0,0 +1,26 @@ +# 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 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 diff --git a/lib/appsignal/rack/generic_instrumentation.rb b/lib/appsignal/rack/generic_instrumentation.rb index a7bda9e77..8f9982a5a 100644 --- a/lib/appsignal/rack/generic_instrumentation.rb +++ b/lib/appsignal/rack/generic_instrumentation.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true module Appsignal - # @api private module Rack + # @api private class GenericInstrumentation < AbstractMiddleware def initialize(app, options = {}) options[:instrument_span_name] ||= "process_action.generic" @@ -14,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 diff --git a/spec/lib/appsignal/rack/generic_instrumentation_spec.rb b/spec/lib/appsignal/rack/generic_instrumentation_spec.rb index a996b0073..8eada77fe 100644 --- a/spec/lib/appsignal/rack/generic_instrumentation_spec.rb +++ b/spec/lib/appsignal/rack/generic_instrumentation_spec.rb @@ -1,37 +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 - before(:context) { start_agent } - around { |example| keep_transactions { example.run } } + it "returns the Rack::GenericInstrumentation constant" do + silence do + expect(Appsignal::Rack::GenericInstrumentation) + .to be(Appsignal::Rack::GenericInstrumentationAlias) + end + end - def make_request(env) - middleware.call(env) - end + it "prints a deprecation warning to STDERR" do + capture_std_streams(std_stream, err_stream) do + Appsignal::Rack::GenericInstrumentation + end - context "without an exception" do - it "reports a process_action.generic event" do - make_request(env) + expect(stderr).to include( + "appsignal WARNING: The constant Appsignal::Rack::GenericInstrumentation " \ + "has been deprecated." + ) + end - expect(last_transaction).to include_event("name" => "process_action.generic") + it "logs a warning" do + logs = + capture_logs do + silence do + Appsignal::Rack::GenericInstrumentation + end + end + + expect(logs).to contains_log( + :warn, + "The constant Appsignal::Rack::GenericInstrumentation has been deprecated." + ) 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) + 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 } } - expect(last_transaction).to have_action("MyAction") + 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 - end - context "without action name metadata" do - it "reports 'unknown' as the action name" do - make_request(env) + 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 diff --git a/spec/lib/appsignal/rack/grape_middleware_spec.rb b/spec/lib/appsignal/rack/grape_middleware_spec.rb index 4312934d8..939ea23df 100644 --- a/spec/lib/appsignal/rack/grape_middleware_spec.rb +++ b/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