From 860d7b8446433b94cb3811118eadc003a2af0137 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Mon, 2 May 2016 12:27:33 -0600 Subject: [PATCH] Log warning if ServoMonitorCache size exceeds threshold. Fixes gh-947 --- .../servo/ServoMetricsAutoConfiguration.java | 4 +- .../metrics/servo/ServoMetricsConfigBean.java | 22 +++++++-- .../metrics/servo/ServoMonitorCache.java | 18 ++++++-- .../metrics/servo/ServoMetricReaderTests.java | 2 +- .../metrics/servo/ServoMonitorCacheTests.java | 46 +++++++++++++++++++ .../SpectatorMetricsAutoConfiguration.java | 18 +++++--- 6 files changed, 92 insertions(+), 18 deletions(-) create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/servo/ServoMonitorCacheTests.java diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricsAutoConfiguration.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricsAutoConfiguration.java index 08306afd..1dfd7cd4 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricsAutoConfiguration.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricsAutoConfiguration.java @@ -71,8 +71,8 @@ public class ServoMetricsAutoConfiguration { } @Bean - public ServoMonitorCache monitorCache(MonitorRegistry monitorRegistry) { - return new ServoMonitorCache(monitorRegistry); + public ServoMonitorCache monitorCache(MonitorRegistry monitorRegistry, ServoMetricsConfigBean servoMetricsConfig) { + return new ServoMonitorCache(monitorRegistry, servoMetricsConfig); } @Bean diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricsConfigBean.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricsConfigBean.java index eb69c153..cbae9f50 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricsConfigBean.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricsConfigBean.java @@ -34,19 +34,33 @@ public class ServoMetricsConfigBean { */ String registryClass = "com.netflix.servo.BasicMonitorRegistry"; - public String getRegistryClass() { - return this.registryClass; - } + /** + * When the `ServoMonitorCache` reaches this size, a warning is logged. + * This will be useful if you are using string concatenation in RestTemplate urls. + */ + int cacheWarningThreshold = 1000; public boolean getEnabled() { return this.enabled; } - public void setEnabled(boolean enabled) { + public void isEnabled(boolean enabled) { this.enabled = enabled; } + public String getRegistryClass() { + return this.registryClass; + } + public void setRegistryClass(String registryClass) { this.registryClass = registryClass; } + + public int getCacheWarningThreshold() { + return cacheWarningThreshold; + } + + public void setCacheWarningThreshold(int cacheWarningThreshold) { + this.cacheWarningThreshold = cacheWarningThreshold; + } } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMonitorCache.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMonitorCache.java index 1a0011cc..35ad0a21 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMonitorCache.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/metrics/servo/ServoMonitorCache.java @@ -20,17 +20,22 @@ import com.netflix.servo.MonitorRegistry; import com.netflix.servo.monitor.BasicTimer; import com.netflix.servo.monitor.MonitorConfig; +import lombok.extern.apachecommons.CommonsLog; + /** * Servo does not provide a mechanism to retrieve an existing monitor by name + tags. * * @author Jon Schneider */ +@CommonsLog public class ServoMonitorCache { private final Map timerCache = new HashMap<>(); private final MonitorRegistry monitorRegistry; + private final ServoMetricsConfigBean config; - public ServoMonitorCache(MonitorRegistry monitorRegistry) { + public ServoMonitorCache(MonitorRegistry monitorRegistry, ServoMetricsConfigBean config) { this.monitorRegistry = monitorRegistry; + this.config = config; } /** @@ -39,13 +44,18 @@ public class ServoMonitorCache { * return it. */ public synchronized BasicTimer getTimer(MonitorConfig config) { - BasicTimer t = timerCache.get(config); + BasicTimer t = this.timerCache.get(config); if (t != null) return t; t = new BasicTimer(config); - timerCache.put(config, t); - monitorRegistry.register(t); + this.timerCache.put(config, t); + + if (this.timerCache.size() > this.config.getCacheWarningThreshold()) { + log.warn("timerCache is above the warning threshold of " + this.config.getCacheWarningThreshold() + " with size " + this.timerCache.size() + "."); + } + + this.monitorRegistry.register(t); return t; } } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricReaderTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricReaderTests.java index 33561961..dbb43e1a 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricReaderTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/servo/ServoMetricReaderTests.java @@ -21,7 +21,7 @@ public class ServoMetricReaderTests { new DimensionalServoMetricNaming()); MonitorConfig.Builder builder = new MonitorConfig.Builder("metricName"); - ServoMonitorCache servoMonitorCache = new ServoMonitorCache(registry); + ServoMonitorCache servoMonitorCache = new ServoMonitorCache(registry, new ServoMetricsConfigBean()); servoMonitorCache.getTimer(builder.build()); List> metrics = Lists.newArrayList(reader.findAll()); diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/servo/ServoMonitorCacheTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/servo/ServoMonitorCacheTests.java new file mode 100644 index 00000000..a7bddf45 --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/metrics/servo/ServoMonitorCacheTests.java @@ -0,0 +1,46 @@ +package org.springframework.cloud.netflix.metrics.servo; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.springframework.boot.test.OutputCapture; + +import com.netflix.servo.MonitorRegistry; +import com.netflix.servo.monitor.MonitorConfig; + +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.*; + +/** + * @author Spencer Gibb + */ +public class ServoMonitorCacheTests { + + @Rule + public OutputCapture capture = new OutputCapture(); + + @Mock + MonitorRegistry monitorRegistry; + + @Before + public void before() { + MockitoAnnotations.initMocks(this); + } + + @Test + public void testCacheWarningLog() { + ServoMetricsConfigBean config = new ServoMetricsConfigBean(); + config.setCacheWarningThreshold(1); + ServoMonitorCache cache = new ServoMonitorCache(monitorRegistry, config); + + cache.getTimer(MonitorConfig.builder("monitorA").build()); + + assertThat(this.capture.toString(), not(containsString("timerCache is above the warning threshold"))); + + cache.getTimer(MonitorConfig.builder("monitorB").build()); + + assertThat(this.capture.toString(), containsString("timerCache is above the warning threshold")); + } +} diff --git a/spring-cloud-netflix-spectator/src/main/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricsAutoConfiguration.java b/spring-cloud-netflix-spectator/src/main/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricsAutoConfiguration.java index bb3b9d12..35fad799 100644 --- a/spring-cloud-netflix-spectator/src/main/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricsAutoConfiguration.java +++ b/spring-cloud-netflix-spectator/src/main/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricsAutoConfiguration.java @@ -13,7 +13,6 @@ package org.springframework.cloud.netflix.metrics.spectator; -import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.actuate.autoconfigure.MetricRepositoryAutoConfiguration; import org.springframework.boot.actuate.endpoint.MetricReaderPublicMetrics; import org.springframework.boot.actuate.metrics.CounterService; @@ -25,6 +24,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.ServoMetricsConfigBean; import org.springframework.cloud.netflix.metrics.servo.ServoMonitorCache; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -50,14 +50,18 @@ import com.netflix.spectator.servo.ServoRegistry; @ConditionalOnClass({ Registry.class, MetricReader.class }) @Import(MetricsInterceptorConfiguration.class) public class SpectatorMetricsAutoConfiguration { - @Value("${netflix.metrics.servo.registryClass:com.netflix.servo.BasicMonitorRegistry}") - String servoRegistryClass; @Bean @ConditionalOnMissingBean - public MonitorRegistry monitorRegistry() { + public ServoMetricsConfigBean servoMetricsConfig() { + return new ServoMetricsConfigBean(); + } + + @Bean + @ConditionalOnMissingBean + public MonitorRegistry monitorRegistry(ServoMetricsConfigBean configBean) { System.setProperty(DefaultMonitorRegistry.class.getCanonicalName() - + ".registryClass", servoRegistryClass); + + ".registryClass", configBean.getRegistryClass()); return DefaultMonitorRegistry.getInstance(); } @@ -68,8 +72,8 @@ public class SpectatorMetricsAutoConfiguration { } @Bean - public ServoMonitorCache monitorCache() { - return new ServoMonitorCache(monitorRegistry()); + public ServoMonitorCache monitorCache(MonitorRegistry monitorRegistry, ServoMetricsConfigBean configBean) { + return new ServoMonitorCache(monitorRegistry, configBean); } @Bean