yabeda-rb / yabeda

Extendable framework for collecting and exporting metrics from your Ruby application
MIT License
767 stars 25 forks source link

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

Open Envek opened 3 years ago

Envek commented 3 years ago

Given the current implementation of group method for DSL: https://github.com/yabeda-rb/yabeda/blob/bd3b49fe0fd293fe6ac2d29f9e04fa5bdde79b0c/lib/yabeda/dsl/class_methods.rb#L31-L37

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?

Originally posted by @liaden in https://github.com/yabeda-rb/yabeda/issues/19#issuecomment-821183885

Envek commented 3 years ago

We can configure sibling groups:

Yabeda.configure do
  group :g1
  # metrics, default tags, etc
  group :g2
  # metrics, default tags, etc
end

I think that this is absolutely fine. Hard to maintain in gem code, but while group contents are reasonably small (fits into single screen) it is both understandable and clean.

Envek commented 3 years ago
Yabeda.configure do
  group :g1 do
    # config
    group :g2 do
      # config
    end
    # more g1 config 
  end

That one implies that nesting groups are supported (which is not true). Here, IMO, we should raise an exception. Or add support for nested groups (however no one has ever asked for this AFAIR)