Skip to content

Commit

Permalink
Fix sentry-rails' backtrace cleaner issues (#2475)
Browse files Browse the repository at this point in the history
* Clear Rails' default backtrace filters from Sentry's backtrace cleaner

In Rails 7.2, Rails's backtrace cleaner, which sentry-rails' backtrace
cleaner inherits from, starts shortening gem's paths in backtraces. This
will prevent sentry-ruby's `Sentry::Backtrace` from handling them correctly,
which will result in issues like #2472.

This commit avoids the issue by clearing the default filters that
Sentry's backtrace cleaner inherits.

* Remove premature path-removal filter

This filter removes the project root part from the stacktrace, which prevents
sentry-ruby from retaining the correct absolute path of it.
  • Loading branch information
st0012 authored Dec 2, 2024
1 parent f225138 commit 0f0666c
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 8 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
- Fix `RescuedExceptionInterceptor` to handle an empty configuration [#2428](https://github.com/getsentry/sentry-ruby/pull/2428)
- Add mutex sync to `SessionFlusher` aggregates [#2469](https://github.com/getsentry/sentry-ruby/pull/2469)
- Fixes [#2468](https://github.com/getsentry/sentry-ruby/issues/2468)
- Fix sentry-rails' backtrace cleaner issues ([#2475](https://github.com/getsentry/sentry-ruby/pull/2475))
- Fixes [#2472](https://github.com/getsentry/sentry-ruby/issues/2472)

## 5.21.0

Expand Down
8 changes: 3 additions & 5 deletions sentry-rails/lib/sentry/rails/backtrace_cleaner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@ class BacktraceCleaner < ActiveSupport::BacktraceCleaner

def initialize
super
# we don't want any default silencers because they're too aggressive
# We don't want any default silencers because they're too aggressive
remove_silencers!
# We don't want any default filters because Rails 7.2 starts shortening the paths. See #2472
remove_filters!

@root = "#{Sentry.configuration.project_root}/"
add_filter do |line|
line.start_with?(@root) ? line.from(@root.size) : line
end
add_filter do |line|
if line =~ RENDER_TEMPLATE_PATTERN
line.sub(RENDER_TEMPLATE_PATTERN, "")
Expand Down
10 changes: 7 additions & 3 deletions sentry-rails/spec/sentry/rails/event_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
it 'marks in_app correctly' do
frames = hash[:exception][:values][0][:stacktrace][:frames]
expect(frames[0][:filename]).to eq("test/some/other/path")
expect(frames[0][:abs_path]).to eq("test/some/other/path")
expect(frames[0][:in_app]).to eq(true)
expect(frames[1][:filename]).to eq("/app/some/other/path")
expect(frames[1][:in_app]).to eq(false)
Expand All @@ -41,7 +42,7 @@
expect(frames[3][:in_app]).to eq(true)
expect(frames[4][:filename]).to eq("vendor/bundle/some_gem.rb")
expect(frames[4][:in_app]).to eq(false)
expect(frames[5][:filename]).to eq("vendor/bundle/cache/other_gem.rb")
expect(frames[5][:filename]).to eq("dummy/test_rails_app/vendor/bundle/cache/other_gem.rb")
expect(frames[5][:in_app]).to eq(false)
end

Expand All @@ -50,6 +51,7 @@
$LOAD_PATH << "#{Rails.root}/app/models"
frames = hash[:exception][:values][0][:stacktrace][:frames]
expect(frames[3][:filename]).to eq("app/models/user.rb")
expect(frames[3][:abs_path]).to eq("#{Rails.root}/app/models/user.rb")
$LOAD_PATH.delete("#{Rails.root}/app/models")
end
end
Expand All @@ -58,15 +60,17 @@
it 'normalizes the filename using the load path' do
$LOAD_PATH.push "vendor/bundle"
frames = hash[:exception][:values][0][:stacktrace][:frames]
expect(frames[5][:filename]).to eq("cache/other_gem.rb")
expect(frames[5][:filename]).to eq("dummy/test_rails_app/vendor/bundle/cache/other_gem.rb")
expect(frames[5][:abs_path]).to eq("#{Rails.root}/vendor/bundle/cache/other_gem.rb")
$LOAD_PATH.pop
end
end

context "when a non-in_app path under project_root isn't on the load path" do
it 'normalizes the filename using project_root' do
frames = hash[:exception][:values][0][:stacktrace][:frames]
expect(frames[5][:filename]).to eq("vendor/bundle/cache/other_gem.rb")
expect(frames[5][:filename]).to eq("dummy/test_rails_app/vendor/bundle/cache/other_gem.rb")
expect(frames[5][:abs_path]).to eq("#{Rails.root}/vendor/bundle/cache/other_gem.rb")
end
end
end
Expand Down
10 changes: 10 additions & 0 deletions sentry-rails/spec/sentry/rails_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,16 @@ def capture_in_separate_process(exit_code:)
expect(traces.dig(-1, "function")).to be_nil
end

it "makes sure BacktraceCleaner gem cleanup doesn't affect context lines population" do
get "/view_exception"

traces = event.dig("exception", "values", 0, "stacktrace", "frames")
gem_frame = traces.find { |t| t["abs_path"].match(/actionview/) }
expect(gem_frame["pre_context"]).not_to be_empty
expect(gem_frame["post_context"]).not_to be_empty
expect(gem_frame["context_line"]).not_to be_empty
end

it "doesn't filters exception backtrace if backtrace_cleanup_callback is overridden" do
make_basic_app do |config|
config.backtrace_cleanup_callback = lambda { |backtrace| backtrace }
Expand Down

0 comments on commit 0f0666c

Please sign in to comment.