Shopify / statsd-instrument

A StatsD client for Ruby apps. Provides metaprogramming methods to inject StatsD instrumentation into your code.
http://shopify.github.io/statsd-instrument
MIT License
570 stars 94 forks source link

`ArgumentError` in Ruby 3 when instrumenting method with positional and keyword arguments #349

Closed maxveldink closed 1 year ago

maxveldink commented 1 year ago

When trying to instrument a method in the form def my_method(arg1, kwarg1:, kwarg2:), we're receiving an ArgumentError since upgrading to Ruby 3.0.

ArgumentError: wrong number of arguments (given 2, expected 1; required keywords: kwarg1, kwarg2)

Looking at the measure metaprogramming method, this seems to be caused by capturing all arguments in *args and not also anticipating keywords with a **kwargs. (This issue exists for other metaprogramming methods as well.

Forking and updating these methods to look like this fixes the issue for us (we haven't considered how it affects generate_metric_name and generate_tags:

define_method(method) do |*args, **kwargs, &block|
  client ||= StatsD.singleton_client
  key = StatsD::Instrument.generate_metric_name(name, self, *args)
  generated_tags = StatsD::Instrument.generate_tags(tags, self, *args)
  client.measure(key, sample_rate: sample_rate, tags: generated_tags, no_prefix: no_prefix) do
    super(*args, **kwargs, &block)
  end
end

I might be missing something here because I'd expect this to be a more widespread issue with Ruby 3.0+ if teams mix positional and keyword arguments. Are any others having this problem? Happy to take a stab at a PR, but I wanted more input first.

casperisfine commented 1 year ago

I might be missing something here because I'd expect this to be a more widespread issue with Ruby 3.0+

Yes, this is something we're not having a problem with, so I suspect there is something wrong with the method you are trying to patch.

Could you share its signature (you can redact the param name) as well as show the callsite (with params passed).

I suspect your method haven't properly be made compatible with Ruby 3 keywords, and that statsd is being blamed for it because it decorates it.

Also please share the backtrace, because it's impossible here to tell if that ArgumentError happen when calling the decorator or when the decorator call the original method.

maxveldink commented 1 year ago

We identified the issue on our end as being a monkey-patch that we didn't know was in the codebase :| Thank you for the reply @casperisfine 🙏🏻