Skip to content

Typo in Curb instrumentation causes ArgumentError #1033

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.

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

Closed
knarewski opened this issue Mar 23, 2022 · 4 comments · Fixed by #1036
Closed

Typo in Curb instrumentation causes ArgumentError #1033

knarewski opened this issue Mar 23, 2022 · 4 comments · Fixed by #1036
Labels
bug community To tag external issues and PRs submitted by the community

Comments

@knarewski
Copy link

Description

When instrumented by NewRelic, following code:

Curl::Easy.new.method(:to_s)

raises `method_with_tracing': wrong number of arguments (given 0, expected 1) (ArgumentError):

Expected Behavior

An object like #<Method: Curl::Easy(Kernel)#to_s()> should be returned

Steps to Reproduce

Paste following code into newrelic_test.rb and invoke ruby newrelic_test.rb

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'curb', '= 1.0.0'
  gem 'newrelic_rpm', '= 8.5.0'
end

Curl::Easy.new.method(:to_s)

Your Environment

ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]

Additional context

I believe the problem can be solved by changing this line to

method_with_tracing(verb) { super }
@knarewski knarewski added the bug label Mar 23, 2022
@github-actions github-actions bot added the community To tag external issues and PRs submitted by the community label Mar 23, 2022
@fallwith
Copy link
Contributor

Hello @knarewski,

Thanks very much for bringing this to our attention. Your explanation of the issue, steps to reproduce, and suggestion for a fix are all terrific and very much appreciated!

I believe you are spot on with your recommended fix. Via the instrumentation.curb setting inside of newrelic.yml, Curb can be instrumented via either prepend or chain. The chain implementation (curb/chain.rb) appears to correctly pass the verb variable over to Curb, but the prepend implementation does not. I believe this is an oversight/typo on our part and that we should be using verb in the manner that you suggest.

Would you be interesting in submitting a GitHub Pull Request against the dev branch of this repository with your recommended fix? The only requirement for doing so would be to sign our Contributor License Agreement when prompted to on the GitHub PR page after submitting. See the Contributor License Agreement section of our contributing guide for more details.

If you would prefer instead for the repo maintainers to make this change, we will be quite happy to do so.

@knarewski
Copy link
Author

Hi @fallwith , thank you for a swift response and for confirming my suspicions! I'd prefer the repo maintainers to make the change if possible, thank you.

@fallwith
Copy link
Contributor

Sure thing, @knarewski, the maintainers will work on the change and keep you posted.

fallwith added a commit that referenced this issue Mar 24, 2022
To address an issue apparently dating back to when the Curb
instrumentation was first split into separate "prepend" and "chain"
approaches, be sure to pass a verb argument to `method_with_tracing`,
which requires it.

Thank you to @knarewski for bringing this issue to our attention,
for providing a means of reproducing an error, and for providing a fix.

resolves #1033
@fallwith
Copy link
Contributor

@knarewski's fix for this bug (thank you!) was merged with #1036 and should become available with the upcoming v8.6.0 release (ETA next 1-3 weeks).

If anyone wishes to leverage the fix before then, they are welcome to use the curb_pass_verb_argument_to_method_with_tracing branch of this repository.

@kbford56 kbford56 moved this to Code Complete/Done in Ruby Engineering Board Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug community To tag external issues and PRs submitted by the community
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants