New ActiveRecord `find_by_sql` patch fails to pass `&block` argument

[olepbr]

Description of the bug

The new patch for ActiveRecord's find_by_sql, added in #1184 and released in opentelemetry-instrumentation-active_record v0.8.0 alters application behaviour. The PR changed the patch from using argument forwarding (def find_by_sql(...)) to using define_method with explicitly passed arguments:

opentelemetry-ruby-contrib/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/querying.rb

Lines 20 to 27 in bf6394d

	module ClassMethods
	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

The problem with this approach is that the function signature of find_by_sql looks like this:

# ActiveRecord v6 & v7
def find_by_sql(sql, binds = [], preparable: nil, &block)
# ActiveRecord v8
def find_by_sql(sql, binds = [], preparable: nil, allow_retry: false, &block)

Notice that find_by_sql takes &block as its final argument, which is neither a positional nor a keyword argument. The old argument forwarding approach automatically forwards &block as well, but the new patch's |*args, **kwargs| is not equivalent. As a result, the block is not passed to the super() call and hence not executed. I noticed this because opentelemetry-instrumentation-active_record v0.8.0 breaks some specs in a Rails application I work on; downgrading to v0.7.4 makes the specs pass again.

I believe just passing &block to the do block and specifying it in the super() call is all that is needed to resolve this:

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 094575ae..0b9f3790 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,9 +20,9 @@ module OpenTelemetry
           module ClassMethods
             method_name = ::ActiveRecord.version >= Gem::Version.new('7.0.0') ? :_query_by_sql : :find_by_sql

-            define_method(method_name) do |*args, **kwargs|
+            define_method(method_name) do |*args, **kwargs, &block|
               tracer.in_span("#{self} query") do
-                super(*args, **kwargs)
+                super(*args, **kwargs, &block)
               end
             end

Monkey-patching the gem with the above diff makes my specs pass, at least.

Share details about your runtime

Operating system details: Debian GNU/Linux trixie (testing)
RUBY_ENGINE: "ruby"
RUBY_VERSION: "3.0.6"
RUBY_DESCRIPTION: "ruby 3.0.6p216 (2023-03-30 revision 23a532679b) [x86_64-linux]"

Share a simplified reproduction if possible

I'm not familiar enough with OpenTelemetry or ActiveRecord's internals to provide a minimal repro, I'm sorry. I hope the above description and suggested fix help, at least.

All the best,
Ole

[arielvalentin]

cc: @mrsimo

[arielvalentin]

@olepbr if you are available to put together a bug fix PR that would be amazing! otherwise I think we need to revert the changes from the previous PR ASAP cc: @open-telemetry/ruby-contrib-approvers

[mrsimo]

Thanks for pinging me @arielvalentin ❤️ . It's very close to my EOD, but I'd be happy to work on a patch tomorrow morning. This is broken in Rails 8.0 which was released very recently, and I don't see it in the Appraisals file. It might have been caught, let's make sure we add 8.0 there!

If we can wait a day to roll forward with this instead of reverting it seems like an easy enough fix?

[olepbr]

EOD here too, but added the patch in #1259. Need to leave now; will sign CLA later.

[mrsimo]

I wanted to mention that I now see the &block parameter was also there in versions other than Rails 8, I don't know why I thought it was a new Rails 8.0 thing.