Skip to content

Conversation

liaden
Copy link
Contributor

@liaden liaden commented Mar 12, 2021

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

format: event.payload[:format],
method: event.payload[:method].downcase,
}.merge!(event.payload.slice(*Yabeda.default_tags.keys))
}.merge!(event.payload.slice(*Yabeda.default_tags.keys.excluding(:controller)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: excluding is only rails 6+.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, now it is obvious that we need to exclude all keys that we manually set in. Please try something like this:

Suggested change
}.merge!(event.payload.slice(*Yabeda.default_tags.keys.excluding(:controller)))
}
labels.merge!(event.payload.slice(*Yabeda.default_tags.keys - labels.keys))

@liaden
Copy link
Contributor Author

liaden commented Mar 12, 2021

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
Copy link
Member

Envek commented Mar 15, 2021

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 #13 by @raivil (maybe it will shed some light)

@liaden
Copy link
Contributor Author

liaden commented Mar 15, 2021

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 yabeda-rb/yabeda#16 would allow for the app to define the labels without having to add complexity to this gem.

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"`.
@Envek
Copy link
Member

Envek commented Mar 15, 2021

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 Envek merged commit fa69d6d into yabeda-rb:master Mar 15, 2021
@Envek
Copy link
Member

Envek commented Mar 15, 2021

Released in 0.7.2

Thank you!

@liaden liaden deleted the patch-1 branch March 15, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants