From b6e0d2f00fe17b6af70aaf462b03f64a23e54e3f Mon Sep 17 00:00:00 2001 From: Albert Llop Date: Thu, 26 Sep 2024 18:32:45 +0200 Subject: [PATCH 1/5] fix: activerecord find_by_sql spans on Rails 7.0+ --- .../active_record/patches/querying.rb | 14 +++++++++++--- .../active_record/patches/querying_test.rb | 10 ++++++++-- instrumentation/active_record/test/test_helper.rb | 13 ++++++++++++- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb b/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb index 41280b1f1..e1dd218aa 100644 --- a/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb +++ b/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb @@ -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 + super + end + end + else + def find_by_sql(...) + tracer.in_span("#{self}.find_by_sql") do + super + end end end diff --git a/instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb b/instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb index 6da620512..40f7cd548 100644 --- a/instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb +++ b/instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb @@ -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 diff --git a/instrumentation/active_record/test/test_helper.rb b/instrumentation/active_record/test/test_helper.rb index 6cf6d2757..754896c8c 100644 --- a/instrumentation/active_record/test/test_helper.rb +++ b/instrumentation/active_record/test/test_helper.rb @@ -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) } @@ -54,9 +60,14 @@ 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 do + t.timestamps + end + create_table :users do |t| t.string 'name' t.integer 'counter' + t.references 'account' t.timestamps end From fbce9bc0ddab923bda120469a7c6f3bb649c549b Mon Sep 17 00:00:00 2001 From: Albert Llop Date: Tue, 1 Oct 2024 21:34:04 +0200 Subject: [PATCH 2/5] tests: fix create_table call --- instrumentation/active_record/test/test_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/active_record/test/test_helper.rb b/instrumentation/active_record/test/test_helper.rb index 754896c8c..f7157eafe 100644 --- a/instrumentation/active_record/test/test_helper.rb +++ b/instrumentation/active_record/test/test_helper.rb @@ -60,7 +60,7 @@ 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 do + create_table :accounts do |t| t.timestamps end From beff0cbce890da4c5d2757cc77f17767bfe4b0c2 Mon Sep 17 00:00:00 2001 From: Albert Llop Date: Wed, 2 Oct 2024 00:42:47 +0200 Subject: [PATCH 3/5] tests: rubocop tweak --- instrumentation/active_record/test/test_helper.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/instrumentation/active_record/test/test_helper.rb b/instrumentation/active_record/test/test_helper.rb index f7157eafe..02751e7f6 100644 --- a/instrumentation/active_record/test/test_helper.rb +++ b/instrumentation/active_record/test/test_helper.rb @@ -60,9 +60,7 @@ 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 do |t| - t.timestamps - end + create_table :accounts, &:timestamps create_table :users do |t| t.string 'name' From 73b1f82cbb6ab71fb41db4f94676adc927687b60 Mon Sep 17 00:00:00 2001 From: Albert Llop Date: Thu, 3 Oct 2024 11:19:07 +0200 Subject: [PATCH 4/5] feat: call spans that produce reads `query` --- .../instrumentation/active_record/patches/querying.rb | 4 ++-- .../instrumentation/active_record/patches/querying_test.rb | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb b/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb index e1dd218aa..bbcfc070e 100644 --- a/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb +++ b/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb @@ -20,13 +20,13 @@ class << base module ClassMethods if ::ActiveRecord.version >= Gem::Version.new('7.0.0') def _query_by_sql(...) - tracer.in_span("#{self}.find_by_sql") do + tracer.in_span("#{self} query") do super end end else def find_by_sql(...) - tracer.in_span("#{self}.find_by_sql") do + tracer.in_span("#{self} query") do super end end diff --git a/instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb b/instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb index 40f7cd548..879af50e6 100644 --- a/instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb +++ b/instrumentation/active_record/test/instrumentation/active_record/patches/querying_test.rb @@ -15,15 +15,15 @@ before { exporter.reset } - describe 'find_by_sql' do + describe 'query' 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' } + user_find_spans = spans.select { |s| s.name == 'User query' } + account_find_span = spans.find { |s| s.name == 'Account query' } _(user_find_spans.length).must_equal(2) _(account_find_span).wont_be_nil From 1f30c8069a6ae7d3854287ae4cdebf959539ae8f Mon Sep 17 00:00:00 2001 From: Albert Llop Date: Mon, 14 Oct 2024 12:27:16 +0200 Subject: [PATCH 5/5] Refactor patching to use define_method --- .../active_record/patches/querying.rb | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb b/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb index bbcfc070e..094575ae4 100644 --- a/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb +++ b/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb @@ -18,17 +18,11 @@ class << base # Contains ActiveRecord::Querying to be patched module ClassMethods - if ::ActiveRecord.version >= Gem::Version.new('7.0.0') - def _query_by_sql(...) - tracer.in_span("#{self} query") do - super - end - end - else - def find_by_sql(...) - tracer.in_span("#{self} query") do - super - end + method_name = ::ActiveRecord.version >= Gem::Version.new('7.0.0') ? :_query_by_sql : :find_by_sql + + define_method(method_name) do |*args, **kwargs| + tracer.in_span("#{self} query") do + super(*args, **kwargs) end end