From 4fd49c6604b2059f7fd3780b5b0630dd5a9ecacd Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Wed, 19 Jun 2024 11:21:41 +0200 Subject: [PATCH] Improve supported for nested Rack middlewares 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. --- ...ested-genericinstrumentation-middleware.md | 6 + lib/appsignal.rb | 1 + lib/appsignal/rack/abstract_middleware.rb | 127 +++++++++ lib/appsignal/rack/generic_instrumentation.rb | 39 +-- lib/appsignal/rack/sinatra_instrumentation.rb | 75 ++---- .../rack/abstract_middleware_spec.rb | 250 ++++++++++++++++++ .../rack/generic_instrumentation_spec.rb | 105 ++------ .../rack/sinatra_instrumentation_spec.rb | 176 +++--------- 8 files changed, 465 insertions(+), 314 deletions(-) create mode 100644 .changesets/support-nested-genericinstrumentation-middleware.md create mode 100644 lib/appsignal/rack/abstract_middleware.rb create mode 100644 spec/lib/appsignal/rack/abstract_middleware_spec.rb diff --git a/.changesets/support-nested-genericinstrumentation-middleware.md b/.changesets/support-nested-genericinstrumentation-middleware.md new file mode 100644 index 000000000..45e5df617 --- /dev/null +++ b/.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 evens. This also improves support for apps that have multiple Rack GenericInstrumentation or SinatraInstrumentation middlewares. diff --git a/lib/appsignal.rb b/lib/appsignal.rb index 1e339d61a..38f21ff8e 100644 --- a/lib/appsignal.rb +++ b/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" diff --git a/lib/appsignal/rack/abstract_middleware.rb b/lib/appsignal/rack/abstract_middleware.rb new file mode 100644 index 000000000..b65bd70cc --- /dev/null +++ b/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 diff --git a/lib/appsignal/rack/generic_instrumentation.rb b/lib/appsignal/rack/generic_instrumentation.rb index 2599566e2..2a9a7bc6f 100644 --- a/lib/appsignal/rack/generic_instrumentation.rb +++ b/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 + super + @instrument_span_name = "process_action.generic" 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 diff --git a/lib/appsignal/rack/sinatra_instrumentation.rb b/lib/appsignal/rack/sinatra_instrumentation.rb index 877969c7e..c0a473495 100644 --- a/lib/appsignal/rack/sinatra_instrumentation.rb +++ b/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 + super + @request_class ||= Sinatra::Request + @params_method ||= :params + @instrument_span_name = "process_action.sinatra" + @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) && diff --git a/spec/lib/appsignal/rack/abstract_middleware_spec.rb b/spec/lib/appsignal/rack/abstract_middleware_spec.rb new file mode 100644 index 000000000..86ff621b8 --- /dev/null +++ b/spec/lib/appsignal/rack/abstract_middleware_spec.rb @@ -0,0 +1,250 @@ +describe Appsignal::Rack::AbstractMiddleware do + let(:app) { double(:call => true) } + let(:request_path) { "/some/path" } + let(:env) do + Rack::MockRequest.env_for( + request_path, + "REQUEST_METHOD" => "GET", + :params => { "page" => 2, "query" => "lorem" } + ) + end + let(:options) { {} } + let(:middleware) { Appsignal::Rack::AbstractMiddleware.new(app, options) } + + before(:context) { start_agent } + around { |example| keep_transactions { example.run } } + + def make_request(env) + middleware.call(env) + end + + def make_request_with_error(env, error) + expect { make_request(env) }.to raise_error(error) + end + + describe "#call" do + context "when appsignal is not active" do + before { allow(Appsignal).to receive(:active?).and_return(false) } + + it "does not instrument requests" do + expect { make_request(env) }.to_not(change { created_transactions.count }) + end + + it "calls the next middleware in the stack" do + expect(app).to receive(:call).with(env) + make_request(env) + end + end + + context "when appsignal is active" do + before { allow(Appsignal).to receive(:active?).and_return(true) } + + it "calls the next middleware in the stack" do + make_request(env) + + expect(app).to have_received(:call).with(env) + end + + context "without an exception" do + it "create a transaction for the request" do + expect { make_request(env) }.to(change { created_transactions.count }.by(1)) + + expect(last_transaction.to_h).to include( + "namespace" => Appsignal::Transaction::HTTP_REQUEST, + "action" => nil, + "error" => nil + ) + end + + it "reports a process.abstract event" do + make_request(env) + + expect(last_transaction.to_h).to include( + "events" => [ + hash_including( + "body" => "", + "body_format" => Appsignal::EventFormatter::DEFAULT, + "count" => 1, + "name" => "process.abstract", + "title" => "" + ) + ] + ) + end + + it "completes the transaction" do + make_request(env) + expect(last_transaction).to be_completed + end + end + + context "with an exception" do + let(:error) { ExampleException.new("error message") } + before do + allow(app).to receive(:call).and_raise(error) + expect { make_request_with_error(env, error) } + .to(change { created_transactions.count }.by(1)) + end + + it "creates a transaction for the request and records the exception" do + expect(last_transaction.to_h).to include( + "error" => hash_including( + "name" => "ExampleException", + "message" => "error message" + ) + ) + end + + it "completes the transaction" do + expect(last_transaction).to be_completed + end + end + + context "without action name metadata" do + it "reports no action name" do + make_request(env) + + expect(last_transaction.to_h).to include("action" => nil) + end + end + + context "with appsignal.route env" do + before do + env["appsignal.route"] = "POST /my-route" + end + + it "reports the appsignal.route value as the action name" do + make_request(env) + + expect(last_transaction.to_h).to include("action" => "POST /my-route") + end + end + + context "with appsignal.action env" do + before do + env["appsignal.action"] = "POST /my-action" + end + + it "reports the appsignal.route value as the action name" do + make_request(env) + + expect(last_transaction.to_h).to include("action" => "POST /my-action") + end + end + + describe "request metadata" do + before do + env.merge("PATH_INFO" => "/some/path", "REQUEST_METHOD" => "GET") + end + + it "sets request metadata" do + make_request(env) + + expect(last_transaction.to_h).to include( + "metadata" => { + "method" => "GET", + "path" => "/some/path" + }, + "sample_data" => hash_including( + "environment" => hash_including( + "REQUEST_METHOD" => "GET", + "PATH_INFO" => "/some/path" + # and more, but we don't need to test Rack mock defaults + ) + ) + ) + end + + it "sets request parameters" do + make_request(env) + + expect(last_transaction.to_h).to include( + "sample_data" => hash_including( + "params" => hash_including( + "page" => "2", + "query" => "lorem" + ) + ) + ) + end + end + + context "with queue start header" do + let(:queue_start_time) { fixed_time * 1_000 } + before do + env["HTTP_X_REQUEST_START"] = "t=#{queue_start_time.to_i}" # in milliseconds + end + + it "sets the queue start" do + make_request(env) + + expect(last_transaction.ext.queue_start).to eq(queue_start_time) + end + end + + class FilteredRequest + attr_reader :env + + def initialize(env) + @env = env + end + + def path + "/static/path" + end + + def request_method + "GET" + end + + def filtered_params + { "abc" => "123" } + end + end + + context "with overridden request class and params method" do + let(:options) do + { :request_class => FilteredRequest, :params_method => :filtered_params } + end + + it "uses the overridden request class and params method to fetch params" do + make_request(env) + + expect(last_transaction.to_h).to include( + "sample_data" => hash_including( + "params" => { "abc" => "123" } + ) + ) + end + end + + context "with parent instrumentation" do + before do + env[Appsignal::Rack::APPSIGNAL_TRANSACTION] = http_request_transaction + end + + it "uses the existing transaction" do + make_request(env) + + expect { make_request(env) }.to_not(change { created_transactions.count }) + end + + it "doesn't complete the existing transaction" do + make_request(env) + + expect(env[Appsignal::Rack::APPSIGNAL_TRANSACTION]).to_not be_completed + end + + context "with custom set action name" do + it "does not overwrite the action name" do + env[Appsignal::Rack::APPSIGNAL_TRANSACTION].set_action("My custom action") + env["appsignal.action"] = "POST /my-action" + make_request(env) + + expect(last_transaction.to_h).to include("action" => "My custom action") + end + end + end + end + end +end diff --git a/spec/lib/appsignal/rack/generic_instrumentation_spec.rb b/spec/lib/appsignal/rack/generic_instrumentation_spec.rb index 0fa0104b9..fef5f1b7b 100644 --- a/spec/lib/appsignal/rack/generic_instrumentation_spec.rb +++ b/spec/lib/appsignal/rack/generic_instrumentation_spec.rb @@ -1,91 +1,38 @@ describe Appsignal::Rack::GenericInstrumentation do - before :context do - start_agent - end - let(:app) { double(:call => true) } - let(:env) { { :path => "/", :method => "GET" } } - let(:options) { {} } - let(:middleware) { Appsignal::Rack::GenericInstrumentation.new(app, options) } - - describe "#call" do - before do - allow(middleware).to receive(:raw_payload).and_return({}) - end - - context "when appsignal is active" do - before { allow(Appsignal).to receive(:active?).and_return(true) } - - it "should call with monitoring" do - expect(middleware).to receive(:call_with_appsignal_monitoring).with(env) - end - end - - context "when appsignal is not active" do - before { allow(Appsignal).to receive(:active?).and_return(false) } - - it "should not call with monitoring" do - expect(middleware).to_not receive(:call_with_appsignal_monitoring) - end + let(:env) { Rack::MockRequest.env_for("/some/path") } + let(:middleware) { Appsignal::Rack::GenericInstrumentation.new(app, {}) } - it "should call the stack" do - expect(app).to receive(:call).with(env) - end - end + before(:context) { start_agent } + around { |example| keep_transactions { example.run } } - after { middleware.call(env) } + def make_request(env) + middleware.call(env) end - describe "#call_with_appsignal_monitoring", :error => false do - it "should create a transaction" do - expect(Appsignal::Transaction).to receive(:create).with( - kind_of(String), - Appsignal::Transaction::HTTP_REQUEST, - kind_of(Rack::Request) - ).and_return(double(:set_action_if_nil => nil, :set_http_or_background_queue_start => nil, - :set_metadata => nil)) - end - - it "should call the app" do - expect(app).to receive(:call).with(env) + context "without an exception" do + it "reports a process_action.generic event" do + make_request(env) + + expect(last_transaction.to_h).to include( + "events" => [ + hash_including( + "body" => "", + "body_format" => Appsignal::EventFormatter::DEFAULT, + "count" => 1, + "name" => "process_action.generic", + "title" => "" + ) + ] + ) end + end - context "with an exception", :error => true do - let(:error) { ExampleException } - let(:app) do - double.tap do |d| - allow(d).to receive(:call).and_raise(error) - end - end - - it "records the exception" do - expect_any_instance_of(Appsignal::Transaction).to receive(:set_error).with(error) - end - end - - it "should set the action to unknown" do - expect_any_instance_of(Appsignal::Transaction).to receive(:set_action_if_nil).with("unknown") - end - - context "with a route specified in the env" do - before do - env["appsignal.route"] = "GET /" - end - - it "should set the action" do - expect_any_instance_of(Appsignal::Transaction).to receive(:set_action_if_nil).with("GET /") - end - end + context "without action name metadata" do + it "reports 'unknown' as the action name" do + make_request(env) - it "should set metadata" do - expect_any_instance_of(Appsignal::Transaction).to receive(:set_metadata).twice + expect(last_transaction.to_h).to include("action" => "unknown") end - - it "should set the queue start" do - expect_any_instance_of(Appsignal::Transaction).to receive(:set_http_or_background_queue_start) - end - - after(:error => false) { middleware.call(env) } - after(:error => true) { expect { middleware.call(env) }.to raise_error(error) } end end diff --git a/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb b/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb index b7d7495e1..2e9865685 100644 --- a/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb +++ b/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb @@ -16,7 +16,9 @@ def make_request_with_error(env, error) let(:settings) { double(:raise_errors => false) } let(:app) { double(:call => true, :settings => settings) } - let(:env) { { "sinatra.route" => "GET /", :path => "/", :method => "GET" } } + let(:env) do + Rack::MockRequest.env_for("/path", "sinatra.route" => "GET /path", "REQUEST_METHOD" => "GET") + end let(:middleware) { Appsignal::Rack::SinatraInstrumentation.new(app) } before(:context) { start_agent } @@ -46,7 +48,9 @@ def make_request_with_error(env, error) let(:settings) { double(:raise_errors => false) } let(:app) { double(:call => true, :settings => settings) } - let(:env) { { "sinatra.route" => "GET /path", :path => "/path", :method => "GET" } } + let(:env) do + Rack::MockRequest.env_for("/path", "sinatra.route" => "GET /path", "REQUEST_METHOD" => "GET") + end let(:options) { {} } let(:middleware) { Appsignal::Rack::SinatraBaseInstrumentation.new(app, options) } @@ -100,64 +104,37 @@ def make_request_with_error(env, error) end it "calls the next middleware in the stack" do - expect(app).to receive(:call).with(env) make_request(env) + + expect(app).to have_received(:call).with(env) end end context "when appsignal is active" do - it "calls the next middleware in the stack" do - expect(app).to receive(:call).with(env) - make_request(env) - end - - context "without an error" do - before do - expect { make_request(env) }.to(change { created_transactions.count }.by(1)) - end + context "without an exception" do + it "reports a process_action.sinatra event" do + make_request(env) - it "creates a transaction without an error" do expect(last_transaction.to_h).to include( - "namespace" => Appsignal::Transaction::HTTP_REQUEST, - "action" => "GET /path", - "error" => nil, - "metadata" => { "path" => "" } + "events" => [ + hash_including( + "body" => "", + "body_format" => Appsignal::EventFormatter::DEFAULT, + "count" => 1, + "name" => "process_action.sinatra", + "title" => "" + ) + ] ) end - - it "completes the transaction" do - expect(last_transaction).to be_completed - end end - context "with an error" do + context "with an error in sinatra.error" do let(:error) { ExampleException.new("error message") } before do - allow(app).to receive(:call).and_raise(error) - expect { make_request_with_error(env, error) } - .to(change { created_transactions.count }.by(1)) - end - - it "creates and completes a transaction and records the exception" do - expect(last_transaction.to_h).to include( - "namespace" => Appsignal::Transaction::HTTP_REQUEST, - "action" => "GET /path", - "error" => hash_including( - "name" => "ExampleException", - "message" => "error message" - ) - ) + env["sinatra.error"] = error end - it "completes the transaction" do - expect(last_transaction).to be_completed - end - end - - context "with an error in sinatra.error" do - let(:error) { ExampleException.new("error message") } - let(:env) { { "sinatra.error" => error } } - context "when raise_errors is off" do let(:settings) { double(:raise_errors => false) } @@ -186,7 +163,12 @@ def make_request_with_error(env, error) end context "if sinatra.skip_appsignal_error is set" do - let(:env) { { "sinatra.error" => error, "sinatra.skip_appsignal_error" => true } } + before do + env.merge!( + "sinatra.error" => error, + "sinatra.skip_appsignal_error" => true + ) + end it "does not record the error" do expect { make_request(env) } @@ -205,7 +187,9 @@ def make_request_with_error(env, error) end context "without 'sinatra.route' env" do - let(:env) { { :path => "/path", :method => "GET" } } + let(:env) do + Rack::MockRequest.env_for("/path", "REQUEST_METHOD" => "GET") + end it "doesn't set an action name" do make_request(env) @@ -224,7 +208,9 @@ def make_request_with_error(env, error) end context "without 'sinatra.route' env" do - let(:env) { { :path => "/path", :method => "GET" } } + let(:env) do + Rack::MockRequest.env_for("/path", "REQUEST_METHOD" => "GET") + end it "doesn't set an action name" do make_request(env) @@ -234,100 +220,6 @@ def make_request_with_error(env, error) end end end - - context "metadata" do - let(:env) { { "PATH_INFO" => "/some/path", "REQUEST_METHOD" => "GET" } } - - it "sets metadata from the environment" do - make_request(env) - - expect(last_transaction.to_h).to include( - "metadata" => { - "method" => "GET", - "path" => "/some/path" - }, - "sample_data" => hash_including( - "environment" => hash_including( - "REQUEST_METHOD" => "GET", - "PATH_INFO" => "/some/path" - ) - ) - ) - end - end - - context "with queue start" do - let(:queue_start_time) { fixed_time * 1_000 } - let(:env) do - { "HTTP_X_REQUEST_START" => "t=#{queue_start_time.to_i}" } # in milliseconds - end - - it "sets the queue start" do - make_request(env) - expect(last_transaction.ext.queue_start).to eq(queue_start_time) - end - end - - class FilteredRequest - def initialize(_args) # rubocop:disable Style/RedundantInitialize - end - - def path - "/static/path" - end - - def request_method - "GET" - end - - def filtered_params - { "abc" => "123" } - end - end - - context "with overridden request class and params method" do - let(:options) do - { :request_class => FilteredRequest, :params_method => :filtered_params } - end - - it "uses the overridden request class and params method to fetch params" do - make_request(env) - - expect(last_transaction.to_h).to include( - "sample_data" => hash_including( - "params" => { "abc" => "123" } - ) - ) - end - end - - context "with parent instrumentation" do - before do - env["PATH_INFO"] = "/some/path" - env["REQUEST_METHOD"] = "GET" - env[Appsignal::Rack::APPSIGNAL_TRANSACTION] = http_request_transaction - make_request(env) - end - - it "uses the existing transaction" do - expect { make_request(env) }.to_not(change { created_transactions.count }) - end - - it "sets metadata on the transaction" do - expect(env[Appsignal::Rack::APPSIGNAL_TRANSACTION].to_h).to include( - "namespace" => Appsignal::Transaction::HTTP_REQUEST, - "action" => "GET /path", - "metadata" => { - "method" => "GET", - "path" => "/some/path" - } - ) - end - - it "doesn't complete the existing transaction" do - expect(env[Appsignal::Rack::APPSIGNAL_TRANSACTION]).to_not be_completed - end - end end end end