Skip to content

fix!: activerecord find_by_sql spans on Rails 7.0+ #1184

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Oct 22, 2024
Merged

fix!: activerecord find_by_sql spans on Rails 7.0+ #1184

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b6e0d2f
fix: activerecord find_by_sql spans on Rails 7.0+
mrsimo Sep 26, 2024
fbce9bc
tests: fix create_table call
mrsimo Oct 1, 2024
beff0cb
tests: rubocop tweak
mrsimo Oct 1, 2024
18c3d5a
Merge branch 'main' into instrumentation-activerecord-find-by-sql
arielvalentin Oct 1, 2024
73b1f82
feat: call spans that produce reads `query`
mrsimo Oct 3, 2024
049c179
Merge branch 'main' into instrumentation-activerecord-find-by-sql
mrsimo Oct 3, 2024
1f30c80
Refactor patching to use define_method
mrsimo Oct 14, 2024
f45ab3c
Merge branch 'main' into instrumentation-activerecord-find-by-sql
mrsimo Oct 14, 2024
f5aad40
Merge branch 'main' into instrumentation-activerecord-find-by-sql
kaylareopelle Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions ...ntation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,17 @@ class << base

# Contains ActiveRecord::Querying to be patched
module ClassMethods
def find_by_sql(...)
tracer.in_span("#{self}.find_by_sql") do
super
if ::ActiveRecord.version >= Gem::Version.new('7.0.0')
def _query_by_sql(...)
tracer.in_span("#{self}.find_by_sql") do
Copy link
Contributor

@kaylareopelle kaylareopelle Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get what you're saying about the naming! If we change the name of the span, we should consider marking this update as a breaking change since all 7.0 spans would now have a new name.

I like your suggestion of query for 7.0+ spans since they don't roll up to find_by_sql.

Unfortunately, instrumentation often has to resort to private methods to get the data users want, so _query_by_sql probably wouldn't be too much of a shock and perhaps easier for a user to connect the dots.

I'd love to hear other opinions!

Copy link
Contributor Author

@mrsimo mrsimo Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like query (or similar) would be the most stable for the future. Since we're hooking onto internal AR methods, it's possible we might need to change methods, needing a breaking change every time.

The downside is that all other spans being generated have the name of the method we're hooking into, so it would be a bit inconsistent. We could make it configurable?

As a user of this instrumentation I'd lean towards something more generic and stable. I usually have the mysql2 or trilogy spans with details about the exact query that ran, and as a developer I'd need to open the rails code to know what _query_by_sql even is. Even the find_by_sql was a bit confusing, I didn't know all reads (used to) go through that method.

Copy link
Contributor

@kaylareopelle kaylareopelle Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I'm leaning toward query now too!

I hesitate to make it configurable. We have some configuration around span naming for ActiveSupport::Notifications in some of our Rails-related instrumentation gems, but in this scenario, since the span is not related to ActiveSupport::Notifications, I think having a single, consistent name would be best.

Copy link
Contributor Author

@mrsimo mrsimo Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a change to use query instead of find_by_sql. Spans are now called User query, Account query, etc. I'm liking it!

Copy link
Contributor

@kaylareopelle kaylareopelle Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to hear! Would you mind updating the PR title to mark this as a breaking change? I think this'll help anyone who uses find_by_sql in their observability backends know to update things accordingly.

It can be marked as a breaking change by updating the title to start with fix!: instead of fix:

If you don't think this should be a breaking change, let me know and we can discuss it further.

Copy link
Contributor Author

@mrsimo mrsimo Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Totally agree that it's a breaking change.

super
end
end
else
def find_by_sql(...)
tracer.in_span("#{self}.find_by_sql") do
super
end
end
end

Expand Down
10 changes: 8 additions & 2 deletions instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,16 @@

describe 'find_by_sql' do
it 'traces' do
Account.create!

User.find_by_sql('SELECT * FROM users')
Account.first.users.to_a

user_find_spans = spans.select { |s| s.name == 'User.find_by_sql' }
account_find_span = spans.find { |s| s.name == 'Account.find_by_sql' }

find_span = spans.find { |s| s.name == 'User.find_by_sql' }
_(find_span).wont_be_nil
_(user_find_spans.length).must_equal(2)
_(account_find_span).wont_be_nil
end
end
end
11 changes: 10 additions & 1 deletion instrumentation/active_record/test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,14 @@
database: 'db/development.sqlite3'
)

# Create User model
# Create ActiveRecord models
class Account < ActiveRecord::Base
has_many :users
end

class User < ActiveRecord::Base
belongs_to :account

validate :name_if_present

scope :recently_created, -> { where('created_at > ?', Time.now - 3600) }
Expand All @@ -54,9 +60,12 @@ class SuperUser < ActiveRecord::Base; end
# Simple migration to create a table to test against
class CreateUserTable < ActiveRecord::Migration[migration_version]
def change
create_table :accounts, &:timestamps

create_table :users do |t|
t.string 'name'
t.integer 'counter'
t.references 'account'
t.timestamps
end

Expand Down