Fix flaky Servo tests by avoiding DefaultMonitorRegistry

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/MetricsClientHttpRequestInterceptor.java

@@ -49,6 +49,9 @@ public class MetricsClientHttpRequestInterceptor implements ClientHttpRequestInt
 	@Autowired
 	Collection<MetricsTagProvider> tagProviders;
 
+	@Autowired
+	ServoMonitorCache servoMonitorCache;
+
 	@Value("${netflix.metrics.restClient.metricName:restclient}")
 	String metricName;
 
@@ -75,7 +78,7 @@ public class MetricsClientHttpRequestInterceptor implements ClientHttpRequestInt
 					.builder(metricName);
 			monitorConfigBuilder.withTags(builder);
 
-			ServoMonitorCache.getTimer(monitorConfigBuilder.build()).record(
+			servoMonitorCache.getTimer(monitorConfigBuilder.build()).record(
 					System.nanoTime() - startTime, TimeUnit.NANOSECONDS);
 		}
 	}

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/MetricsHandlerInterceptor.java

@@ -48,6 +48,9 @@ public class MetricsHandlerInterceptor extends HandlerInterceptorAdapter {
 	MonitorRegistry registry;
 
 	@Autowired
+	ServoMonitorCache servoMonitorCache;
+
+	@Autowired
 	Collection<MetricsTagProvider> tagProviders;
 
 	@Override
@@ -89,7 +92,7 @@ public class MetricsHandlerInterceptor extends HandlerInterceptorAdapter {
 		MonitorConfig.Builder monitorConfigBuilder = MonitorConfig.builder(metricName);
 		monitorConfigBuilder.withTags(builder);
 
-		ServoMonitorCache.getTimer(monitorConfigBuilder.build()).record(
+		servoMonitorCache.getTimer(monitorConfigBuilder.build()).record(
 				System.nanoTime() - startTime, TimeUnit.NANOSECONDS);
 	}
 }

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricsAutoConfiguration.java

@@ -60,13 +60,18 @@ public class ServoMetricsAutoConfiguration {
 
 	@Bean
 	@ConditionalOnMissingBean
-	public MonitorRegistry monitorRegistry() {
-		System.setProperty(DefaultMonitorRegistry.class.getCanonicalName() + ".registryClass", servoMetricsConfig()
+	public MonitorRegistry monitorRegistry(ServoMetricsConfigBean servoMetricsConfig) {
+		System.setProperty(DefaultMonitorRegistry.class.getCanonicalName() + ".registryClass", servoMetricsConfig
 				.getRegistryClass());
 		return DefaultMonitorRegistry.getInstance();
 	}
 
 	@Bean
+	public ServoMonitorCache monitorCache(MonitorRegistry monitorRegistry) {
+		return new ServoMonitorCache(monitorRegistry);
+	}
+
+	@Bean
 	public MetricReaderPublicMetrics servoPublicMetrics(MonitorRegistry monitorRegistry, ServoMetricNaming servoMetricNaming) {
 		ServoMetricReader reader = new ServoMetricReader(monitorRegistry, servoMetricNaming);
 		return new MetricReaderPublicMetrics(reader);

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMonitorCache.java

@@ -16,10 +16,8 @@ package org.springframework.cloud.netflix.metrics.servo;
 import java.util.HashMap;
 import java.util.Map;
 
-import com.netflix.servo.DefaultMonitorRegistry;
 import com.netflix.servo.MonitorRegistry;
 import com.netflix.servo.monitor.BasicTimer;
-import com.netflix.servo.monitor.Monitor;
 import com.netflix.servo.monitor.MonitorConfig;
 
 /**
@@ -28,32 +26,26 @@ import com.netflix.servo.monitor.MonitorConfig;
  * @author Jon Schneider
  */
 public class ServoMonitorCache {
-	private static final Map<MonitorConfig, BasicTimer> timerCache = new HashMap<>();
+	private final Map<MonitorConfig, BasicTimer> timerCache = new HashMap<>();
+	private final MonitorRegistry monitorRegistry;
+
+	public ServoMonitorCache(MonitorRegistry monitorRegistry) {
+		this.monitorRegistry = monitorRegistry;
+	}
 
 	/**
 	 * @param config contains the name and tags that uniquely identify a timer
 	 * @return an already registered timer if it exists, otherwise create/register one and
 	 * return it.
 	 */
-	public synchronized static BasicTimer getTimer(MonitorConfig config) {
+	public synchronized BasicTimer getTimer(MonitorConfig config) {
 		BasicTimer t = timerCache.get(config);
 		if (t != null)
 			return t;
 
 		t = new BasicTimer(config);
 		timerCache.put(config, t);
-		DefaultMonitorRegistry.getInstance().register(t);
+		monitorRegistry.register(t);
 		return t;
 	}
-
-	/**
-	 * Useful for tests to clear the monitor registry between runs
-	 */
-	public static void unregisterAll() {
-		MonitorRegistry registry = DefaultMonitorRegistry.getInstance();
-		for (Monitor<?> monitor : registry.getRegisteredMonitors()) {
-			registry.unregister(monitor);
-		}
-		timerCache.clear();
-	}
 }

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/MetricsClientHttpRequestInterceptorTests.java

@@ -24,6 +24,7 @@ import org.springframework.cloud.netflix.metrics.servo.ServoMetricsAutoConfigura
 import org.springframework.cloud.netflix.metrics.servo.ServoMonitorCache;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Primary;
 import org.springframework.http.HttpMethod;
 import org.springframework.http.MediaType;
 import org.springframework.test.context.ContextConfiguration;
@@ -46,11 +47,14 @@ import com.netflix.servo.monitor.MonitorConfig;
 		MetricsRestTemplateTestConfig.class })
 @TestPropertySource(properties = { "netflix.metrics.restClient.metricName=metricName",
 		"spring.aop.proxy-target-class=true" })
-public class MetricsClientHttpRequestInterceptorTests extends AbstractMetricsTests {
+public class MetricsClientHttpRequestInterceptorTests {
 	@Autowired
 	MonitorRegistry registry;
 
 	@Autowired
+	ServoMonitorCache servoMonitorCache;
+
+	@Autowired
 	RestTemplate restTemplate;
 
 	@Test
@@ -67,7 +71,7 @@ public class MetricsClientHttpRequestInterceptorTests extends AbstractMetricsTes
 				.withTag("status", "200")
 				.withTag("clientName", "none");
 
-		BasicTimer timer = ServoMonitorCache.getTimer(builder.build());
+		BasicTimer timer = servoMonitorCache.getTimer(builder.build());
 
 		Assert.assertEquals(1L, (long) timer.getCount());
 		mockServer.verify();
@@ -78,6 +82,11 @@ public class MetricsClientHttpRequestInterceptorTests extends AbstractMetricsTes
 @ImportAutoConfiguration({ ServoMetricsAutoConfiguration.class,
 		PropertyPlaceholderAutoConfiguration.class, AopAutoConfiguration.class })
 class MetricsRestTemplateTestConfig {
+	@Bean
+	@Primary
+	public MonitorRegistry monitorRegistry() {
+		return new SimpleMonitorRegistry();
+	}
 }
 
 @Configuration

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/MetricsHandlerInterceptorIntegrationTests.java

@@ -26,6 +26,7 @@ import org.springframework.cloud.netflix.metrics.servo.ServoMetricsAutoConfigura
 import org.springframework.cloud.netflix.metrics.servo.ServoMonitorCache;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Primary;
 import org.springframework.http.HttpStatus;
 import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.context.ContextConfiguration;
@@ -60,13 +61,16 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.
 @WebAppConfiguration
 @TestPropertySource(properties = "netflix.metrics.rest.metricName=metricName")
 @DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD)
-public class MetricsHandlerInterceptorIntegrationTests extends AbstractMetricsTests {
+public class MetricsHandlerInterceptorIntegrationTests {
 	@Autowired
 	WebApplicationContext webAppContext;
 
 	@Autowired
 	MonitorRegistry registry;
 
+	@Autowired
+	ServoMonitorCache servoMonitorCache;
+
 	MockMvc mvc;
 
 	@Test
@@ -117,7 +121,7 @@ public class MetricsHandlerInterceptorIntegrationTests extends AbstractMetricsTe
 		if (exceptionType != null)
 			builder = builder.withTag("exception", exceptionType);
 
-		BasicTimer timer = ServoMonitorCache.getTimer(builder.build());
+		BasicTimer timer = servoMonitorCache.getTimer(builder.build());
 		Assert.assertEquals(1L, (long) timer.getCount());
 	}
 }
@@ -131,6 +135,12 @@ class MetricsTestConfig {
 	MetricsTestController testController() {
 		return new MetricsTestController();
 	}
+
+	@Bean
+	@Primary
+	public MonitorRegistry monitorRegistry() {
+		return new SimpleMonitorRegistry();
+	}
 }
 
 @RestController

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/AbstractMetricsTests.java → spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/SimpleMonitorRegistry.java

@@ -13,15 +13,40 @@
 
 package org.springframework.cloud.netflix.metrics;
 
-import org.junit.Before;
-import org.springframework.cloud.netflix.metrics.servo.ServoMonitorCache;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import com.netflix.servo.MonitorRegistry;
+import com.netflix.servo.monitor.Monitor;
 
 /**
  * @author Jon Schneider
  */
-public class AbstractMetricsTests {
-	@Before
-	public void setup() {
-		ServoMonitorCache.unregisterAll();
+public class SimpleMonitorRegistry implements MonitorRegistry {
+	List<Monitor<?>> monitors = new ArrayList<>();
+
+	@Override
+	public Collection<Monitor<?>> getRegisteredMonitors() {
+		return monitors;
+	}
+
+	@Override
+	public void register(Monitor<?> monitor) {
+		monitors.add(monitor);
+	}
+
+	@Override
+	public void unregister(Monitor<?> monitor) {
+		monitors.remove(monitor);
+	}
+
+	@Override
+	public boolean isRegistered(Monitor<?> monitor) {
+		for (Monitor<?> m : monitors) {
+			if (m.equals(monitor))
+				return true;
+		}
+		return false;
 	}
 }
\ No newline at end of file

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricReaderTests.java

@@ -4,37 +4,24 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
-import org.junit.Ignore;
 import org.junit.Test;
 import org.springframework.boot.actuate.metrics.Metric;
-import org.springframework.cloud.netflix.metrics.AbstractMetricsTests;
+import org.springframework.cloud.netflix.metrics.SimpleMonitorRegistry;
 
 import com.google.common.collect.Lists;
-import com.netflix.servo.DefaultMonitorRegistry;
-import com.netflix.servo.MonitorRegistry;
-import com.netflix.servo.monitor.BasicTimer;
-import com.netflix.servo.monitor.Monitor;
 import com.netflix.servo.monitor.MonitorConfig;
 
 import static junit.framework.Assert.assertEquals;
 
-public class ServoMetricReaderTests extends AbstractMetricsTests {
+public class ServoMetricReaderTests {
 	@Test
-	@Ignore
 	public void singleCompositeMonitorYieldsMultipleActuatorMetrics() {
-		MonitorRegistry registry = DefaultMonitorRegistry.getInstance();
-
-		// deal with monitors registered in other tests
-		for (Monitor<?> monitor : registry.getRegisteredMonitors()) {
-			registry.unregister(monitor);
-		}
-
-		ServoMetricReader reader = new ServoMetricReader(registry,
-				new DimensionalServoMetricNaming());
+		SimpleMonitorRegistry registry = new SimpleMonitorRegistry();
+		ServoMetricReader reader = new ServoMetricReader(registry, new DimensionalServoMetricNaming());
 
 		MonitorConfig.Builder builder = new MonitorConfig.Builder("metricName");
-
-		BasicTimer timer = ServoMonitorCache.getTimer(builder.build());
+		ServoMonitorCache servoMonitorCache = new ServoMonitorCache(registry);
+		servoMonitorCache.getTimer(builder.build());
 
 		List<Metric<?>> metrics = Lists.newArrayList(reader.findAll());
 

spring-cloud-netflix-spectator/src/main/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricsAutoConfiguration.java

@@ -23,6 +23,7 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean
 import org.springframework.cloud.netflix.metrics.DefaultMetricsTagProvider;
 import org.springframework.cloud.netflix.metrics.MetricsInterceptorConfiguration;
 import org.springframework.cloud.netflix.metrics.MetricsTagProvider;
+import org.springframework.cloud.netflix.metrics.servo.ServoMonitorCache;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
@@ -67,6 +68,11 @@ public class SpectatorMetricsAutoConfiguration {
 	}
 
     @Bean
+    public ServoMonitorCache monitorCache() {
+        return new ServoMonitorCache(monitorRegistry());
+    }
+
+	@Bean
 	@ConditionalOnMissingBean(MetricPoller.class)
 	MetricPoller metricPoller() {
 		return new MonitorRegistryMetricPoller();

spring-cloud-netflix-spectator/src/test/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricsHandlerInterceptorIntegrationTests.java

@@ -67,6 +67,9 @@ public class SpectatorMetricsHandlerInterceptorIntegrationTests {
 	@Autowired
 	MonitorRegistry registry;
 
+	@Autowired
+	ServoMonitorCache servoMonitorCache;
+
 	MockMvc mvc;
 
 	@Test
@@ -117,7 +120,7 @@ public class SpectatorMetricsHandlerInterceptorIntegrationTests {
 		if (exceptionType != null)
 			builder = builder.withTag("exception", exceptionType);
 
-		BasicTimer timer = ServoMonitorCache.getTimer(builder.build());
+		BasicTimer timer = servoMonitorCache.getTimer(builder.build());
 		Assert.assertEquals(1L, (long) timer.getCount());
 	}
 }