Skip to content

Fix sentry-rails' backtrace cleaner issues #2475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 2, 2024
Merged

Fix sentry-rails' backtrace cleaner issues #2475

merged 3 commits into from
Dec 2, 2024

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Nov 27, 2024

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.

Generally speaking, if stacktrace is messed up between Rails versions, but not Ruby versions, then it's usually caused by Sentry::Rails::BacktraceCleaner and/or the backtrace_cleanup_callback sentry-ruby registers (less likely). But if the backtrace is messed up between Ruby versions, it'd be on sentry-ruby's backtrace handling instead.

Closes #2472

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.17%. Comparing base (f225138) to head (d1dd8f8).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2475      +/-   ##
==========================================
- Coverage   98.17%   98.17%   -0.01%     
==========================================
  Files         128      128              
  Lines        4829     4827       -2     
==========================================
- Hits         4741     4739       -2     
  Misses         88       88              
Components Coverage Δ
sentry-ruby 98.57% <ø> (ø)
sentry-rails 97.07% <100.00%> (-0.01%) ⬇️
sentry-sidekiq 96.96% <ø> (ø)
sentry-resque 92.85% <ø> (ø)
sentry-delayed_job 95.65% <ø> (ø)
sentry-opentelemetry 99.31% <ø> (ø)
Files with missing lines Coverage Δ
sentry-rails/lib/sentry/rails/backtrace_cleaner.rb 100.00% <100.00%> (ø)

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Nov 27, 2024

I'm fine with this change (after I test that grouping doesn't break this way either).

But we still need at least the abs_path change in #2474 too because the current handling of abs_path after running backtrace cleaner is not really what we should be sending because it is sometimes already the stripped version. The whole path should be in abs_path and the processed/stripped path should be in filename.

https://github.com/getsentry/relay/blob/bc70b9e3401bccb69e362acf16b9e6bf630aa3a2/relay-event-schema/src/protocol/stacktrace.rs#L74-L82

@sl0thentr0py
Copy link
Member

ok I tested grouping so this is fine to merge.

so the only problem now that I'd ideally like to fix (if possible in a simpler way that #2474) is that this block

add_filter do |line|
line.start_with?(@root) ? line.from(@root.size) : line
end

does not preserve the abs_path for in_app frames as seen here
image

That logic is either way repeated here

prefix =
if under_project_root? && in_app
@project_root
elsif under_project_root?
longest_load_path || @project_root
else
longest_load_path
end
prefix ? abs_path[prefix.to_s.chomp(File::SEPARATOR).length + 1..-1] : abs_path

so are you ok with just removing that filter then?

@st0012 st0012 changed the title Clear Rails' default backtrace filters from Sentry's backtrace cleaner Fix sentry-rails' backtrace cleaner issues Nov 30, 2024
@st0012
Copy link
Collaborator Author

st0012 commented Nov 30, 2024

@sl0thentr0py ok I included that part of the fix too, and updated the PR description accordingly.

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.
This filter removes the project root part from the stacktrace, which prevents
sentry-ruby from retaining the correct absolute path of it.
@sl0thentr0py sl0thentr0py merged commit 0f0666c into master Dec 2, 2024
141 of 142 checks passed
@sl0thentr0py sl0thentr0py deleted the fix-#2472 branch December 2, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Source Code Extraction to Stack Traces (regression via rails)
2 participants