From 06a7ab0cd5f3036e7ece4edf7c80428874afc362 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 20 Dec 2016 13:52:53 -0800 Subject: [PATCH] Polish ReservoirFactory support Polish Dropwizrd reservoir support including a refactor of `ReservoirFactory` to allow reservoirs to be created based on a metric name. See gh-5199 See gh-7105 --- .../MetricsDropwizardAutoConfiguration.java | 10 +- .../dropwizard/DropwizardMetricServices.java | 133 ++++++++++++------ .../metrics/dropwizard/ReservoirFactory.java | 36 +++-- ...tricsDropwizardAutoConfigurationTests.java | 34 ++--- .../DropwizardMetricServicesTests.java | 43 +++--- 5 files changed, 162 insertions(+), 94 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsDropwizardAutoConfiguration.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsDropwizardAutoConfiguration.java index 577b21a87c..a53bc9db45 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsDropwizardAutoConfiguration.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsDropwizardAutoConfiguration.java @@ -18,7 +18,7 @@ package org.springframework.boot.actuate.autoconfigure; import com.codahale.metrics.MetricRegistry; -import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.boot.actuate.endpoint.MetricReaderPublicMetrics; import org.springframework.boot.actuate.metrics.CounterService; import org.springframework.boot.actuate.metrics.GaugeService; @@ -43,8 +43,12 @@ import org.springframework.context.annotation.Configuration; @AutoConfigureBefore(MetricRepositoryAutoConfiguration.class) public class MetricsDropwizardAutoConfiguration { - @Autowired(required = false) - private ReservoirFactory reservoirFactory; + private final ReservoirFactory reservoirFactory; + + public MetricsDropwizardAutoConfiguration( + ObjectProvider reservoirFactory) { + this.reservoirFactory = reservoirFactory.getIfAvailable(); + } @Bean @ConditionalOnMissingBean diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/dropwizard/DropwizardMetricServices.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/dropwizard/DropwizardMetricServices.java index 846c51ff48..98a70ad8cc 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/dropwizard/DropwizardMetricServices.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/dropwizard/DropwizardMetricServices.java @@ -31,6 +31,8 @@ import com.codahale.metrics.Timer; import org.springframework.boot.actuate.metrics.CounterService; import org.springframework.boot.actuate.metrics.GaugeService; +import org.springframework.core.ResolvableType; +import org.springframework.util.Assert; /** * A {@link GaugeService} and {@link CounterService} that sends data to a Dropwizard @@ -55,7 +57,7 @@ public class DropwizardMetricServices implements CounterService, GaugeService { private final MetricRegistry registry; - private ReservoirFactory reservoirFactory; + private final ReservoirFactory reservoirFactory; private final ConcurrentMap gauges = new ConcurrentHashMap(); @@ -66,19 +68,20 @@ public class DropwizardMetricServices implements CounterService, GaugeService { * @param registry the underlying metric registry */ public DropwizardMetricServices(MetricRegistry registry) { - this.registry = registry; + this(registry, null); } /** * Create a new {@link DropwizardMetricServices} instance. * @param registry the underlying metric registry * @param reservoirFactory the factory that instantiates the {@link Reservoir} that - * will be used on Timers and Histograms + * will be used on Timers and Histograms */ public DropwizardMetricServices(MetricRegistry registry, ReservoirFactory reservoirFactory) { this.registry = registry; - this.reservoirFactory = reservoirFactory; + this.reservoirFactory = (reservoirFactory == null ? ReservoirFactory.NONE + : reservoirFactory); } @Override @@ -106,14 +109,10 @@ public class DropwizardMetricServices implements CounterService, GaugeService { @Override public void submit(String name, double value) { if (name.startsWith("histogram")) { - long longValue = (long) value; - Histogram metric = registerHistogram(name); - metric.update(longValue); + submitHistogram(name, value); } else if (name.startsWith("timer")) { - long longValue = (long) value; - Timer metric = registerTimer(name); - metric.update(longValue, TimeUnit.MILLISECONDS); + submitTimer(name, value); } else { name = wrapGaugeName(name); @@ -121,40 +120,36 @@ public class DropwizardMetricServices implements CounterService, GaugeService { } } - private Histogram registerHistogram(String name) { - if (this.reservoirFactory == null) { - return this.registry.histogram(name); - } - else { - Histogram histogram = new Histogram(this.reservoirFactory.getObject()); - return getOrAddMetric(name, histogram); - } + private void submitTimer(String name, double value) { + long longValue = (long) value; + Timer metric = register(name, new TimerMetricRegistrar()); + metric.update(longValue, TimeUnit.MILLISECONDS); } - private Timer registerTimer(String name) { - if (this.reservoirFactory == null) { - return this.registry.timer(name); - } - else { - Timer timer = new Timer(this.reservoirFactory.getObject()); - return getOrAddMetric(name, timer); - } + private void submitHistogram(String name, double value) { + long longValue = (long) value; + Histogram metric = register(name, new HistogramMetricRegistrar()); + metric.update(longValue); } @SuppressWarnings("unchecked") - private T getOrAddMetric(String name, T newMetric) { - Metric metric = this.registry.getMetrics().get(name); - if (metric == null) { - return this.registry.register(name, newMetric); + private T register(String name, MetricRegistrar registrar) { + Reservoir reservoir = this.reservoirFactory.getReservoir(name); + if (reservoir == null) { + return registrar.register(this.registry, name); } - else { - if (metric.getClass().equals(newMetric.getClass())) { - return (T) metric; - } - else { - throw new IllegalArgumentException( - name + " is already used for a different type of metric"); - } + Metric metric = this.registry.getMetrics().get(name); + if (metric != null) { + registrar.checkExisting(metric); + return (T) metric; + } + try { + return this.registry.register(name, registrar.createForReservoir(reservoir)); + } + catch (IllegalArgumentException ex) { + Metric added = this.registry.getMetrics().get(name); + registrar.checkExisting(metric); + return (T) added; } } @@ -201,10 +196,6 @@ public class DropwizardMetricServices implements CounterService, GaugeService { this.registry.remove(name); } - void setReservoirFactory(ReservoirFactory reservoirFactory) { - this.reservoirFactory = reservoirFactory; - } - /** * Simple {@link Gauge} implementation to {@literal double} value. */ @@ -227,4 +218,62 @@ public class DropwizardMetricServices implements CounterService, GaugeService { } + /** + * Strategy used to register metrics. + */ + private static abstract class MetricRegistrar { + + private final Class type; + + @SuppressWarnings("unchecked") + MetricRegistrar() { + this.type = (Class) ResolvableType + .forClass(MetricRegistrar.class, getClass()).resolveGeneric(); + } + + public void checkExisting(Metric metric) { + Assert.isInstanceOf(this.type, metric, + "Different metric type already registered"); + } + + protected abstract T register(MetricRegistry registry, String name); + + protected abstract T createForReservoir(Reservoir reservoir); + + } + + /** + * {@link MetricRegistrar} for {@link Timer} metrics. + */ + private static class TimerMetricRegistrar extends MetricRegistrar { + + @Override + protected Timer register(MetricRegistry registry, String name) { + return registry.timer(name); + } + + @Override + protected Timer createForReservoir(Reservoir reservoir) { + return new Timer(reservoir); + } + + } + + /** + * {@link MetricRegistrar} for {@link Histogram} metrics. + */ + private static class HistogramMetricRegistrar extends MetricRegistrar { + + @Override + protected Histogram register(MetricRegistry registry, String name) { + return registry.histogram(name); + } + + @Override + protected Histogram createForReservoir(Reservoir reservoir) { + return new Histogram(reservoir); + } + + } + } diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/dropwizard/ReservoirFactory.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/dropwizard/ReservoirFactory.java index b1de0bb628..64396043c5 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/dropwizard/ReservoirFactory.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/dropwizard/ReservoirFactory.java @@ -18,22 +18,34 @@ package org.springframework.boot.actuate.metrics.dropwizard; import com.codahale.metrics.Reservoir; -import org.springframework.beans.BeansException; -import org.springframework.beans.factory.ObjectFactory; - /** - * A {@link Reservoir} factory to instantiate the Reservoir that will be set as default - * for the {@link DropwizardMetricServices}. - * The Reservoir instances can't be shared across {@link com.codahale.metrics.Metric}. + * Factory interface that can be used by {@link DropwizardMetricServices} to create a + * custom {@link Reservoir}. * * @author Lucas Saldanha + * @author Phillip Webb + * @since 1.5.0 */ -public abstract class ReservoirFactory implements ObjectFactory { +public interface ReservoirFactory { - protected abstract Reservoir defaultReservoir(); + /** + * Default empty {@link ReservoirFactory} implementation. + */ + ReservoirFactory NONE = new ReservoirFactory() { + + @Override + public Reservoir getReservoir(String name) { + return null; + } + + }; + + /** + * Return the {@link Reservoir} instance to use or {@code null} if a custom reservoir + * is not needed. + * @param name the name of the metric + * @return a reservoir instance or {@code null} + */ + Reservoir getReservoir(String name); - @Override - public Reservoir getObject() throws BeansException { - return defaultReservoir(); - } } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/MetricsDropwizardAutoConfigurationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/MetricsDropwizardAutoConfigurationTests.java index 5f032deea6..f878306b97 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/MetricsDropwizardAutoConfigurationTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/MetricsDropwizardAutoConfigurationTests.java @@ -18,7 +18,6 @@ package org.springframework.boot.actuate.autoconfigure; import com.codahale.metrics.Reservoir; import com.codahale.metrics.UniformReservoir; - import org.junit.After; import org.junit.Test; @@ -27,7 +26,6 @@ import org.springframework.boot.actuate.metrics.dropwizard.ReservoirFactory; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; - import org.springframework.test.util.ReflectionTestUtils; import static org.assertj.core.api.Assertions.assertThat; @@ -52,24 +50,23 @@ public class MetricsDropwizardAutoConfigurationTests { public void dropwizardWithoutCustomReservoirConfigured() { this.context = new AnnotationConfigApplicationContext( MetricsDropwizardAutoConfiguration.class); - DropwizardMetricServices dropwizardMetricServices = this.context .getBean(DropwizardMetricServices.class); - - assertThat(ReflectionTestUtils.getField(dropwizardMetricServices, "reservoirFactory")) - .isNull(); + ReservoirFactory reservoirFactory = (ReservoirFactory) ReflectionTestUtils + .getField(dropwizardMetricServices, "reservoirFactory"); + assertThat(reservoirFactory.getReservoir("test")).isNull(); } @Test public void dropwizardWithCustomReservoirConfigured() { this.context = new AnnotationConfigApplicationContext( MetricsDropwizardAutoConfiguration.class, Config.class); - DropwizardMetricServices dropwizardMetricServices = this.context .getBean(DropwizardMetricServices.class); - - assertThat(ReflectionTestUtils.getField(dropwizardMetricServices, "reservoirFactory")) - .isNotNull(); + ReservoirFactory reservoirFactory = (ReservoirFactory) ReflectionTestUtils + .getField(dropwizardMetricServices, "reservoirFactory"); + assertThat(reservoirFactory.getReservoir("test")) + .isInstanceOf(UniformReservoir.class); } @Configuration @@ -77,13 +74,18 @@ public class MetricsDropwizardAutoConfigurationTests { @Bean public ReservoirFactory reservoirFactory() { - return new ReservoirFactory() { - @Override - protected Reservoir defaultReservoir() { - return new UniformReservoir(); - } - }; + return new UniformReservoirFactory(); } + + } + + private static class UniformReservoirFactory implements ReservoirFactory { + + @Override + public Reservoir getReservoir(String name) { + return new UniformReservoir(); + } + } } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/dropwizard/DropwizardMetricServicesTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/dropwizard/DropwizardMetricServicesTests.java index 04f4c1d65a..535797bc5c 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/dropwizard/DropwizardMetricServicesTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/dropwizard/DropwizardMetricServicesTests.java @@ -22,14 +22,18 @@ import java.util.List; import com.codahale.metrics.Gauge; import com.codahale.metrics.Histogram; import com.codahale.metrics.MetricRegistry; -import com.codahale.metrics.Reservoir; import com.codahale.metrics.Timer; import com.codahale.metrics.UniformReservoir; +import org.junit.Before; import org.junit.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; import org.springframework.test.util.ReflectionTestUtils; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Matchers.anyString; /** * Tests for {@link DropwizardMetricServices}. @@ -39,10 +43,18 @@ import static org.assertj.core.api.Assertions.assertThat; */ public class DropwizardMetricServicesTests { - private final MetricRegistry registry = new MetricRegistry(); + private MetricRegistry registry = new MetricRegistry(); - private final DropwizardMetricServices writer = new DropwizardMetricServices( - this.registry); + @Mock + private ReservoirFactory reservoirFactory; + + private DropwizardMetricServices writer; + + @Before + public void setup() { + MockitoAnnotations.initMocks(this); + this.writer = new DropwizardMetricServices(this.registry, this.reservoirFactory); + } @Test public void incrementCounter() { @@ -87,20 +99,14 @@ public class DropwizardMetricServicesTests { @Test public void setCustomReservoirTimer() { - this.writer.setReservoirFactory(new ReservoirFactory() { - @Override - protected Reservoir defaultReservoir() { - return new UniformReservoir(); - } - }); - + given(this.reservoirFactory.getReservoir(anyString())) + .willReturn(new UniformReservoir()); this.writer.submit("timer.foo", 200); this.writer.submit("timer.foo", 300); assertThat(this.registry.timer("timer.foo").getCount()).isEqualTo(2); - Timer timer = (Timer) this.registry.getMetrics().get("timer.foo"); - Histogram histogram = (Histogram) ReflectionTestUtils - .getField(timer, "histogram"); + Histogram histogram = (Histogram) ReflectionTestUtils.getField(timer, + "histogram"); assertThat(ReflectionTestUtils.getField(histogram, "reservoir").getClass() .equals(UniformReservoir.class)).isTrue(); } @@ -114,13 +120,8 @@ public class DropwizardMetricServicesTests { @Test public void setCustomReservoirHistogram() { - this.writer.setReservoirFactory(new ReservoirFactory() { - @Override - protected Reservoir defaultReservoir() { - return new UniformReservoir(); - } - }); - + given(this.reservoirFactory.getReservoir(anyString())) + .willReturn(new UniformReservoir()); this.writer.submit("histogram.foo", 2.1); this.writer.submit("histogram.foo", 2.3); assertThat(this.registry.histogram("histogram.foo").getCount()).isEqualTo(2);