diff --git a/sentry-rails/CHANGELOG.md b/sentry-rails/CHANGELOG.md index ecf4be4e8..bd2f98f45 100644 --- a/sentry-rails/CHANGELOG.md +++ b/sentry-rails/CHANGELOG.md @@ -22,6 +22,7 @@ config.rails.tracing_subscribers = [MySubscriber] - Report exceptions from the interceptor middleware for exceptions app [#1379](https://github.com/getsentry/sentry-ruby/pull/1379) - Fixes [#1371](https://github.com/getsentry/sentry-ruby/issues/1371) +- Re-position CaptureExceptions middleware to reduce tracing noise [#1405](https://github.com/getsentry/sentry-ruby/pull/1405) ## 4.3.4 diff --git a/sentry-rails/lib/sentry/rails/capture_exceptions.rb b/sentry-rails/lib/sentry/rails/capture_exceptions.rb index f1f2ae297..9792e2434 100644 --- a/sentry-rails/lib/sentry/rails/capture_exceptions.rb +++ b/sentry-rails/lib/sentry/rails/capture_exceptions.rb @@ -31,15 +31,15 @@ def capture_exception(exception) end def start_transaction(env, scope) - transaction = super + sentry_trace = env["HTTP_SENTRY_TRACE"] + options = { name: scope.transaction_name, op: transaction_op } - return unless transaction - - if @assets_regex && transaction.name.match?(@assets_regex) - transaction.instance_variable_set(:@sampled, false) + if @assets_regex && scope.transaction_name.match?(@assets_regex) + options.merge!(sampled: false) end - transaction + transaction = Sentry::Transaction.from_sentry_trace(sentry_trace, **options) if sentry_trace + Sentry.start_transaction(transaction: transaction, **options) end end end diff --git a/sentry-rails/lib/sentry/rails/overrides/file_handler.rb b/sentry-rails/lib/sentry/rails/overrides/file_handler.rb deleted file mode 100644 index 5abed4474..000000000 --- a/sentry-rails/lib/sentry/rails/overrides/file_handler.rb +++ /dev/null @@ -1,16 +0,0 @@ -module Sentry - module Rails - module Overrides - module FileHandler - def serve(*args) - if Sentry.initialized? && current_transaction = Sentry.get_current_scope.span - # we don't want to expose a setter for @sampled just for this case - current_transaction.instance_variable_set(:@sampled, false) - end - - super - end - end - end - end -end diff --git a/sentry-rails/lib/sentry/rails/railtie.rb b/sentry-rails/lib/sentry/rails/railtie.rb index 3567f639d..ac87f7f50 100644 --- a/sentry-rails/lib/sentry/rails/railtie.rb +++ b/sentry-rails/lib/sentry/rails/railtie.rb @@ -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 @@ -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 @@ -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 diff --git a/sentry-rails/spec/sentry/rails/tracing_spec.rb b/sentry-rails/spec/sentry/rails/tracing_spec.rb index 48de91047..2e76e5f9f 100644 --- a/sentry-rails/spec/sentry/rails/tracing_spec.rb +++ b/sentry-rails/spec/sentry/rails/tracing_spec.rb @@ -87,12 +87,18 @@ end context "with sprockets-rails" do + let(:string_io) { StringIO.new } + let(:logger) do + ::Logger.new(string_io) + end + before do require "sprockets/railtie" make_basic_app do |config, app| app.config.public_file_server.enabled = true config.traces_sample_rate = 1.0 + config.logger = logger end end @@ -101,14 +107,21 @@ expect(response).to have_http_status(:not_found) expect(transport.events).to be_empty + expect(string_io.string).not_to match(/\[Tracing\] Starting /) end 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 @@ -117,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 /) end it "doesn't get messed up by previous exception" do diff --git a/sentry-rails/spec/sentry/rails_spec.rb b/sentry-rails/spec/sentry/rails_spec.rb index 6224da9d7..a9c933df1 100644 --- a/sentry-rails/spec/sentry/rails_spec.rb +++ b/sentry-rails/spec/sentry/rails_spec.rb @@ -24,7 +24,8 @@ it "inserts middleware to a correct position" do app = Rails.application - expect(app.middleware.find_index(Sentry::Rails::CaptureExceptions)).to eq(0) + index_of_executor = app.middleware.find_index { |m| m == ActionDispatch::Executor } + expect(app.middleware.find_index(Sentry::Rails::CaptureExceptions)).to eq(index_of_executor + 1) expect(app.middleware.find_index(Sentry::Rails::RescuedExceptionInterceptor)).to eq(app.middleware.count - 1) end