fix: instrumentation/active_record: add :allow_retry option to find_by_sql patch

[andrewn617]

Rails 7.2 (rails/rails@eabcff2) introduces the :allow_retry option for the find_by_sql method, so we need to add it to this patch to maintain compatibility. The patch doesn't use the method's args, so we can just use (...) to avoid this problem.

To check the Rails version, I had to add rails as a development dependency. I added it in the gemspec and set the minimum version to 6.1 following the pattern I saw in the other libraries in this repo (for example).

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: andrewn617 / name: Andrew Novoselac (8869502)
• ✅ login: kaylareopelle / name: Kayla Reopelle (she/her) (0070364)
• ✅ login: arielvalentin / name: Ariel Valentin (f9aef75)

[dmathieu]

Do we need the condition?
If we provide the new option no matter the rails version, and someone is using a version lower than 7.2, super will fail just as it would if the instrumentation wasn't there.

[arielvalentin]

Do we need the condition?

If we provide the new option no matter the rails version, and someone is using a version lower than 7.2, super will fail just as it would if the instrumentation wasn't there.

I think we do. We must honor the monkey patched class method signatures.

Without that condition, the side effect will be changing the public API to add an option that does not exist. Though it will be backward compatible it will confuse end users and if they disable the instrumentation it will break their application.

I am not keen on the if statements and would probably prefer mixing in separate modules depending on the gem versions but I'd like the original author @robertlaurin to weigh in on that.

[arielvalentin]

Are there other methods here that need the same treatment?

[andrewn617]

No it does not look like it.

[kaylareopelle]

Thank you, @andrewn617! Are there existing tests that cover this change?

[andrewn617]

Thank you, @andrewn617! Are there existing tests that cover this change?

@kaylareopelle I think now that we are just using (...) there is nothing to test, since it is just ruby functionality to forward all the args to when calling super.

[arielvalentin]

Seems to me like we need to do something similar for these to future proof API changes:

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/00703640ef59799bfcca0e24dad3e38bb17e1be9/instrumentation/active_record/lib/opentelemetry/instrumentation/active_record/patches/persistence_class_methods.rb

[arielvalentin]

@andrewn617 I did miss @kaylareopelle comment about adding code coverage.

Would you kindly add a test that uses allow_retry as a parameter to ensure we do not introduce a regression in the future?

[arielvalentin]

hmmm actually this is edge rails so we cannot really test this at the moment.