vladmihalcea / flexy-pool

FlexyPool adds metrics and failover strategies to a given Connection Pool, allowing it to resize on demand.
Apache License 2.0
1.09k stars 120 forks source link

Fix the Include uniqueName in the metric names #60 issue #62

Closed atokle closed 4 years ago

atokle commented 4 years ago

All metrics wil now start with flexypool_ making it easy to see which metrics is made from flexy-pool All metrics will have a parameter poolname= with the value from the pools uniqueName making it possible to see what pool the metric belongs to. Example. The metrics:

concurrentConnectionRequestsHistogram_count 12.0
concurrentConnectionRequestsHistogram_sum 6.0

will with 2 pools with uniqueNames quartzPool and batchPool become

flexypool_concurrentConnectionRequestsHistogram_count{poolname="quartzPool",} 6.0
flexypool_concurrentConnectionRequestsHistogram_sum{poolname="quartzPool",} 3.0
flexypool_concurrentConnectionRequestsHistogram_count{poolname="batchPool",} 6.0
flexypool_concurrentConnectionRequestsHistogram_sum{poolname="batchPool",} 3.0
vladmihalcea commented 4 years ago

This is a backwards incompatible change as all systems that parse the old format will break. So, I think this change needs to be done based on a strategy that provided via the Configuration. The default strategy should be the existing one, while this new one would be configured explicitly. More, it needs to be applied yo Dropwizard Metrics, not just Micrometer.

atokle commented 4 years ago

I can make it configurable with old behavour as default. But I'm not sure about Dropwizard Metric. Doesn't Dropwizard already use uniqueName in metricName? And the prefix does not make sense the same way in jmx, where the metric is in a tree-structure, and is grouped together anyway. About how to make it configureable, I'm not sure how it should be done with a strategy. But it could easily be done with an extra field in the Configuration class.

vladmihalcea commented 4 years ago

The name generator can be designed as an interface. The current logic becomes the default implementation. Your change is a different implementation of the same interface. The Configuration can take the class name of this metric name strategy.

atokle commented 4 years ago

I have made the changes configureable using a strategy, and with default to old behavour. But still only for MicroMetric. DropWizard do already use uniqueName, and if I apply the strategy there too, it will again breake backward compatibility

vladmihalcea commented 4 years ago

Thanks. I'll review it when I have some time.

vladmihalcea commented 4 years ago

Applied upstream and released in 2.2.2 version.