Skip to content

Commit

Permalink
Use AbstractMiddleware for Rails middleware
Browse files Browse the repository at this point in the history 
Use our new AbstractMiddleware as a basis for the Rails instrumentation
middleware. This moves all responsibility for handling nested
instrumentation middleware to the AbstractMiddleware.

The Rails middleware can now focus on only the part it needs to do: the
action name and some additional metadata.

The RailsInstrumentation middleware previously didn't create an event,
but the AbstractMiddleware does do that now. It will report a new
`middleware.rails` event now.

I didn't add a changeset for this change as it's mostly an internal
refactor.
  • Loading branch information
@tombruijn
tombruijn committed Jun 24, 2024
1 parent 66be021 commit b7cdd26
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 190 deletions.
6 changes: 3 additions & 3 deletions lib/appsignal.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,11 +323,11 @@ def const_missing(name)
require "appsignal/probes"
require "appsignal/marker"
require "appsignal/garbage_collection"
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/integrations/railtie" if defined?(::Rails)
require "appsignal/transaction"
require "appsignal/version"
require "appsignal/transmitter"
require "appsignal/heartbeat"
66 changes: 12 additions & 54 deletions lib/appsignal/rack/rails_instrumentation.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,66 +5,24 @@
module Appsignal
module Rack
# @api private
class RailsInstrumentation
class RailsInstrumentation < Appsignal::Rack::AbstractMiddleware
def initialize(app, options = {})
Appsignal.internal_logger.debug "Initializing Appsignal::Rack::RailsInstrumentation"
@app = app
@options = options
options[:request_class] ||= ActionDispatch::Request
options[:params_method] ||= :filtered_parameters
options[:instrument_span_name] ||= "middleware.rails"
super
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 = ActionDispatch::Request.new(env)
transaction = env.fetch(
Appsignal::Rack::APPSIGNAL_TRANSACTION,
Appsignal::Transaction::NilTransaction.new
)
private

begin
@app.call(env)
rescue Exception => error # rubocop:disable Lint/RescueException
transaction.set_error(error)
raise error
ensure
controller = env["action_controller.instance"]
if controller
transaction.set_action_if_nil("#{controller.class}##{controller.action_name}")
end
transaction.set_params_if_nil(fetch_params(request))
request_id = fetch_request_id(env)
transaction.set_tags(:request_id => request_id) if request_id
transaction.set_metadata("path", request.path)
request_method = fetch_request_method(request)
transaction.set_metadata("method", request_method) if request_method
end
end
def add_transaction_metadata_after(transaction, request)
controller = request.env["action_controller.instance"]
transaction.set_action_if_nil("#{controller.class}##{controller.action_name}") if controller

def fetch_request_id(env)
env["action_dispatch.request_id"]
end

def fetch_params(request)
return unless request.respond_to?(:filtered_parameters)

request.filtered_parameters
rescue => error
# Getting params from the request has been know to fail.
Appsignal.internal_logger.debug "Exception while getting Rails params: #{error}"
nil
end
request_id = request.env["action_dispatch.request_id"]
transaction.set_tags(:request_id => request_id) if request_id

def fetch_request_method(request)
request.request_method
rescue => error
Appsignal.internal_logger.error("Unable to report HTTP request method: '#{error}'")
nil
super
end
end
end
Expand Down
186 changes: 53 additions & 133 deletions spec/lib/appsignal/rack/rails_instrumentation_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ class MockController; end
"password" => "super secret"
}
end
let(:env_extra) { {} }
let(:env) do
http_request_env_with_data({
http_request_env_with_data(
:params => params,
:with_queue_start => true,
"action_dispatch.request_id" => "request_id123",
Expand All @@ -36,160 +35,81 @@ class MockController; end
:class => MockController,
:action_name => "index"
)
}.merge(env_extra))
)
end
let(:middleware) { Appsignal::Rack::RailsInstrumentation.new(app, {}) }
around { |example| keep_transactions { example.run } }
before do
env[Appsignal::Rack::APPSIGNAL_TRANSACTION] = transaction
end

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 "calls 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 "does not call with monitoring" do
expect(middleware).to_not receive(:call_with_appsignal_monitoring)
end

