From 63b0c8c67d0bf6439773e6e1975e1918addf7159 Mon Sep 17 00:00:00 2001 From: Brian Harrington Date: Thu, 28 Jan 2016 09:10:36 -0800 Subject: [PATCH] fix registration bug in SpectatorMetricServices The primary change is to fix a memory leak reported in Netflix/spectator#264. Each time a gauge was updated it was creating a new registration and because the map holds a strong reference these would never get collected. Further, the aggregate value created by the multiple registrations was not correct. In addition I added some test cases around the various inputs and checked that the results were reflected as expected in the registry. I noticed the timer values had a unit of milliseconds, but it isn't immediately clear if the reported value can ever less than 1.0. The conversion to long is now delayed until after converting to nanoseconds so duration values less than 1.0 will now work instead of just recording 0. For the histogram I changed to just using a cast to `long` to avoid boxing to a `Double`. As an FYI for the future, there is a DoubleDistributionSummary we have experimented with in spectator-ext-sandbox that might be more appropriate for this use-case. --- .../spectator/SpectatorMetricServices.java | 36 ++------ .../SpectatorMetricServicesTests.java | 84 +++++++++++++++++++ 2 files changed, 91 insertions(+), 29 deletions(-) diff --git a/spring-cloud-netflix-spectator/src/main/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServices.java b/spring-cloud-netflix-spectator/src/main/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServices.java index fc25e853..55162f75 100644 --- a/spring-cloud-netflix-spectator/src/main/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServices.java +++ b/spring-cloud-netflix-spectator/src/main/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServices.java @@ -13,7 +13,6 @@ package org.springframework.cloud.netflix.metrics.spectator; -import java.util.Collections; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; @@ -22,10 +21,7 @@ import java.util.concurrent.atomic.AtomicLong; import org.springframework.boot.actuate.metrics.CounterService; import org.springframework.boot.actuate.metrics.GaugeService; -import com.netflix.spectator.api.AbstractMeter; -import com.netflix.spectator.api.Gauge; import com.netflix.spectator.api.Id; -import com.netflix.spectator.api.Measurement; import com.netflix.spectator.api.Registry; import com.netflix.spectator.impl.AtomicDouble; @@ -74,7 +70,6 @@ public class SpectatorMetricServices implements CounterService, GaugeService { final Id id = registry.createId(name); final AtomicLong gauge = getCounterStorage(id); gauge.addAndGet(value); - registry.register(new NumericGauge(id, gauge)); } } @@ -87,48 +82,31 @@ public class SpectatorMetricServices implements CounterService, GaugeService { @Override public void submit(String name, double dValue) { - long value = ((Double) dValue).longValue(); if (name.startsWith("histogram.")) { - registry.distributionSummary(stripMetricName(name)).record(value); + registry.distributionSummary(stripMetricName(name)).record((long) dValue); } else if (name.startsWith("timer.")) { - registry.timer(stripMetricName(name)).record(value, TimeUnit.MILLISECONDS); + // Input is in milliseconds. Convert to nanos before casting to long to allow + // sub-millisecond durations to be recorded correctly. + long value = (long) (dValue * 1e6); + registry.timer(stripMetricName(name)).record(value, TimeUnit.NANOSECONDS); } else { final Id id = registry.createId(name); final AtomicDouble gauge = getGaugeStorage(id); gauge.set(dValue); - registry.register(new NumericGauge(id, gauge)); } } private AtomicDouble getGaugeStorage(Id id) { final AtomicDouble newGauge = new AtomicDouble(0); final AtomicDouble existingGauge = gauges.putIfAbsent(id, newGauge); - return existingGauge == null ? newGauge : existingGauge; + return existingGauge == null ? registry.gauge(id, newGauge) : existingGauge; } private AtomicLong getCounterStorage(Id id) { final AtomicLong newCounter = new AtomicLong(0); final AtomicLong existingCounter = counters.putIfAbsent(id, newCounter); - return existingCounter == null ? newCounter : existingCounter; - } - - private class NumericGauge extends AbstractMeter implements Gauge { - NumericGauge(Id id, Number val) { - super(registry.clock(), id, val); - } - - @Override - public Iterable measure() { - return Collections.singleton(new Measurement(this.id, this.clock.wallTime(), - this.value())); - } - - @SuppressWarnings("ConstantConditions") - @Override - public double value() { - return this.ref.get().doubleValue(); - } + return existingCounter == null ? registry.gauge(id, newCounter) : existingCounter; } } diff --git a/spring-cloud-netflix-spectator/src/test/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServicesTests.java b/spring-cloud-netflix-spectator/src/test/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServicesTests.java index 17ee9f2e..0624ff88 100644 --- a/spring-cloud-netflix-spectator/src/test/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServicesTests.java +++ b/spring-cloud-netflix-spectator/src/test/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServicesTests.java @@ -13,9 +13,16 @@ package org.springframework.cloud.netflix.metrics.spectator; +import com.netflix.spectator.api.DefaultRegistry; +import com.netflix.spectator.api.Measurement; +import com.netflix.spectator.api.Registry; import org.junit.Test; +import java.util.Iterator; +import java.util.concurrent.TimeUnit; + import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.springframework.cloud.netflix.metrics.spectator.SpectatorMetricServices.stripMetricName; public class SpectatorMetricServicesTests { @@ -36,4 +43,81 @@ public class SpectatorMetricServicesTests { public void metricTypeNameEmbeddedInMiddleOfMetricNameIsNotRemoved() { assertEquals("bar.timer.foo", stripMetricName("bar.timer.foo")); } + + @Test + public void meterPrefixShouldIncrementCounter() { + Registry registry = new DefaultRegistry(); + SpectatorMetricServices ms = new SpectatorMetricServices(registry); + ms.increment("meter.test"); + assertEquals(1, registry.counter("test").count()); + } + + @Test + public void otherPrefixShouldUpdateGauge() { + Registry registry = new DefaultRegistry(); + SpectatorMetricServices ms = new SpectatorMetricServices(registry); + + ms.increment("gauge.test"); + assertGaugeEquals(registry, "gauge.test", 1.0); + + ms.decrement("gauge.test"); + assertGaugeEquals(registry, "gauge.test", 0.0); + } + + @Test + public void histogramSubmit() { + Registry registry = new DefaultRegistry(); + SpectatorMetricServices ms = new SpectatorMetricServices(registry); + ms.submit("histogram.test", 42.0); + assertEquals(1L, registry.distributionSummary("test").count()); + assertEquals(42L, registry.distributionSummary("test").totalAmount()); + } + + @Test + public void timerSubmit() { + Registry registry = new DefaultRegistry(); + SpectatorMetricServices ms = new SpectatorMetricServices(registry); + ms.submit("timer.test", 42.0); + assertEquals(1L, registry.timer("test").count()); + assertEquals(TimeUnit.MILLISECONDS.toNanos(42L), registry.timer("test").totalTime()); + } + + @Test + public void timerMicrosSubmit() { + Registry registry = new DefaultRegistry(); + SpectatorMetricServices ms = new SpectatorMetricServices(registry); + ms.submit("timer.test", 0.042); + assertEquals(1L, registry.timer("test").count()); + assertEquals(TimeUnit.MICROSECONDS.toNanos(42L), registry.timer("test").totalTime()); + } + + @Test + public void gaugeSubmit() { + Registry registry = new DefaultRegistry(); + SpectatorMetricServices ms = new SpectatorMetricServices(registry); + ms.submit("gauge.test", 42.0); + assertGaugeEquals(registry, "gauge.test", 42.0); + + ms.submit("gauge.test", 1.0); + assertGaugeEquals(registry, "gauge.test", 1.0); + } + + @Test + public void gaugeSubmitManyTimes() { + // Sanity check for memory leak reported in: + // https://github.com/Netflix/spectator/issues/264 + Registry registry = new DefaultRegistry(); + SpectatorMetricServices ms = new SpectatorMetricServices(registry); + for (int i = 0; i < 10000; ++i) { + ms.submit("gauge.test", 42.0); + assertGaugeEquals(registry, "gauge.test", 42.0); + } + } + + private void assertGaugeEquals(Registry registry, String name, double expected) { + Iterator it = registry.get(registry.createId(name)).measure().iterator(); + assertTrue(it.hasNext()); + assertEquals(expected, it.next().value(), 1e-3); + assertTrue(!it.hasNext()); + } }