Allow Yabeda::Counter#increment to be called without argument #26
Closed
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.
Line 6 in ab2be22
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
Metadata
Assignees
Labels
No labels
Activity
Envek commentedon May 25, 2022
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 commentedon Feb 16, 2023
+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 commentedon Sep 12, 2024
What if this was configurable and defaulted to not allow it?
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 commentedon Oct 2, 2024
Sorry for the long wait. I finally got persuaded.
Released in 0.13.0, thanks!
magec commentedon Oct 3, 2024
This has broken rails integration I believe:
This is a debug session on the rails integration:
Will rollback to 0.12
Envek commentedon Oct 3, 2024
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 commentedon Oct 7, 2024
Well, the problem here is ruby 2.x.
With ruby 2
The code:
The usage:
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:
🤷♂️
Allow to increment counters and measure histograms without tags
Envek commentedon Oct 10, 2024
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:
Invoking it with tags (with keys as symbols) but without keyword arguments will always promote positional hash to keyword argument. 🤯
It is because of:
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 commentedon Oct 11, 2024
Fix has been released in 0.13.1