Deprecate setMetricRegistry in favour of setMetricsTrackerFactory #1091
Conversation
… provide static factory methods in the interface.
Codecov Report
@@ Coverage Diff @@
## dev #1091 +/- ##
============================================
- Coverage 72.98% 72.23% -0.76%
+ Complexity 557 551 -6
============================================
Files 24 24
Lines 1940 1945 +5
Branches 258 261 +3
============================================
- Hits 1416 1405 -11
- Misses 369 372 +3
- Partials 155 168 +13
Continue to review full report at Codecov.
|
| @@ -26,4 +31,13 @@ | |||
| * @return a IMetricsTracker implementation instance | |||
| */ | |||
| IMetricsTracker create(String poolName, PoolStats poolStats); | |||
|
|
|||
| static MetricsTrackerFactory from(MetricRegistry metricRegistry) { | |||
There was a problem hiding this comment.
I don't think MetricsTrackerFactory should contain references to implementation specific code.
There was a problem hiding this comment.
I didn't want to create MetricsTrackerFactoryFactory :)
This seems not right at first but in JDK itself starting from 9 there are lots of examples like this. For example List.of(...)
Do you want me to revert this file so that users have to do setMetricsTrackerFactory(new CodahaleMetricsTrackerFactory(metricRegistry)) instead?
There was a problem hiding this comment.
I think that makes more sense, that way there is no risk of CNFE and it makes it easier to deprecate the old metrics tracker.
There was a problem hiding this comment.
There is no risk of CNFE here if that's your concern. This method cannot be invoked without creating MetricRegistry first. Which means the class will already be loaded by JVM before reaching this point.
When deprecating corresponding metricsTracker implementation this factory method can be deprecated at the same time.
There was a problem hiding this comment.
More concerned about it being too tightly coupled.
brettwooldridge
force-pushed
the
dev
branch
2 times, most recently
from
6d319ab
to
7cacd0f
Compare
Following up on my yesterday's PR #1089.
Even though it is ok, I think the fact of accepting an Object and then doing tricks to find out wether the library class is in classpath and later on casting the object to correct type are not very nice.
This is especially not great in light of Change in dropwizard metrics package name in upcoming v5. For some time we will need to support both versions. This will add even more complexity in that part of the code.
That's why I propose to deprecate/delete setMetricRegistry in favour of setMetricsTrackerFactory. Also by providing overloaded static factory methods in the MetricsTrackerFactory interface we won't need to do type checks at all. Users can just use
setMetricsTrackerFactory(from(metricRegistry))instead ofsetMetricRegistry(metricRegistry)allowing to greatly simplify code around registering metricRegsitry .This is preliminary PR just to get your feedback. Can update it with complete removal.
WDYT?