diff --git a/CHANGELOG.md b/CHANGELOG.md index b99e80414..327242a8f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb b/sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb index 09468b6cc..0f985789c 100644 --- a/sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb +++ b/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 diff --git a/sentry-ruby/lib/sentry/backtrace.rb b/sentry-ruby/lib/sentry/backtrace.rb index b0a74cd9e..73a77acfd 100644 --- a/sentry-ruby/lib/sentry/backtrace.rb +++ b/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 - "" - 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 diff --git a/sentry-ruby/lib/sentry/backtrace/line.rb b/sentry-ruby/lib/sentry/backtrace/line.rb new file mode 100644 index 000000000..2a9f6c96e --- /dev/null +++ b/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 + "" + end + end + end +end diff --git a/sentry-ruby/spec/sentry/backtrace/lines_spec.rb b/sentry-ruby/spec/sentry/backtrace/lines_spec.rb index fa331e10d..ff0fc495b 100644 --- a/sentry-ruby/spec/sentry/backtrace/lines_spec.rb +++ b/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