Injectable metricsRegistry #263
Conversation
| * | ||
| * @return the metrics | ||
| */ | ||
| Map<String, Metric> getMetrics(); |
There was a problem hiding this comment.
All other getters in this class return a SortedMap. This should too. At least for the sake of consistency.
There was a problem hiding this comment.
The underlying metrics registry returns a sortedMap in all those cases. So it's a pass through. The getMetrics though just returns a Collections.unmodifiableMap(metrics) Assuming you want this just sorted in the impl in this case.
| @@ -53,6 +54,7 @@ public CodaHaleMetricRegistry() { | |||
| this(new com.codahale.metrics.MetricRegistry()); | |||
| } | |||
|
|
|||
| @JsonValue | |||
| public com.codahale.metrics.MetricRegistry getMetricRegistry() { | |||
| return metricRegistry; | |||
| } | |||
There was a problem hiding this comment.
Is this @JsonValue here to serialise the CodaHaleMetricRegistry instance?
Please consider providing a Mixin class instead. See com.hotels.styx.infrastructure.configuration.json.mixins as an example.
There was a problem hiding this comment.
I believe we need to remove the getMetricRegistry going forward. It exposes the implementation detail outside of the metrics interface. Ideally, a Coda Hale metrics registry required for Graphite & Jmx reporters would be bridged to Styx metrics registry.
But that is another story and I don't think necessary for this PR.
There was a problem hiding this comment.
There are 2 ObjectMappers in the MetricsHandler. For the filtered one, it's declared at the top of the MetricsHandler. If not filtered, it defaults the the internal one in JsonHandler. I added a new module so I could send the mixin to that objectMapper
This allows the user to set the metrics registry instead of it being auto created. Useful for Dependency Injection.
taer
force-pushed
the
metricsRegistryInjection
branch
from
031086c
to
29b5fb2
Compare
taer
force-pushed
the
metricsRegistryInjection
branch
from
2110510
to
d1950d1
Compare
This allows the user to set the metrics registry instead of it being auto created.
Useful for Dependency Injection.