it "calls the app" do
expect(app).to receive(:call).with(env)
end
end

after { middleware.call(env) }
def make_request(env)
middleware.call(env)
last_transaction.complete # Manually close transaction to set sample data
end

describe "#call_with_appsignal_monitoring" do
def run
middleware.call(env)
last_transaction.complete # Manually close transaction to set sample data
end
it "sets the controller action as the action name" do
make_request(env)

it "calls the wrapped app" do
expect { run }.to_not(change { created_transactions.length })
expect(app).to have_received(:call).with(env)
end
expect(last_transaction.to_h).to include(
"namespace" => Appsignal::Transaction::HTTP_REQUEST,
"action" => "MockController#index"
)
end

it "sets request metadata on the transaction" do
run
it "sets request metadata on the transaction" do
make_request(env)

expect(last_transaction.to_h).to include(
"namespace" => Appsignal::Transaction::HTTP_REQUEST,
"action" => "MockController#index",
"metadata" => hash_including(
"method" => "GET",
"path" => "/blog"
),
"sample_data" => hash_including(
"tags" => { "request_id" => "request_id123" }
)
expect(last_transaction.to_h).to include(
"metadata" => hash_including(
"method" => "GET",
"path" => "/blog"
),
"sample_data" => hash_including(
"tags" => { "request_id" => "request_id123" }
)
end

it "reports Rails filter parameters" do
run
)
end

expect(last_transaction.to_h).to include(
"sample_data" => hash_including(
"params" => params.merge(
"my_custom_param" => "[FILTERED]",
"password" => "[FILTERED]"
)
)
it "reports Rails filter parameters" do
make_request(env)

expect(last_transaction.to_h).to include(
"sample_data" => hash_including(
"params" => {
"controller" => "blog_posts",
"action" => "show",
"id" => "1",
"my_custom_param" => "[FILTERED]",
"password" => "[FILTERED]"
}
)
end

context "with custom params" do
let(:app) do
lambda do |env|
env[Appsignal::Rack::APPSIGNAL_TRANSACTION].set_params("custom_param" => "yes")
end
end

it "allows custom params to be set" do
run

expect(last_transaction.to_h).to include(
"sample_data" => hash_including(
"params" => {
"custom_param" => "yes"
}
)
)
end
end

context "with an invalid HTTP request method" do
let(:env_extra) { { :request_method => "FOO", "REQUEST_METHOD" => "FOO" } }

it "does not store the HTTP request method" do
run

transaction_hash = last_transaction.to_h
expect(transaction_hash["metadata"]).to_not have_key("method")
expect(log_contents(log))
.to contains_log(:error, "Unable to report HTTP request method: '")
end
end

context "with an exception" do
let(:error) { ExampleException.new("ExampleException message") }
let(:app) do
double.tap do |d|
allow(d).to receive(:call).and_raise(error)
end
end
)
end

it "records the exception" do
expect { run }.to raise_error(error)
context "with an invalid HTTP request method" do
it "does not store the invalid HTTP request method" do
make_request(env.merge(:request_method => "FOO", "REQUEST_METHOD" => "FOO"))

transaction_hash = last_transaction.to_h
expect(transaction_hash["error"]).to include(
"name" => "ExampleException",
"message" => "ExampleException message",
"backtrace" => kind_of(String)
)
end
transaction_hash = last_transaction.to_h
expect(transaction_hash["metadata"]).to_not have_key("method")
expect(log_contents(log))
.to contains_log(:error, "Unable to report HTTP request method: '")
end
end

context "with a request path that's not a route" do
let(:env_extra) do
{
context "with a request path that's not a route" do
it "doesn't set an action name" do
make_request(
env.merge(
:path => "/unknown-route",
"action_controller.instance" => nil
}
end

it "doesn't set an action name" do
run

expect(last_transaction.to_h).to include(
"action" => nil
)
end
end
end

describe "#fetch_request_id" do
subject { middleware.fetch_request_id(env) }

let(:env) { { "action_dispatch.request_id" => "id" } }
)

it "returns the action dispatch id" do
is_expected.to eq "id"
expect(last_transaction.to_h).to include(
"action" => nil
)
end
end
end
Expand Down

0 comments on commit b7cdd26

Please sign in to comment.