-
-
Notifications
You must be signed in to change notification settings - Fork 520
[rails] improve AR tracing performance #2769
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2769 +/- ##
==========================================
- Coverage 90.28% 90.18% -0.10%
==========================================
Files 131 132 +1
Lines 5258 5280 +22
==========================================
+ Hits 4747 4762 +15
- Misses 511 518 +7
🚀 New features to boost your workflow:
|
47c1e3a to
cff0932
Compare
cff0932 to
160e343
Compare
sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb
Outdated
Show resolved
Hide resolved
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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