Ability to change metrics setting for individual adapter #37
Conversation
There was a problem hiding this comment.
Hey, thank you very much for your pull request!
Can you please tell more about your use case? What are you doing and why some metrics need to use only one adapter of many (and what are these?)
I believe we can figure out some nice API based on your insights.
Some thoughts:
- Maybe option should be not only on metric but also on group level? So e.g. all rails/sidekiq/(your app) metrics can be limited to a single adapter?
- Probably metric object should be responsible for calculation of the list of adapters it should be reported to.
Let's discuss!
.gitignore
Outdated
| @@ -1,5 +1,6 @@ | |||
| /.bundle/ | |||
| /.yardoc | |||
| /.idea/ | |||
There was a problem hiding this comment.
Please don't add non-project related files to project gitignore. Set up and use global gitignore file on your machine instead. See https://docs.github.com/en/get-started/getting-started-with-git/ignoring-files#configuring-ignored-files-for-all-repositories-on-your-computer
| /.idea/ |
lib/yabeda/metric.rb
Outdated
| @@ -14,6 +14,7 @@ class Metric | |||
| option :per, optional: true, comment: "Per which unit is measured `unit`. E.g. `call` as in seconds per call" | |||
| option :group, optional: true, comment: "Category name for grouping metrics" | |||
| option :aggregation, optional: true, comment: "How adapters should aggregate values from different processes" | |||
| option :adapter, optional: true, comment: "Which adapter apply the indicator settings." | |||
There was a problem hiding this comment.
Let's reword comment a bit for clarity:
| option :adapter, optional: true, comment: "Which adapter apply the indicator settings." | |
| option :adapter, optional: true, comment: "Monitoring system adapter to register metric in and report metric values to (other adapters won't be used)" |
lib/yabeda/histogram.rb
Outdated
| @@ -20,7 +20,7 @@ def measure(tags, value = nil) | |||
|
|
|||
| all_tags = ::Yabeda::Tags.build(tags, group) | |||
| values[all_tags] = value | |||
| ::Yabeda.adapters.each do |_, adapter| | |||
| ::Yabeda.adapters.each_value do |adapter| | |||
There was a problem hiding this comment.
Shouldn't adapters also be filtered here? (and in other metric types)
There was a problem hiding this comment.
Yes, you are right! Adapters should be filtered
There was a problem hiding this comment.
Maybe metric itself should have adapters method (that will default to all registered adapters unless limiting option specified)?
spec/yabeda_spec.rb
Outdated
There was a problem hiding this comment.
I believe we also need to test a case when adapter is registered, but metrics doesn't use it (so we need to have at least two adapters declared in spec file)
lib/yabeda/dsl/class_methods.rb
Outdated
| @@ -115,6 +116,15 @@ def register_group_for(metric) | |||
|
|
|||
| group.register_metric(metric) | |||
| end | |||
|
|
|||
| def register_metric_for_adapters(metric) | |||
There was a problem hiding this comment.
I feel that this some of this logic should be in Metric class.
|
Hello! Thanks for your review!
|
|
@Envek, I've fixed all! |
* master: Switch to RubyGems Trusted publishing in CI release workflow [ci skip] Update CI for rubocop and fix lint issues fix: railtie loading to prevent calling methods that have not yet been defined (yabeda-rb#38)
|
Hey @Keallar, thanks for improving the pull request! I edited your pull request further a bit: moved more logic into |
|
Hi @Envek, yes, it looks better this way. I liked your solution to define the |
|
But i need to add and fix some test cases |
|
I've done |
|
Thanks for cooperation! |
|
Released in 0.13.0, thanks! |
Ability to change metric settings for individual adapters