Skip to content

S
spring-cloud-netflix
Commit 63b0c8c6 by Brian Harrington Committed by Spencer Gibb
Options

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.
parent 32c0f909
Showing with 91 additions and 29 deletions
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<Number> implements Gauge {
NumericGauge(Id id, Number val) {
super(registry.clock(), id, val);
}
@Override
public Iterable<Measurement> 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;
}
}
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<Measurement> it = registry.get(registry.createId(name)).measure().iterator();
assertTrue(it.hasNext());
assertEquals(expected, it.next().value(), 1e-3);
assertTrue(!it.hasNext());
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment