diff --git a/lib/appsignal/rack.rb b/lib/appsignal/rack.rb index 88453447..2c41a113 100644 --- a/lib/appsignal/rack.rb +++ b/lib/appsignal/rack.rb @@ -37,5 +37,66 @@ def self.queue_start_from(env) end end end + + class ApplyRackRequest + attr_reader :request, :options + + def initialize(request, options = {}) + @request = request + @options = options + @params_method = options.fetch(:params_method, :params) + end + + def apply_to(transaction) + transaction.set_metadata("path", request.path) + + request_method = request_method_for(request) + transaction.set_metadata("method", request_method) if request_method + + transaction.add_params { params_for(request) } + transaction.add_session_data { session_data_for(request) } + transaction.add_headers do + request.env if request.respond_to?(:env) + end + + queue_start = Appsignal::Rack::Utils.queue_start_from(request.env) + transaction.set_queue_start(queue_start) if queue_start + end + + private + + def params_for(request) + return unless request.respond_to?(@params_method) + + request.send(@params_method) + rescue => error + Appsignal.internal_logger.error( + "Exception while fetching params from '#{request.class}##{@params_method}': " \ + "#{error.class} #{error}" + ) + nil + end + + def request_method_for(request) + request.request_method + rescue => error + Appsignal.internal_logger.error( + "Exception while fetching the HTTP request method: #{error.class}: #{error}" + ) + nil + end + + def session_data_for(request) + return unless request.respond_to?(:session) + + request.session.to_h + rescue => error + Appsignal.internal_logger.error( + "Exception while fetching session data from '#{request.class}': " \ + "#{error.class} #{error}" + ) + nil + end + end end end diff --git a/lib/appsignal/rack/abstract_middleware.rb b/lib/appsignal/rack/abstract_middleware.rb index 3abaca98..03ab0148 100644 --- a/lib/appsignal/rack/abstract_middleware.rb +++ b/lib/appsignal/rack/abstract_middleware.rb @@ -19,7 +19,6 @@ def initialize(app, options = {}) @app = app @options = options @request_class = options.fetch(:request_class, ::Rack::Request) - @params_method = options.fetch(:params_method, :params) @instrument_event_name = options.fetch(:instrument_event_name, nil) @report_errors = options.fetch(:report_errors, DEFAULT_ERROR_REPORTING) end @@ -136,52 +135,9 @@ def add_transaction_metadata_before(transaction, request) # 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) - transaction.set_metadata("path", request.path) - - request_method = request_method_for(request) - transaction.set_metadata("method", request_method) if request_method - - transaction.add_params { params_for(request) } - transaction.add_session_data { session_data_for(request) } - transaction.add_headers do - request.env if request.respond_to?(:env) - end - - queue_start = Appsignal::Rack::Utils.queue_start_from(request.env) - transaction.set_queue_start(queue_start) if queue_start - end - - def params_for(request) - return unless request.respond_to?(@params_method) - - request.send(@params_method) - rescue => error - Appsignal.internal_logger.error( - "Exception while fetching params from '#{@request_class}##{@params_method}': " \ - "#{error.class} #{error}" - ) - nil - end - - def request_method_for(request) - request.request_method - rescue => error - Appsignal.internal_logger.error( - "Exception while fetching the HTTP request method: #{error.class}: #{error}" - ) - nil - end - - def session_data_for(request) - return unless request.respond_to?(:session) - - request.session.to_h - rescue => error - Appsignal.internal_logger.error( - "Exception while fetching session data from '#{@request_class}': " \ - "#{error.class} #{error}" - ) - nil + Appsignal::Rack::ApplyRackRequest + .new(request, @options) + .apply_to(transaction) end def request_for(env) diff --git a/spec/lib/appsignal/rack/abstract_middleware_spec.rb b/spec/lib/appsignal/rack/abstract_middleware_spec.rb index 3ea6bf97..2fcbf702 100644 --- a/spec/lib/appsignal/rack/abstract_middleware_spec.rb +++ b/spec/lib/appsignal/rack/abstract_middleware_spec.rb @@ -1,19 +1,8 @@ describe Appsignal::Rack::AbstractMiddleware do - class HashLike < Hash - def initialize(value) - @value = value - end - - def to_h - @value - end - end - let(:app) { DummyApp.new } - let(:request_path) { "/some/path" } let(:env) do Rack::MockRequest.env_for( - request_path, + "/some/path", "REQUEST_METHOD" => "GET", :params => { "page" => 2, "query" => "lorem" }, "rack.session" => { "session" => "data", "user_id" => 123 } @@ -174,6 +163,8 @@ def make_request_with_error(error_class, error_message) end end + # Partial duplicate tests from Appsignal::Rack::ApplyRackRequest that + # ensure the request metadata is set on via the AbstractMiddleware. describe "request metadata" do it "sets request metadata" do env.merge!("PATH_INFO" => "/some/path", "REQUEST_METHOD" => "GET") @@ -190,36 +181,6 @@ def make_request_with_error(error_class, error_message) ) end - context "with an invalid HTTP request method" do - it "stores the invalid HTTP request method" do - env["REQUEST_METHOD"] = "FOO" - make_request - - expect(last_transaction).to include_metadata("method" => "FOO") - end - end - - context "when fetching the request method raises an error" do - class BrokenRequestMethodRequest < Rack::Request - def request_method - raise "uh oh!" - end - end - - let(:options) { { :request_class => BrokenRequestMethodRequest } } - - it "does not store the invalid HTTP request method" do - env["REQUEST_METHOD"] = "FOO" - logs = capture_logs { make_request } - - expect(last_transaction).to_not include_metadata("method" => anything) - expect(logs).to contains_log( - :error, - "Exception while fetching the HTTP request method: RuntimeError: uh oh" - ) - end - end - it "sets request parameters" do make_request @@ -229,107 +190,63 @@ def request_method ) end - context "when setting custom params" do - let(:app) do - DummyApp.new do |_env| - Appsignal::Transaction.current.set_params("custom" => "param") - end - end - - it "allow custom request parameters to be set" do - make_request - - expect(last_transaction).to include_params("custom" => "param") - end - end - - context "when fetching the request method raises an error" do - class BrokenRequestParamsRequest < Rack::Request - def params - raise "uh oh!" - end - end - - let(:options) do - { :request_class => BrokenRequestParamsRequest, :params_method => :params } - end - - it "does not store the invalid HTTP request method" do - logs = capture_logs { make_request } - - expect(last_transaction).to_not include_params - expect(logs).to contains_log( - :error, - "Exception while fetching params " \ - "from 'BrokenRequestParamsRequest#params': RuntimeError uh oh!" - ) - end - end - it "sets session data" do make_request expect(last_transaction).to include_session_data("session" => "data", "user_id" => 123) end - it "sets session data if the session is a Hash-like type" do - env["rack.session"] = HashLike.new("hash-like" => "value", "user_id" => 123) - make_request - - expect(last_transaction).to include_session_data("hash-like" => "value", "user_id" => 123) - end - end - - context "with queue start header" do - let(:queue_start_time) { fixed_time * 1_000 } + context "with queue start header" do + let(:queue_start_time) { fixed_time * 1_000 } - it "sets the queue start" do - env["HTTP_X_REQUEST_START"] = "t=#{queue_start_time.to_i}" # in milliseconds - make_request + it "sets the queue start" do + env["HTTP_X_REQUEST_START"] = "t=#{queue_start_time.to_i}" # in milliseconds + make_request - expect(last_transaction).to have_queue_start(queue_start_time) + expect(last_transaction).to have_queue_start(queue_start_time) + end end - end - class FilteredRequest - attr_reader :env + class SomeFilteredRequest + attr_reader :env - def initialize(env) - @env = env - end + def initialize(env) + @env = env + end - def path - "/static/path" - end + def path + "/static/path" + end - def request_method - "GET" - end + def request_method + "GET" + end - def filtered_params - { "abc" => "123" } - end + def filtered_params + { "abc" => "123" } + end - def session - { "data" => "value" } + def session + { "data" => "value" } + end end - end - context "with overridden request class and params method" do - let(:options) do - { :request_class => FilteredRequest, :params_method => :filtered_params } - end + context "with overridden request class and params method" do + let(:options) do + { :request_class => SomeFilteredRequest, :params_method => :filtered_params } + end - it "uses the overridden request class and params method to fetch params" do - make_request + it "uses the overridden request class and params method to fetch params" do + make_request - expect(last_transaction).to include_params("abc" => "123") - end + expect(last_transaction).to include_params("abc" => "123") + end - it "uses the overridden request class to fetch session data" do - make_request + it "uses the overridden request class to fetch session data" do + make_request - expect(last_transaction).to include_session_data("data" => "value") + expect(last_transaction).to include_session_data("data" => "value") + end end end diff --git a/spec/lib/appsignal/rack_spec.rb b/spec/lib/appsignal/rack_spec.rb index 6d7bec5d..44626a7e 100644 --- a/spec/lib/appsignal/rack_spec.rb +++ b/spec/lib/appsignal/rack_spec.rb @@ -61,3 +61,173 @@ end end end + +describe Appsignal::Rack::ApplyRackRequest do + describe "#apply_to" do + let(:merged_env) do + Rack::MockRequest.env_for( + "/some/path", + { + "REQUEST_METHOD" => "GET", + :params => { "page" => 2, "query" => "lorem" }, + "rack.session" => { "session" => "data", "user_id" => 123 } + }.merge(env) + ) + end + let(:env) { {} } + let(:request) { ::Rack::Request.new(merged_env) } + let(:options) { {} } + let(:helper) { described_class.new(request, options) } + let(:transaction) { http_request_transaction } + before { start_agent } + + def apply_to(transaction) + helper.apply_to(transaction) + transaction._sample + end + + it "sets request metadata" do + apply_to(transaction) + + expect(transaction).to include_metadata( + "method" => "GET", + "path" => "/some/path" + ) + expect(transaction).to include_environment( + "REQUEST_METHOD" => "GET", + "PATH_INFO" => "/some/path" + # and more, but we don't need to test Rack mock defaults + ) + end + + context "with an invalid HTTP request method" do + let(:env) { { "REQUEST_METHOD" => "FOO" } } + + it "stores the invalid HTTP request method" do + apply_to(transaction) + + expect(transaction).to include_metadata("method" => "FOO") + end + end + + context "when fetching the request method raises an error" do + class BrokenRequestMethodRequest < Rack::Request + def request_method + raise "uh oh!" + end + end + + let(:env) { { "REQUEST_METHOD" => "FOO" } } + let(:request) { BrokenRequestMethodRequest.new(merged_env) } + + it "does not store the invalid HTTP request method" do + logs = capture_logs { apply_to(transaction) } + + expect(transaction).to_not include_metadata("method" => anything) + expect(logs).to contains_log( + :error, + "Exception while fetching the HTTP request method: RuntimeError: uh oh" + ) + end + end + + it "sets request parameters" do + apply_to(transaction) + + expect(transaction).to include_params( + "page" => "2", + "query" => "lorem" + ) + end + + context "when fetching the request method raises an error" do + class BrokenRequestParamsRequest < Rack::Request + def params + raise "uh oh!" + end + end + + let(:request) { BrokenRequestParamsRequest.new(merged_env) } + let(:options) { { :params_method => :params } } + + it "does not store the invalid HTTP request method" do + logs = capture_logs { apply_to(transaction) } + + expect(transaction).to_not include_params + expect(logs).to contains_log( + :error, + "Exception while fetching params " \ + "from 'BrokenRequestParamsRequest#params': RuntimeError uh oh!" + ) + end + end + + it "sets session data" do + apply_to(transaction) + + expect(transaction).to include_session_data("session" => "data", "user_id" => 123) + end + + context "with Hash-like session data" do + let(:env) { { "rack.session" => HashLike.new("hash-like" => "value", "user_id" => 123) } } + + it "sets session data" do + apply_to(transaction) + + expect(transaction).to include_session_data("hash-like" => "value", "user_id" => 123) + end + end + + context "with queue start header" do + let(:queue_start_time) { fixed_time * 1_000 } + let(:env) { { "HTTP_X_REQUEST_START" => "t=#{queue_start_time.to_i}" } } # in milliseconds + + it "sets the queue start" do + apply_to(transaction) + + expect(transaction).to have_queue_start(queue_start_time) + end + end + + class RackFilteredRequest + 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 + + def session + { "data" => "value" } + end + end + + context "with overridden request class and params method" do + let(:request) { RackFilteredRequest.new(env) } + let(:options) { { :params_method => :filtered_params } } + + it "uses the overridden request class and params method to fetch params" do + apply_to(transaction) + + expect(transaction).to include_params("abc" => "123") + end + + it "uses the overridden request class to fetch session data" do + apply_to(transaction) + + expect(transaction).to include_session_data("data" => "value") + end + end + end +end diff --git a/spec/support/mocks/hash_like.rb b/spec/support/mocks/hash_like.rb new file mode 100644 index 00000000..ccaabd4c --- /dev/null +++ b/spec/support/mocks/hash_like.rb @@ -0,0 +1,10 @@ +class HashLike < Hash + def initialize(value) + super + @value = value + end + + def to_h + @value + end +end