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

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> reservoirFactory) {
+		this.reservoirFactory = reservoirFactory.getIfAvailable();
+	}
 
 	@Bean
 	@ConditionalOnMissingBean

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<String, SimpleGauge> gauges = new ConcurrentHashMap<String, SimpleGauge>();
 
@@ -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 extends Metric> T getOrAddMetric(String name, T newMetric) {
+	private <T extends Metric> T register(String name, MetricRegistrar<T> registrar) {
+		Reservoir reservoir = this.reservoirFactory.getReservoir(name);
+		if (reservoir == null) {
+			return registrar.register(this.registry, name);
+		}
 		Metric metric = this.registry.getMetrics().get(name);
-		if (metric == null) {
-			return this.registry.register(name, newMetric);
+		if (metric != null) {
+			registrar.checkExisting(metric);
+			return (T) metric;
 		}
-		else {
-			if (metric.getClass().equals(newMetric.getClass())) {
-				return (T) metric;
-			}
-			else {
-				throw new IllegalArgumentException(
-						name + " is already used for a different type of 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<T extends Metric> {
+
+		private final Class<T> type;
+
+		@SuppressWarnings("unchecked")
+		MetricRegistrar() {
+			this.type = (Class<T>) 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<Timer> {
+
+		@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<Histogram> {
+
+		@Override
+		protected Histogram register(MetricRegistry registry, String name) {
+			return registry.histogram(name);
+		}
+
+		@Override
+		protected Histogram createForReservoir(Reservoir reservoir) {
+			return new Histogram(reservoir);
+		}
+
+	}
+
 }

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<Reservoir> {
+public interface ReservoirFactory {
+
+	/**
+	 * Default empty {@link ReservoirFactory} implementation.
+	 */
+	ReservoirFactory NONE = new ReservoirFactory() {
+
+		@Override
+		public Reservoir getReservoir(String name) {
+			return null;
+		}
+
+	};
 
-	protected abstract Reservoir defaultReservoir();
+	/**
+	 * 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();
-	}
 }

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();
+		}
+
 	}
 
 }

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);