Skip to content

Commit

Permalink
fix registration bug in SpectatorMetricServices
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@brharrington
brharrington committed Jan 28, 2016
1 parent 29e691a commit 053c17d
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 29 deletions.
36 changes: 7 additions & 29 deletions ...ain/java/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServices.java
View file
Expand Up @@ -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;
Expand All @@ -22,10 +21,7 @@
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;

Expand Down Expand Up @@ -74,7 +70,6 @@ else if (name.startsWith("meter.")) {
final Id id = registry.createId(name);
final AtomicLong gauge = getCounterStorage(id);
gauge.addAndGet(value);
registry.register(new NumericGauge(id, gauge));
}
}

Expand All @@ -87,48 +82,31 @@ public void reset(String name) {

@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;
}
}
84 changes: 84 additions & 0 deletions ...ava/org/springframework/cloud/netflix/metrics/spectator/SpectatorMetricServicesTests.java
View file
Expand Up @@ -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 {
Expand All @@ -36,4 +43,81 @@ public void metricTypeIsStrippedFromMetricName() {
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());
}
}

0 comments on commit 053c17d

Please sign in to comment.