[rails] improve AR tracing performance

CHANGELOG.md

@@ -1,3 +1,9 @@
+## Unreleased
+
+### Improvements
+
+- Optimize getting query source location in ActiveRecord tracing - this makes tracing up to roughly 40-60% faster depending on the use cases ([#2769](https://github.com/getsentry/sentry-ruby/pull/2769))
+
 ## 6.1.0
 
 ### Features

sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "sentry/rails/tracing/abstract_subscriber"
+require "sentry/backtrace"
 
 module Sentry
   module Rails
@@ -14,9 +15,11 @@ class ActiveRecordSubscriber < AbstractSubscriber
         SUPPORT_SOURCE_LOCATION = ActiveSupport::BacktraceCleaner.method_defined?(:clean_frame)
 
         if SUPPORT_SOURCE_LOCATION
-          class_attribute :backtrace_cleaner, default: (ActiveSupport::BacktraceCleaner.new.tap do |cleaner|
+          backtrace_cleaner = ActiveSupport::BacktraceCleaner.new.tap do |cleaner|
             cleaner.add_silencer { |line| line.include?("sentry-ruby/lib") || line.include?("sentry-rails/lib") }
-          end)
+          end.method(:clean_frame)
+
+          class_attribute :backtrace_cleaner, default: backtrace_cleaner
         end
 
         class << self
@@ -62,43 +65,21 @@ def subscribe!
                 span.set_data(Span::DataConventions::SERVER_PORT, db_config[:port]) if db_config[:port]
                 span.set_data(Span::DataConventions::SERVER_SOCKET_ADDRESS, db_config[:socket]) if db_config[:socket]
 
-                next unless record_query_source
-
                 # both duration and query_source_threshold are in ms
-                next unless duration >= query_source_threshold
-
-                source_location = query_source_location
-
-                if source_location
-                  backtrace_line = Sentry::Backtrace::Line.parse(source_location)
-
-                  span.set_data(Span::DataConventions::FILEPATH, backtrace_line.file) if backtrace_line.file
-                  span.set_data(Span::DataConventions::LINENO, backtrace_line.number) if backtrace_line.number
-                  span.set_data(Span::DataConventions::FUNCTION, backtrace_line.method) if backtrace_line.method
-                  # Only JRuby has namespace in the backtrace
-                  span.set_data(Span::DataConventions::NAMESPACE, backtrace_line.module_name) if backtrace_line.module_name
+                if record_query_source && duration >= query_source_threshold
+                  backtrace_line = Backtrace.source_location(&backtrace_cleaner)
+
+                  if backtrace_line
+                    span.set_data(Span::DataConventions::FILEPATH, backtrace_line.file) if backtrace_line.file
+                    span.set_data(Span::DataConventions::LINENO, backtrace_line.number) if backtrace_line.number
+                    span.set_data(Span::DataConventions::FUNCTION, backtrace_line.method) if backtrace_line.method
+                    # Only JRuby has namespace in the backtrace
+                    span.set_data(Span::DataConventions::NAMESPACE, backtrace_line.module_name) if backtrace_line.module_name
+                  end
                 end
               end
             end
           end
-
-          # Thread.each_caller_location is an API added in Ruby 3.2 that doesn't always collect the entire stack like
-          # Kernel#caller or #caller_locations do. See https://github.com/rails/rails/pull/49095 for more context.
-          if SUPPORT_SOURCE_LOCATION && Thread.respond_to?(:each_caller_location)
-            def query_source_location
-              Thread.each_caller_location do |location|
-                frame = backtrace_cleaner.clean_frame(location)
-                return frame if frame
-              end
-              nil
-            end
-          else
-            # Since Sentry is mostly used in production, we don't want to fallback to the slower implementation
-            # and adds potentially big overhead to the application.
-            def query_source_location
-              nil
-            end
-          end
         end
       end
     end

sentry-ruby/lib/sentry/backtrace.rb

@@ -1,86 +1,12 @@
 # frozen_string_literal: true
 
 require "rubygems"
+require "concurrent/map"
+require "sentry/backtrace/line"
 
 module Sentry
   # @api private
   class Backtrace
-    # Handles backtrace parsing line by line
-    class Line
-      RB_EXTENSION = ".rb"
-      # regexp (optional leading X: on windows, or JRuby9000 class-prefix)
-      RUBY_INPUT_FORMAT = /
-        ^ \s* (?: [a-zA-Z]: | uri:classloader: )? ([^:]+ | <.*>):
-        (\d+)
-        (?: :in\s('|`)(?:([\w:]+)\#)?([^']+)')?$
-      /x
-
-      # org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:170)
-      JAVA_INPUT_FORMAT = /^([\w$.]+)\.([\w$]+)\(([\w$.]+):(\d+)\)$/
-
-      # The file portion of the line (such as app/models/user.rb)
-      attr_reader :file
-
-      # The line number portion of the line
-      attr_reader :number
-
-      # The method of the line (such as index)
-      attr_reader :method
-
-      # The module name (JRuby)
-      attr_reader :module_name
-
-      attr_reader :in_app_pattern
-
-      # 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)
-        ruby_match = unparsed_line.match(RUBY_INPUT_FORMAT)
-
-        if ruby_match
-          _, file, number, _, module_name, method = ruby_match.to_a
-          file.sub!(/\.class$/, RB_EXTENSION)
-          module_name = module_name
-        else
-          java_match = unparsed_line.match(JAVA_INPUT_FORMAT)
-          _, module_name, method, file, number = java_match.to_a
-        end
-        new(file, number, method, module_name, in_app_pattern)
-      end
-
-      def initialize(file, number, method, module_name, in_app_pattern)
-        @file = file
-        @module_name = module_name
-        @number = number.to_i
-        @method = method
-        @in_app_pattern = in_app_pattern
-      end
-
-      def in_app
-        return false unless in_app_pattern
-
-        if file =~ in_app_pattern
-          true
-        else
-          false
-        end
-      end
-
-      # Reconstructs the line in a readable fashion
-      def to_s
-        "#{file}:#{number}:in `#{method}'"
-      end
-
-      def ==(other)
-        to_s == other.to_s
-      end
-
-      def inspect
-        "<Line:#{self}>"
-      end
-    end
-
     # holder for an Array of Backtrace::Line instances
     attr_reader :lines
 
@@ -100,6 +26,48 @@ def self.parse(backtrace, project_root, app_dirs_pattern, &backtrace_cleanup_cal
       new(lines)
     end
 
+    # Thread.each_caller_location is an API added in Ruby 3.2 that doesn't always collect
+    # the entire stack like Kernel#caller or #caller_locations do.
+    #
+    # @see https://github.com/rails/rails/pull/49095 for more context.
+    if Thread.respond_to?(:each_caller_location)
+      def self.source_location(&backtrace_cleaner)
+        Thread.each_caller_location do |location|
+          frame_key = [location.absolute_path, location.lineno]
+          cached_value = line_cache[frame_key]
+
+          next if cached_value == :skip
+
+          if cached_value
+            return cached_value
+          else
+            if cleaned_frame = backtrace_cleaner.(location)
+              line = Line.from_source_location(location)
+              line_cache[frame_key] = line
+
+              return line
+            else
+              line_cache[frame_key] = :skip
+
+              next
+            end
+          end
+        end
+      end
+
+      def self.line_cache
+        @line_cache ||= Concurrent::Map.new
+      end
+    else
+      # Since Sentry is mostly used in production, we don't want to fallback
+      # to the slower implementation and adds potentially big overhead to the
+      # application.
+      def self.source_location(*)
+        nil
+      end
+    end
+
+
     def initialize(lines)
       @lines = lines
     end

sentry-ruby/lib/sentry/backtrace/line.rb

@@ -0,0 +1,99 @@
+# frozen_string_literal: true
+
+module Sentry
+  # @api private
+  class Backtrace
+    # Handles backtrace parsing line by line
+    class Line
+      RB_EXTENSION = ".rb"
+      # regexp (optional leading X: on windows, or JRuby9000 class-prefix)
+      RUBY_INPUT_FORMAT = /
+        ^ \s* (?: [a-zA-Z]: | uri:classloader: )? ([^:]+ | <.*>):
+        (\d+)
+        (?: :in\s('|`)(?:([\w:]+)\#)?([^']+)')?$
+      /x
+
+      # org.jruby.runtime.callsite.CachingCallSite.call(CachingCallSite.java:170)
+      JAVA_INPUT_FORMAT = /^([\w$.]+)\.([\w$]+)\(([\w$.]+):(\d+)\)$/
+
+      # The file portion of the line (such as app/models/user.rb)
+      attr_reader :file
+
+      # The line number portion of the line
+      attr_reader :number
+
+      # The method of the line (such as index)
+      attr_reader :method
+
+      # The module name (JRuby)
+      attr_reader :module_name
+
+      attr_reader :in_app_pattern
+
+      # 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)
+        ruby_match = unparsed_line.match(RUBY_INPUT_FORMAT)
+
+        if ruby_match
+          _, file, number, _, module_name, method = ruby_match.to_a
+          file.sub!(/\.class$/, RB_EXTENSION)
+          module_name = module_name
+        else
+          java_match = unparsed_line.match(JAVA_INPUT_FORMAT)
+          _, module_name, method, file, number = java_match.to_a
+        end
+        new(file, number, method, module_name, in_app_pattern)
+      end
+
+      # Creates a Line from a Thread::Backtrace::Location object
+      # This is more efficient than converting to string and parsing with regex
+      # @param [Thread::Backtrace::Location] location The location object
+      # @param [Regexp, nil] in_app_pattern Optional pattern to determine if the line is in-app
+      # @return [Line] The backtrace line
+      def self.from_source_location(location, in_app_pattern = nil)
+        file = location.absolute_path
+        number = location.lineno
+        method = location.base_label
+
+        label = location.label
+        index = label.index("#") || label.index(".")
+        module_name = label[0, index] if index
+
+        new(file, number, method, module_name, in_app_pattern)
+      end
+
+      def initialize(file, number, method, module_name, in_app_pattern)
+        @file = file
+        @module_name = module_name
+        @number = number.to_i
+        @method = method
+        @in_app_pattern = in_app_pattern
+      end
+
+      def in_app
+        return false unless in_app_pattern
+
+        if file =~ in_app_pattern
+          true
+        else
+          false
+        end
+      end
+
+      # Reconstructs the line in a readable fashion
+      def to_s
+        "#{file}:#{number}:in `#{method}'"
+      end
+
+      def ==(other)
+        to_s == other.to_s
+      end
+
+      def inspect
+        "<Line:#{self}>"
+      end
+    end
+  end
+end

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

@@ -41,4 +41,40 @@
       expect(line.in_app).to eq(false)
     end
   end
+
+  describe ".from_source_location", skip: !Thread.respond_to?(:each_caller_location) do
+    it "creates a Line from Thread::Backtrace::Location" do
+      location = caller_locations.first
+      line = described_class.from_source_location(location, in_app_pattern)
+
+      expect(line).to be_a(described_class)
+      expect(line.file).to be_a(String)
+      expect(line.number).to be_a(Integer)
+      expect(line.method).to be_a(String)
+      expect(line.in_app_pattern).to eq(in_app_pattern)
+    end
+
+    it "extracts file, line number, and method correctly" do
+      location = caller_locations.first
+      line = described_class.from_source_location(location)
+
+      expect(line.file).to eq(location.absolute_path)
+      expect(line.number).to eq(location.lineno)
+      expect(line.method).to eq(location.base_label)
+    end
+
+    it "extracts module name from label when present", when: { ruby_version?: [:>=, "3.4"] } do
+      location = caller_locations.first
+      line = described_class.from_source_location(location)
+
+      expect(line.module_name).to be_a(String)
+    end
+
+    it "skips module name from label when present", when: { ruby_version?: [:<, "3.4"] } do
+      location = caller_locations.first
+      line = described_class.from_source_location(location)
+
+      expect(line.module_name).to be(nil)
+    end
+  end
 end