[rails] improve AR tracing performance #2769
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2769 +/- ##
==========================================
- Coverage 90.28% 90.15% -0.13%
==========================================
Files 131 132 +1
Lines 5258 5281 +23
==========================================
+ Hits 4747 4761 +14
- Misses 511 520 +9
🚀 New features to boost your workflow:
|
solnic
force-pushed
the
solnic/optimize-source-location
branch
3 times, most recently
from
47c1e3a to
cff0932
Compare
solnic
force-pushed
the
solnic/optimize-source-location
branch
from
cff0932 to
160e343
Compare
solnic
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| # 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) |
There was a problem hiding this comment.
Bug: Missing require "sentry/backtrace" causes NameError when calling Backtrace.source_location.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The Backtrace.source_location method is called at sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb:69 without an explicit require "sentry/backtrace" statement. This will result in a NameError: uninitialized constant Backtrace when SUPPORT_SOURCE_LOCATION is true (Ruby 3.2+ with clean_frame method), Sentry.configuration.rails.enable_db_query_source is enabled, and a database query exceeds db_query_source_threshold_ms.
💡 Suggested Fix
Add require "sentry/backtrace" to sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb to ensure the Backtrace constant is defined.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb#L69
Potential issue: The `Backtrace.source_location` method is called at
`sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb:69` without an
explicit `require "sentry/backtrace"` statement. This will result in a `NameError:
uninitialized constant Backtrace` when `SUPPORT_SOURCE_LOCATION` is true (Ruby 3.2+ with
`clean_frame` method), `Sentry.configuration.rails.enable_db_query_source` is enabled,
and a database query exceeds `db_query_source_threshold_ms`.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| index = label.index("#") || label.index(".") | ||
| module_name = label[0, index] if index | ||
|
|
||
| new(file, number, method, module_name, in_app_pattern) |
There was a problem hiding this comment.
Bug: Ruby VersionGuard for Module Name Extraction
The from_source_location method extracts module_name from location.label on all Ruby versions, but the tests and CHANGELOG indicate this should only happen on Ruby 3.4+. Earlier Ruby versions don't include namespace information in the label format, so extracting it may produce incorrect results. The extraction logic needs a Ruby version check to match the documented behavior.
After introducing
enable_db_query_sourcein Rails that's enabled by default ActiveRecord tracing may have potentially became slower, or much slower, depending on the use cases. I believe this is the main culprit causing #2595.This PR optimizes how query source information is obtained by:
Sentry::Backtrace::Linemanually based on the location object that we already haveThis makes a significant difference according to various benchmarks I ran:
master - disabled
enable_db_query_sourcevs enabledthis PR - same benchmark disabled
enabled_db_query_sourcevs enabledThis shows a much smaller impact both in terms of execution time and memory usage.
Rails app benchmarks
I tested this with a simple rails app that just does a bunch of AR queries.
Low intensity
High intensity
Closes #2595