Allow Yabeda::Counter#increment to be called without argument

[goalaleo]

Hi, this is a small feature request that should be backwards compatible.

Issue

We have some counters in our app that we're not using any tags with. These counters are currently incremented like this

Yabeda.some_counter.increment({})

The increment method has one required positional argument -tags- which has no default value. https://github.com/yabeda-rb/yabeda/blob/ab2be2235253ed6e0d45a4fa97e4decaa32be480/lib/yabeda/counter.rb#L6

Suggestion

Define tags to have a default value of {} so that increment can be called without parentheses

def increment(tags = {}, by: 1)

Yabeda.some_counter.increment

[Envek]

Hmm. From the one hand sounds reasonable.

From the other – it is quite rare situation when you really need “global” counter without any segmentation. In that case missing required argument tries to remind you that you possibly forget to add tags.

[chrp]

+1 For this. We actually have some global counters where there is no segmentation. Seemed kind of unexpected to have to declare "no tags needed here" explicitly.

[goalaleo]

What if this was configurable and defaulted to not allow it?

def increment(tags = :none, by: 1)
  if tags == :none && Yabeda.config.implicit_empty_tags_for_increment
    tags = {}
  else
    raise ArgumentError, <<~ ERROR_MSG
      if there are no tags you must explicitly pass tags as an empty hash or set

        Yabeda.config.implicit_empty_tags_for_increment = true

      to use an empty hash as the default value  
    ERROR MSG
  end

  # ...
end

it is quite rare situation when you really need “global” counter without any segmentation

This probably varies between apps. In our app we have 59 calls and 15 use no tags so that's ~25% of calls. You could also argue that the counter isn't really "global" because the counter has a name so the name itself differentiates the counter from other counters

[Envek]

Sorry for the long wait. I finally got persuaded.

Released in 0.13.0, thanks!

[magec]

This has broken rails integration I believe:

This is a debug session on the rails integration:

(byebug) event.labels
{:controller=>"interface", :action=>"app", :status=>200, :format=>:html, :method=>"get"}
(byebug) rails_requests_total.increment(event.labels)
*** ArgumentError Exception: unknown keywords: :controller, :action, :status, :format, :method

nil

Will rollback to 0.12

[Envek]

This has broken rails integration I believe

Hmm, can't imagine how adding default value to argument could break anything.

But #38 from the same release could break rails-related stuff.

[magec]

Well, the problem here is ruby 2.x.

With ruby 2 The code:

def method_with_default_assignment(tags = {}, by:1)
  puts tags.inspect
end

def method_without_default_assignment(tags, by:1)
  puts tags.inspect
end

The usage:

irb(main):001:0> def method_without_default_assignment(tags, by:1)
irb(main):002:1>   puts tags.inspect
irb(main):003:1>   end
=> :method_without_default_assignment
irb(main):004:0> def method_with_default_assignment(tags = {}, by: 1)
irb(main):005:1>   puts tags.inspect
irb(main):006:1>   end
=> :method_with_default_assignment
irb(main):007:0> a_tags_hash =  {controller: 'hello', action: 'world'}
=> {:controller=>"hello", :action=>"world"}
irb(main):008:0> method_with_default_assignment(a_tags_hash)
Traceback (most recent call last):
        20: from /home/jose/.rbenv/versions/2.7.7/bin/irb:23:in `<main>'
        19: from /home/jose/.rbenv/versions/2.7.7/bin/irb:23:in `load'
        18: from /home/jose/.gem/ruby/2.7.0/gems/irb-1.3.5/exe/irb:11:in `<top (required)>'
         1: from (irb):8:in `<main>'
(irb):4:in `method_with_default_assignment': unknown keywords: :controller, :action (ArgumentError)
irb(main):009:0> method_without_default_assignment(a_tags_hash)
{:controller=>"hello", :action=>"world"}
=> nil
irb(main):011:0> method_with_default_assignment(a_tags_hash, by: 2)
{:controller=>"hello", :action=>"world"}
=> nil

In Ruby 3 works well.

The issue with ruby 2 is that it seems that it thinks that when you are not passing the by (because you want the default value) it kind of assumes the tags hash is the keywords args hash and thus it returns an exception.

A possible solution would be:

def increment(tags = {}, **kwargs)
   by = kwargs[:by] || 1
   ....

:man_shrugging:

[Envek]

Thanks for your analysis, @magec.

Actually I can't see a way how to fix it for Ruby 2.x due to the way how differently Ruby 2.x handles keyword arguments for methods with arguments with defaults (sic!)

For method defined like this:

def increment(tags = {}, **kwargs)
# or even
def increment(tags = nil, by: 1)

Invoking it with tags (with keys as symbols) but without keyword arguments will always promote positional hash to keyword argument. :exploding_head:

increment({foo: :bar})
# unknown keyword: :foo (ArgumentError)

It is because of:

In Ruby 2, keyword arguments can be treated as the last positional Hash argument and a last positional Hash argument can be treated as keyword arguments.

See https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

It is interesting that it only happens when positional argument has any default (not shouldn't be a hash to trigger this).

It is a total mess, I'm so glad that they changed this behavior to the sane one in Ruby 3.x.

---

So we either have to revert this change or drop Ruby 2.7 support (it has reached EOL anyway)

[Envek]

Fix has been released in 0.13.1