search

yabeda-rb / yabeda-rails

Yabeda plugin to collect basic metrics for Rails applications
MIT License
155 stars 20 forks source link

Do not merge controller back in #19

Closed liaden closed 3 years ago

liaden commented 3 years ago

If a user defines a default_tag :controller, nil, then it cause the metrics to use event.payload[:controller] which is "ApplicationController" instead of "application".

liaden commented 3 years ago

Optionally, I'd prefer there be a Yabeda::Rails.extra_tags = [:my, :custom, :tags], and it adds those tags to the metrics that are defined instead of the tags instead of requiring a globally polluting default_tag that affects other gems that may be included as well.

Envek commented 3 years ago

Hey! Thank you for your PR!

Can you please tell more about your use case? Default tags are meant to be really application-global (both collected by gems and user-defined in the application code)

And just a note that current behavior was added in https://github.com/yabeda-rb/yabeda-rails/pull/13 by @raivil (maybe it will shed some light)

liaden commented 3 years ago

Can you please tell more about your use case? Default tags are meant to be really application-global (both collected by gems and user-defined in the application code)

From the readme, we have to both do a default_tag call as well as the append_info_to_payload to pass it through. My thought was the extra_tags could be splatted onto all the metrics that are defined by yabeda-rails to avoid the label being applied to everything. I don't think this matters since https://github.com/yabeda-rb/yabeda/issues/16 would allow for the app to define the labels without having to add complexity to this gem.

Envek commented 3 years ago

default_tag is needed only because some adapters (well, only prometheus at the moment) requires us to declare beforehand what tags every metric can have. So it is kind of hack.

Envek commented 3 years ago

Released in 0.7.2

Thank you!