allow to configure Reservoir implementations #20
Conversation
introduction of a setting in the MetricsConfiguration class to be able to configure the Reservoir implementation to use for produced Timer & Histogram instances. see astefanutti#12
|
@astefanutti do you know why the coverage decreased even if a test covering the new behavior is included in the PR? |
|
@McFoggy Thanks for the PR. Regarding the coverage, it looks like your test returns |
|
You're right! I'll add more tests to cover the other branch. |
|
If you want me to squash, do not hesitate to ask. |
| @@ -63,7 +81,24 @@ private static Meter meter(InjectionPoint ip, MetricRegistry registry, MetricNam | |||
| } | |||
|
|
|||
| @Produces | |||
| private static Timer timer(InjectionPoint ip, MetricRegistry registry, MetricName metricName) { | |||
| return registry.timer(metricName.of(ip)); | |||
| private static Timer timer(InjectionPoint ip, MetricRegistry registry, MetricName metricName, MetricsExtension extension) { | |||
There was a problem hiding this comment.
The same logic is missing from the MetricsInterceptor as we want to apply the custom reservoir to metrics created by interceptors as well for timers.
| * @return this Metrics CDI configuration | ||
| * @throws IllegalStateException if called outside of the observer method invocation | ||
| */ | ||
| MetricsConfiguration useReservoirBuilder(ReservoirBuidler builder); |
There was a problem hiding this comment.
I would add a default implementation to avoid breaking existing implementation. That requires Java 8 and Metrics CDI still builds on Java 7. That being said, I'd be inclined to drop Java 7 for that and other benefits.
| Set<MetricsParameter> getParameters() { | ||
| return Collections.unmodifiableSet(configuration); | ||
| } | ||
|
|
||
| ReservoirBuidler getReservoirBuilder() { |
There was a problem hiding this comment.
I created an EnumSet so that we could add other configuration parameters. I wonder whether we change it to EnumMap and add the ReservoirBuidler configuration to it. The idea being that it avoids having to much getters.
| * @param type the kind of metric for which a reservoir is required | ||
| * @return the reservoir to use, or null if default reservoir implementation has to be used | ||
| */ | ||
| Reservoir build(String metricName, ReservoirUsage type); |
There was a problem hiding this comment.
Why not passing the metric class instead of creating a new ReservoirUsage object?
There was a problem hiding this comment.
I wonder whether returning Optional<Reservoir> would better capture the defaulting rather than returning null. That requires Java 8 but as previously said, I'm inclined to drop Java 7.
| configuration.useReservoirBuilder(new ReservoirBuidler() { | ||
| @Override | ||
| public Reservoir build(String metricName, ReservoirUsage type) { | ||
| counter.incrementAndGet(); |
There was a problem hiding this comment.
I wonder whether we could have a more functional test assertion rather than just checking that the build has actually being called. Not a big deal, but I'd rather have functional assertions than technical ones whenever possible.
|
@astefanutti I have adapted a bit the PR to integrate your remarks. I have not changed:
|
Thanks a lot. That looks great!
I can do it, no big deal.
It's more a matter of style and I appreciate that this is going to be a lot more work in that case. So that's perfectly ok. |
|
Thanks for the PR. You may want to associate your matthieu at brouilard.fr address to your GitHub account. |
|
Thanks for the info, but in fact it is a mistake in the git config if the clone in which I did the latest changes. It should have been brouillard with 2 L ; too late to change the commit. |
|
@McFoggy I've just fixed the history 👌. I'll do a release ASAP. |
|
Thank you Antoine. |
simple enhancement of MetricsConfiguration class to be able to register a ReservoirBuilder class allowing for specific Reservoir instances to be used in produced Timer & Histogram