Skip to content

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

Closed
@goalaleo

Description

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.

def increment(tags, by: 1)

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

Activity

changed the title Allow Yabeda::Counter#increment to be called without tags Allow Yabeda::Counter#increment to be called without argument on May 25, 2022
Envek

Envek commented on May 25, 2022

@Envek
Member

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

chrp commented on Feb 16, 2023

@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

goalaleo commented on Sep 12, 2024

@goalaleo
Author

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

Envek commented on Oct 2, 2024

@Envek
Member

Sorry for the long wait. I finally got persuaded.

Released in 0.13.0, thanks!

magec

magec commented on Oct 3, 2024

@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

Envek commented on Oct 3, 2024

@Envek
Member

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

magec commented on Oct 7, 2024

@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
   ....

🤷‍♂️

Envek

Envek commented on Oct 10, 2024

@Envek
Member

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. 🤯

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

Envek commented on Oct 11, 2024

@Envek
Member

Fix has been released in 0.13.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Allow Yabeda::Counter#increment to be called without argument · Issue #26 · yabeda-rb/yabeda