fix: Pass block explicitly in define_method calls for PG instrumentation query methods
#574
Conversation
…uery methods open-telemetry#570 "As a result of the fact blocks are captured by closures they cannot be passed implicitly to methods created with define_method; they must instead be passed explicitly using the define_method(meth) { |args, &block| ... } syntax." from https://banisterfiend.wordpress.com/2010/11/06/behavior-of-yield-in-define_method/
Capncavedan
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam and
robbkidd
as code owners
|
@Capncavedan thanks again for your help! Please review and sign the Easy CLA so that we may start the PR review process. |
|
@arielvalentin done |
|
Thank you. Did you happen to have the ability to test this in a sample app? If so would you be able to share some screen shots of the fix in action? |
|
@arielvalentin Only in the repro script so far. This bug manifested up in a background process of ours, and we patched it to process its Postgres queries differently. Apologies if this is already clear - the bug isn't affecting telemetry, rather one specific way of issuing Postgres queries using the PG gem. |
arielvalentin
changed the title
define_method calls for PG instrumentation query methodsdefine_method calls for PG instrumentation query methods
There was a problem hiding this comment.
Thank you for this contribution!
Please update your commit messages to use conventional commits: https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/CONTRIBUTING.md#use-conventional-commit-messages
instrumentation/pg/test/opentelemetry/instrumentation/pg/patches/connection_test.rb
Outdated
Show resolved
Hide resolved
instrumentation/pg/test/opentelemetry/instrumentation/pg/patches/connection_test.rb
Show resolved
Hide resolved
| require_relative '../../../../../lib/opentelemetry/instrumentation/pg' | ||
| require_relative '../../../../../lib/opentelemetry/instrumentation/pg/patches/connection' | ||
|
|
||
| describe OpenTelemetry::Instrumentation::PG::Patches do |
There was a problem hiding this comment.
✨ Thank you for adding tests for this!
|
Fantastic. I’ll incorporate feedback early next week (I’m out this week) and update the PR. -DanOn Jul 25, 2023, at 14:12, kaylareopelle ***@***.***> wrote:
@kaylareopelle commented on this pull request.
This looks good to me once require 'active_record' is removed from the tests.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@Capncavedan enjoy your holiday! I have committed the suggestion knowing you are ok with it via the GitHub PR suggestion feature. |
|
Thanks @arielvalentin ! |
Capncavedan
deleted the
patch-block-syntax-result-match-dot-method-result
branch
#570
"As a result of the fact blocks are captured by closures they cannot be passed implicitly to methods created with
define_method; they must instead be passed explicitly using thedefine_method(meth) { |args, &block| ... }syntax."from https://banisterfiend.wordpress.com/2010/11/06/behavior-of-yield-in-define_method/
This PR changes the "exec-ish" and "exec-prepared-ish" methods to all optionally handle an explicit block, with a new separate test file specifically for these PG::Connection patches.
This PR does not change the "prepare-ish" methods as those don't seem to return anything anyway.