Add default tags only for a specific group #19

liaden · 2021-03-15T13:39:41Z

This would fix #16

liaden · 2021-03-15T13:43:17Z

lib/yabeda/group.rb

@@ -9,6 +9,10 @@ class Group

    param :name

+    def default_tags


Probably should be a different name since this doesn't include the default_tags of the global scope?

liaden · 2021-03-15T13:47:07Z

lib/yabeda/dsl/class_methods.rb

+        if group
+          Yabeda::Tags.tag_group(group)[name] = value
+        else
+          Yabeda.default_tags[name] = value


I considered updating Yabeda.default_tags itself to be a hash of hashes and let nil be the key that accessed top level tags.

Envek · 2021-03-15T17:51:41Z

Hi! Thank you very much for the pull request!

While I'm generally like the idea and want this feature to be in Yabeda, I will probably will leave some notes about API and implementation, but not today. I will take more thorough look in a few days.

liaden · 2021-03-15T19:49:34Z

No rush on this one. I do like the idea of this functionality, but API is important.

One thought I had was if groups can be nested. If so, the storage of the default tags for a given group seems more appropriate on the group. It seems like in that case then default_tags at the global scope is just a group with no name specified.

Additionally:

Yabeda.configure do
  group :something do
    # should probably have `group :something` implied here
    default_tag :key, value
  end
end

Some of these things made the idea a lot more complex, so I just opted to KISS with this PR. I don't mind expanding the scope and doing more changes if that is better though.

Envek · 2021-03-15T20:29:12Z

I like nested default_tag calls, inside groups. If feels more natural. And it should be easy to implement as in DSL current group is available as @group:

yabeda/lib/yabeda/dsl/class_methods.rb

Lines 31 to 37 in bd3b49f

    
           def group(group_name) 
        
             @group = group_name 
        
             return unless block_given? 
        
             yield 
        
             @group = nil 
        
           end

However, not nested calls looks useful too. For example if someone would like to add some tags to all sidekiq-related metrics from yabeda-sidekiq in their application.

Envek · 2021-03-15T20:29:59Z

lib/yabeda/dsl/class_methods.rb

@@ -58,8 +58,12 @@ def histogram(*args, **kwargs, &block)
      #
      # @param name [Symbol] Name of default tag
      # @param value [String] Value of default tag
-      def default_tag(name, value)
-        ::Yabeda.default_tags[name] = value
+      def default_tag(name, value, group: nil)


You can use this and it will work 😃

Suggested change

def default_tag(name, value, group: nil)

def default_tag(name, value, group: @group)

I've pushed up a new commit (to maybe make reviewing easier). I'll squash prior to merge if it all looks good.

Some thoughts as I was working onit:

I created a group object with a name of nil to account for the global group.

with_tags can nest with_tags

Named group's continue to be a child of the global group. Current code doesn't support deeper nesting, and Yabeda.group1.group2.some_gauge.set({}, 5) feels verbose/complicated (and probably not needed).

