Skip to content

Commit

Permalink
Re-position CaptureExceptions middleware
Browse files Browse the repository at this point in the history
When `CaptureExceptions` is the first middleware, the benefits are:

1. It captures as many exceptions as possible.
2. Transactions can record almost the full length of the request's
   events.

But there are also some downsides:

1. Exceptions of the outter middlewares may mostly be noise to users, as
   they usually are the Rails default.
2. It makes skipping some requests' transactions very hard. For example:
  #1404

By re-positioning the middleware after `ActionDispatch::Executor`, we
can automatically avoid the downsides while still keeping most of the
benefits. It also allows us to remove the patch on `FileHandler`.
  • Loading branch information
st0012 committed Apr 18, 2021
1 parent 8c43d22 commit 953e68c
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 28 deletions.
16 changes: 0 additions & 16 deletions sentry-rails/lib/sentry/rails/overrides/file_handler.rb

This file was deleted.

13 changes: 2 additions & 11 deletions sentry-rails/lib/sentry/rails/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ module Sentry
class Railtie < ::Rails::Railtie
# middlewares can't be injected after initialize
initializer "sentry.use_rack_middleware" do |app|
# need to be placed at first to capture as many errors as possible
app.config.middleware.insert 0, Sentry::Rails::CaptureExceptions
# placed after all the file-sending middlewares so we can avoid unnecessary transactions
app.config.middleware.insert_after ActionDispatch::Executor, Sentry::Rails::CaptureExceptions
# need to be placed at last to smuggle app exceptions via env
app.config.middleware.use(Sentry::Rails::RescuedExceptionInterceptor)
end
Expand All @@ -21,7 +21,6 @@ class Railtie < ::Rails::Railtie
extend_active_job if defined?(ActiveJob)
patch_background_worker if defined?(ActiveRecord)
override_streaming_reporter if defined?(ActionView)
override_file_handler if defined?(ActionDispatch) && app.config.public_file_server.enabled
setup_backtrace_cleanup_callback
inject_breadcrumbs_logger
activate_tracing
Expand Down Expand Up @@ -79,14 +78,6 @@ def override_streaming_reporter
end
end

def override_file_handler
require "sentry/rails/overrides/file_handler"

ActiveSupport.on_load :action_controller do
ActionDispatch::FileHandler.send(:prepend, Sentry::Rails::Overrides::FileHandler)
end
end

def activate_tracing
if Sentry.configuration.tracing_enabled?
subscribers = Sentry.configuration.rails.tracing_subscribers
Expand Down
7 changes: 7 additions & 0 deletions sentry-rails/spec/sentry/rails/tracing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,16 @@
end

context "with config.public_file_server.enabled = true" do
let(:string_io) { StringIO.new }
let(:logger) do
::Logger.new(string_io)
end

before do
make_basic_app do |config, app|
app.config.public_file_server.enabled = true
config.traces_sample_rate = 1.0
config.logger = logger
end
end

Expand All @@ -124,6 +130,7 @@

expect(response).to have_http_status(:ok)
expect(transport.events).to be_empty
expect(string_io.string).not_to match(/\[Tracing\] Starting <rails\.request>/)
end

it "doesn't get messed up by previous exception" do
Expand Down
2 changes: 1 addition & 1 deletion sentry-rails/spec/sentry/rails_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

it "inserts middleware to a correct position" do
app = Rails.application
expect(app.middleware.find_index(Sentry::Rails::CaptureExceptions)).to eq(0)
expect(app.middleware.find_index(Sentry::Rails::CaptureExceptions)).to eq(4)
expect(app.middleware.find_index(Sentry::Rails::RescuedExceptionInterceptor)).to eq(app.middleware.count - 1)
end

Expand Down

0 comments on commit 953e68c

Please sign in to comment.