Don't let backtrace_cleanup_callback affect abs_path and separate filename handling

CHANGELOG.md

@@ -17,6 +17,8 @@
 - Fix `send_default_pii` handling in rails controller spans [#2443](https://github.com/getsentry/sentry-ruby/pull/2443)
   - Fixes [#2438](https://github.com/getsentry/sentry-ruby/issues/2438)
 - Fix `RescuedExceptionInterceptor` to handle an empty configuration [#2428](https://github.com/getsentry/sentry-ruby/pull/2428)
+- Don't let `backtrace_cleanup_callback` affect `abs_path` and separate filename handling ([#2474](https://github.com/getsentry/sentry-ruby/pull/2474))
+  - Fixes [#2472](https://github.com/getsentry/sentry-ruby/issues/2472)
 
 ## 5.21.0
 

sentry-rails/spec/sentry/rails_spec.rb

@@ -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 }

sentry-ruby/lib/sentry/backtrace.rb

@@ -18,6 +18,9 @@
       # org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:170)
       JAVA_INPUT_FORMAT = /^(.+)\.([^\.]+)\(([^\:]+)\:(\d+)\)$/
 
+      # The absolute path in the backtrace
+      attr_reader :abs_path
+
       # The file portion of the line (such as app/models/user.rb)
       attr_reader :file
 
@@ -35,31 +38,45 @@
       # Parses a single line of a given backtrace
       # @param [String] unparsed_line The raw line from +caller+ or some backtrace
       # @return [Line] The parsed backtrace line
-      def self.parse(unparsed_line, in_app_pattern = nil)
+      def self.parse(unparsed_line, in_app_pattern = nil, cleaned_line = nil)
         ruby_match = unparsed_line.match(RUBY_INPUT_FORMAT)
         if ruby_match
-          _, file, number, _, method = ruby_match.to_a
-          file.sub!(/\.class$/, RB_EXTENSION)
+          _, abs_path, number, _, method = ruby_match.to_a
+          abs_path.sub!(/\.class$/, RB_EXTENSION)
           module_name = nil
+
+          cleaned_match = cleaned_line.match(RUBY_INPUT_FORMAT) if cleaned_line
+          if cleaned_match
+            # prefer data from cleaned line
+            _, file, number, _, method = cleaned_match.to_a
+            file.sub!(/\.class$/, RB_EXTENSION)
+          end
         else
           java_match = unparsed_line.match(JAVA_INPUT_FORMAT)
-          _, module_name, method, file, number = java_match.to_a
+          _, module_name, method, abs_path, number = java_match.to_a
+
+
+          cleaned_match = cleaned_line.match(JAVA_INPUT_FORMAT) if cleaned_line
+          # prefer data from cleaned line
+          _, module_name, method, file, number = cleaned_match.to_a if cleaned_match
         end
-        new(file, number, method, module_name, in_app_pattern)
+
+        new(abs_path, number, method, module_name, in_app_pattern, file || abs_path)
       end
 
-      def initialize(file, number, method, module_name, in_app_pattern)
-        @file = file
+      def initialize(abs_path, number, method, module_name, in_app_pattern, file = nil)
+        @abs_path = abs_path
         @module_name = module_name
         @number = number.to_i
         @method = method
         @in_app_pattern = in_app_pattern
+        @file = file
       end
 
       def in_app
         return false unless in_app_pattern
 
-        if file =~ in_app_pattern
+        if abs_path =~ in_app_pattern
           true
         else
           false
@@ -86,14 +103,14 @@
     def self.parse(backtrace, project_root, app_dirs_pattern, &backtrace_cleanup_callback)
       ruby_lines = backtrace.is_a?(Array) ? backtrace : backtrace.split(/\n\s*/)
 
-      ruby_lines = backtrace_cleanup_callback.call(ruby_lines) if backtrace_cleanup_callback
+      cleaned_lines = backtrace_cleanup_callback ? backtrace_cleanup_callback.call(ruby_lines) : []
 
       in_app_pattern ||= begin
         Regexp.new("^(#{project_root}/)?#{app_dirs_pattern}")
       end
 
-      lines = ruby_lines.to_a.map do |unparsed_line|
-        Line.parse(unparsed_line, in_app_pattern)
+      lines = ruby_lines.to_a.zip(cleaned_lines).map do |unparsed_line, cleaned_line|
+        Line.parse(unparsed_line, in_app_pattern, cleaned_line)
       end
 
       new(lines)

sentry-ruby/lib/sentry/interfaces/stacktrace.rb

@@ -31,32 +31,33 @@ def initialize(project_root, line, strip_backtrace_load_path = true)
         @project_root = project_root
         @strip_backtrace_load_path = strip_backtrace_load_path
 
-        @abs_path = line.file
+        @abs_path = line.abs_path
         @function = line.method if line.method
         @lineno = line.number
         @in_app = line.in_app
         @module = line.module_name if line.module_name
-        @filename = compute_filename
+
+        @filename = compute_filename(line.file)
       end
 
       def to_s
         "#{@filename}:#{@lineno}"
       end
 
-      def compute_filename
-        return if abs_path.nil?
-        return abs_path unless @strip_backtrace_load_path
+      def compute_filename(file)
+        return if file.nil?
+        return file unless @strip_backtrace_load_path
 
+        under_project_root = under_project_root?(file)
         prefix =
-          if under_project_root? && in_app
+          if under_project_root && in_app
             @project_root
-          elsif under_project_root?
-            longest_load_path || @project_root
           else
-            longest_load_path
+            longest_load_path = longest_load_path(file)
+            under_project_root ? longest_load_path || @project_root : longest_load_path
           end
 
-        prefix ? abs_path[prefix.to_s.chomp(File::SEPARATOR).length + 1..-1] : abs_path
+        prefix ? file[prefix.to_s.chomp(File::SEPARATOR).length + 1..-1] : file
       end
 
       def set_context(linecache, context_lines)
@@ -77,12 +78,12 @@ def to_hash(*args)
 
       private
 
-      def under_project_root?
-        @project_root && abs_path.start_with?(@project_root)
+      def under_project_root?(file)
+        @project_root && file.start_with?(@project_root)
       end
 
-      def longest_load_path
-        $LOAD_PATH.select { |path| abs_path.start_with?(path.to_s) }.max_by(&:size)
+      def longest_load_path(file)
+        $LOAD_PATH.select { |path| file.start_with?(path.to_s) }.max_by(&:size)
       end
     end
   end

sentry-ruby/spec/sentry/backtrace/lines_spec.rb

@@ -12,6 +12,9 @@
   let(:unparsed_gem_line) do
     "/PATH_TO_RUBY/gems/2.7.0/gems/sinatra-2.1.0/lib/sinatra/base.rb:1675:in `call'"
   end
+  let(:cleaned_gem_line) do
+    "sinatra (2.1.0) lib/sinatra/base.rb:1675:in `call'"
+  end
 
   let(:in_app_pattern) do
     project_root = Sentry.configuration.project_root&.to_s
@@ -22,6 +25,7 @@
     it "parses app backtrace correctly" do
       line = described_class.parse(unparsed_app_line, in_app_pattern)
 
+      expect(line.abs_path).to eq("app.rb")
       expect(line.file).to eq("app.rb")
       expect(line.number).to eq(12)
       expect(line.method).to eq("/")
@@ -33,6 +37,9 @@
     it "parses gem backtrace correctly" do
       line = described_class.parse(unparsed_gem_line, in_app_pattern)
 
+      expect(line.abs_path).to eq(
+        "/PATH_TO_RUBY/gems/2.7.0/gems/sinatra-2.1.0/lib/sinatra/base.rb"
+      )
       expect(line.file).to eq(
         "/PATH_TO_RUBY/gems/2.7.0/gems/sinatra-2.1.0/lib/sinatra/base.rb"
       )
@@ -42,5 +49,21 @@
       expect(line.module_name).to eq(nil)
       expect(line.in_app).to eq(false)
     end
+
+    it "parses cleaned gem backtrace correctly" do
+      line = described_class.parse(unparsed_gem_line, in_app_pattern, cleaned_gem_line)
+
+      expect(line.abs_path).to eq(
+        "/PATH_TO_RUBY/gems/2.7.0/gems/sinatra-2.1.0/lib/sinatra/base.rb"
+      )
+      expect(line.file).to eq(
+        "sinatra (2.1.0) lib/sinatra/base.rb"
+      )
+      expect(line.number).to eq(1675)
+      expect(line.method).to eq("call")
+      expect(line.in_app_pattern).to eq(in_app_pattern)
+      expect(line.module_name).to eq(nil)
+      expect(line.in_app).to eq(false)
+    end
   end
 end

sentry-ruby/spec/sentry/interfaces/stacktrace_spec.rb

@@ -18,27 +18,69 @@
     it "initializes a Frame with the correct info from the given Backtrace::Line object" do
       first_frame = Sentry::StacktraceInterface::Frame.new(configuration.project_root, lines.first)
 
-      expect(first_frame.filename).to match(/base.rb/)
+      expect(first_frame.abs_path).to eq("#{Dir.home}/.rvm/gems/activerecord/base.rb")
+      expect(first_frame.filename).to eq("#{Dir.home}/.rvm/gems/activerecord/base.rb")
       expect(first_frame.in_app).to eq(false)
       expect(first_frame.function).to eq("save")
       expect(first_frame.lineno).to eq(10)
 
       second_frame = Sentry::StacktraceInterface::Frame.new(configuration.project_root, lines.last)
 
-      expect(second_frame.filename).to match(/post.rb/)
+      expect(second_frame.abs_path).to match("#{configuration.project_root}/app/models/post.rb")
+      expect(second_frame.filename).to match("app/models/post.rb")
       expect(second_frame.in_app).to eq(true)
       expect(second_frame.function).to eq("save_user")
       expect(second_frame.lineno).to eq(5)
     end
 
     it "does not strip load path when strip_backtrace_load_path is false" do
       first_frame = Sentry::StacktraceInterface::Frame.new(configuration.project_root, lines.first, false)
+      expect(first_frame.abs_path).to eq(first_frame.abs_path)
       expect(first_frame.filename).to eq(first_frame.abs_path)
       expect(first_frame.filename).to eq(raw_lines.first.split(':').first)
 
       second_frame = Sentry::StacktraceInterface::Frame.new(configuration.project_root, lines.last, false)
+      expect(second_frame.abs_path).to eq(second_frame.abs_path)
       expect(second_frame.filename).to eq(second_frame.abs_path)
       expect(second_frame.filename).to eq(raw_lines.last.split(':').first)
     end
+
+    context "with backtrace cleaner" do
+      let(:backtrace_cleanup_callback) do
+        proc do |backtrace|
+          backtrace.map do |line|
+            if line.match(/gems/)
+              line.sub("#{Dir.home}/.rvm/gems/", "")
+            elsif line.start_with?(configuration.project_root)
+              line.sub(configuration.project_root, "")
+            else
+              line
+            end
+          end
+        end
+      end
+
+      let(:lines) do
+        Sentry::Backtrace.parse(raw_lines, configuration.project_root, configuration.app_dirs_pattern, &backtrace_cleanup_callback).lines
+      end
+
+      it "handles raw and cleaned lines properly" do
+        first_frame = Sentry::StacktraceInterface::Frame.new(configuration.project_root, lines.first)
+
+        expect(first_frame.abs_path).to eq("#{Dir.home}/.rvm/gems/activerecord/base.rb")
+        expect(first_frame.filename).to eq("activerecord/base.rb")
+        expect(first_frame.in_app).to eq(false)
+        expect(first_frame.function).to eq("save")
+        expect(first_frame.lineno).to eq(10)
+
+        second_frame = Sentry::StacktraceInterface::Frame.new(configuration.project_root, lines.last)
+
+        expect(second_frame.abs_path).to match("#{configuration.project_root}/app/models/post.rb")
+        expect(second_frame.filename).to match("app/models/post.rb")
+        expect(second_frame.in_app).to eq(true)
+        expect(second_frame.function).to eq("save_user")
+        expect(second_frame.lineno).to eq(5)
+      end
+    end
   end
 end