I got into some weird states on testing when trying to remove methods so I tried to make Yabeda.reset! a bit more resilient and comprehensive (so a bad test doesn't break every subsequent test).

I got the feeling that a group instance could be the evaluation context for the DSL. A group having default_tags and having metrics already feels very similar to the Yabeda module. If this was done, then arbitrary nesting of groups becomes easier.

The two register calls for groups and metrics was a bit confusing since they both call each other.

lib/yabeda/dsl/class_methods.rb

liaden · 2021-03-16T22:53:31Z

lib/yabeda/histogram.rb

@@ -7,7 +7,7 @@ class Histogram < Metric
    option :buckets

    def measure(tags, value)
-      all_tags = ::Yabeda::Tags.build(tags)
+      all_tags = ::Yabeda::Tags.build(tags, group)


Would it be worth extracting this to Metric since all three metrics have this? I don't have a good name for it since we already have tags for just the keys.

liaden · 2021-03-16T22:55:06Z

lib/yabeda/metric.rb

@@ -17,15 +17,15 @@ class Metric

    # Returns the value for the given label set
    def get(labels = {})
-      values[::Yabeda::Tags.build(labels)]
+      values[::Yabeda::Tags.build(labels, group)]


I ran into some confusion at first that I thought group would be an instance of Group as opposed to a symbol.

liaden · 2021-04-07T17:59:30Z

@Envek Any feedback or other thoughts?

Envek · 2021-04-07T18:12:31Z

Sorry, couldn't get to it. I definitely want this feature to be in Yabeda. Hopefully will be able to review it later this week.

Envek · 2021-04-16T09:38:55Z

lib/yabeda/group.rb

+      if name
+        Yabeda.groups[nil].default_tags.merge!(@default_tags)
+      else
+        @default_tags.dup
+      end


While concept of implicit global group looks reasonable, such places to distinguish global tags from group-local ones are looking very weird.

I am not sure what you mean by 'such places?'

liaden · 2021-04-16T13:37:29Z

Looking back at this again, I think the handling of groups leaves room for user error/confusion.

Given the curerent implementation of

      def group(group_name)
        @group = group_name
        return unless block_given?

        yield
        @group = nil
      end

We can configure sibling groups

Yabeda.configure do
  group :g1
  # config
  group :g2
  # config
  end

or attempt to have nested groups

Yabeda.configure do
  group :g1 do
    # config
    group :g2 do
      # config
    end
    # more g1 config 
  end

The nested groups are really the same as the first situation where we create two siblings. Both default_tags and looking up metrics would be handled the same in both cases with an additional surprise that the 'more g1 config'j would apply to the global scope.

I think that it would be good to raise an exception or at least warn when someone attempts to nest two groups to prevent the issue until there is a need for deeper nesting?

Envek · 2021-04-16T14:57:07Z

Yeah, groups are somewhat messy, but it is apparently outside of scope for this pull request. Let's discuss it in #20

Envek · 2021-04-16T15:22:01Z

I pushed one more commit right into this pull request (yep, “allow edits from maintainers” checkbox allows to do such things) which effectively removes global group usage (but it is still there). Do you like these edits? Let's discuss!

liaden · 2021-04-29T12:06:44Z

Yeah, this looks good to me.

Envek · 2021-05-07T13:31:43Z

Released in 0.9.0. Thank you!

Add default tags only for a specific group

2e8dd93

liaden commented Mar 15, 2021

View reviewed changes

Envek reviewed Mar 15, 2021

View reviewed changes

Place ownership of default_tags under group

75a1336

liaden commented Mar 16, 2021

View reviewed changes

lib/yabeda/dsl/class_methods.rb Show resolved Hide resolved

liaden commented Mar 16, 2021

View reviewed changes

Envek reviewed Apr 16, 2021

View reviewed changes

Reintroduce global default_tags back

346dc38

Envek mentioned this pull request Apr 16, 2021

Current handling of groups leaves room for user error/confusion #20

Open

Envek added 2 commits May 7, 2021 16:21

Fix missing receiver in method (and add test case to catch it)

9decb1e

Prepare 0.9.0 release

4fe1e45

Envek merged commit d15c7c5 into yabeda-rb:master May 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default tags only for a specific group #19

Add default tags only for a specific group #19

liaden commented Mar 15, 2021

liaden Mar 15, 2021

liaden Mar 15, 2021

Envek commented Mar 15, 2021

liaden commented Mar 15, 2021

Envek commented Mar 15, 2021

Envek Mar 15, 2021

liaden Mar 16, 2021

liaden Mar 16, 2021

liaden Mar 16, 2021

liaden commented Apr 7, 2021

Envek commented Apr 7, 2021

Envek Apr 16, 2021

liaden Apr 16, 2021

liaden commented Apr 16, 2021

Envek commented Apr 16, 2021

Envek commented Apr 16, 2021

liaden commented Apr 29, 2021

Envek commented May 7, 2021

	def default_tag(name, value, group: nil)
	def default_tag(name, value, group: @group)

Add default tags only for a specific group #19

Add default tags only for a specific group #19

Conversation

liaden commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Envek commented Mar 15, 2021

liaden commented Mar 15, 2021

Envek commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liaden commented Apr 7, 2021

Envek commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liaden commented Apr 16, 2021

Envek commented Apr 16, 2021

Envek commented Apr 16, 2021

liaden commented Apr 29, 2021

Envek commented May 7, 2021