Skip to content

Commit

Permalink
Extract Rack request metadata helpers to class
Browse files Browse the repository at this point in the history 
Create a new class for the Rack request metadata extraction so it can be
reused in other places more easily too.

Some of the specs are duplicated. I've moved all the specs for edge
cases to the class with the actual implementation. The
AbstractMiddleware just tests the basics that we expect that class to
handle.
  • Loading branch information
@tombruijn
tombruijn committed Aug 30, 2024
1 parent db3a778 commit 62c2303
Show file tree
Hide file tree
Showing 5 changed files with 283 additions and 169 deletions.
61 changes: 61 additions & 0 deletions lib/appsignal/rack.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
50 changes: 3 additions & 47 deletions lib/appsignal/rack/abstract_middleware.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
161 changes: 39 additions & 122 deletions spec/lib/appsignal/rack/abstract_middleware_spec.rb
View file
Original file line number Diff line number Diff line change
@@ -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 }
Expand Down Expand Up @@ -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")
Expand All @@ -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

Expand All @@ -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

Expand Down
Loading

0 comments on commit 62c2303

Please sign in to comment.