From 0cbd0b609a61d698092f6d52317ce984fa8caf4a Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Thu, 22 May 2014 12:02:51 +0100 Subject: [PATCH] Add increment() to PrefixMetricWriter In the redis repository we also switch to store the value in the zset (so it can be atomically incremented) rather than in the regular key-value. Fixes gh-929 --- .../export/PrefixMetricGroupExporter.java | 2 +- .../metrics/export/RichGaugeExporter.java | 17 +++----- .../repository/InMemoryMetricRepository.java | 16 +++++++- .../redis/RedisMultiMetricRepository.java | 39 +++++++++++++------ .../metrics/writer/PrefixMetricWriter.java | 9 ++++- .../PrefixMetricGroupExporterTests.java | 2 +- .../InMemoryPrefixMetricRepositoryTests.java | 18 ++++++++- .../RedisMultiMetricRepositoryTests.java | 30 +++++++++++--- 8 files changed, 100 insertions(+), 33 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/PrefixMetricGroupExporter.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/PrefixMetricGroupExporter.java index 48c8837d24..21ab561485 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/PrefixMetricGroupExporter.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/PrefixMetricGroupExporter.java @@ -85,7 +85,7 @@ public class PrefixMetricGroupExporter extends AbstractMetricExporter { @Override protected void write(String group, Collection> values) { - this.writer.save(group, values); + this.writer.set(group, values); } } diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/RichGaugeExporter.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/RichGaugeExporter.java index 550e019019..273f246108 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/RichGaugeExporter.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/export/RichGaugeExporter.java @@ -25,6 +25,7 @@ import org.springframework.boot.actuate.metrics.repository.MultiMetricRepository import org.springframework.boot.actuate.metrics.rich.RichGauge; import org.springframework.boot.actuate.metrics.rich.RichGaugeReader; import org.springframework.boot.actuate.metrics.writer.MetricWriter; +import org.springframework.boot.actuate.metrics.writer.PrefixMetricWriter; /** * Exporter or converter for {@link RichGauge} data to a metric-based back end. Each gauge @@ -53,13 +54,14 @@ public class RichGaugeExporter extends AbstractMetricExporter { private static final String ALPHA = ".alpha"; private final RichGaugeReader reader; - private final MetricWriter writer; + private final PrefixMetricWriter writer; - public RichGaugeExporter(RichGaugeReader reader, MetricWriter writer) { + public RichGaugeExporter(RichGaugeReader reader, PrefixMetricWriter writer) { this(reader, writer, ""); } - public RichGaugeExporter(RichGaugeReader reader, MetricWriter writer, String prefix) { + public RichGaugeExporter(RichGaugeReader reader, PrefixMetricWriter writer, + String prefix) { super(prefix); this.reader = reader; this.writer = writer; @@ -89,14 +91,7 @@ public class RichGaugeExporter extends AbstractMetricExporter { @Override protected void write(String group, Collection> values) { - if (this.writer instanceof MultiMetricRepository) { - ((MultiMetricRepository) this.writer).save(group, values); - } - else { - for (Metric value : values) { - this.writer.set(value); - } - } + this.writer.set(group, values); } } diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/InMemoryMetricRepository.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/InMemoryMetricRepository.java index 72ec83bbc7..e4db5accc0 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/InMemoryMetricRepository.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/InMemoryMetricRepository.java @@ -71,7 +71,7 @@ public class InMemoryMetricRepository implements MetricRepository, MultiMetricRe } @Override - public void save(String group, Collection> values) { + public void set(String group, Collection> values) { String prefix = group; if (!prefix.endsWith(".")) { prefix = prefix + "."; @@ -86,6 +86,20 @@ public class InMemoryMetricRepository implements MetricRepository, MultiMetricRe this.groups.add(group); } + @Override + public void increment(String group, Delta delta) { + String prefix = group; + if (!prefix.endsWith(".")) { + prefix = prefix + "."; + } + if (!delta.getName().startsWith(prefix)) { + delta = new Delta(prefix + delta.getName(), delta.getValue(), + delta.getTimestamp()); + } + increment(delta); + this.groups.add(group); + } + @Override public Iterable groups() { return Collections.unmodifiableCollection(this.groups); diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepository.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepository.java index 4611f98901..f4b5ffb93d 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepository.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepository.java @@ -25,6 +25,7 @@ import java.util.Set; import org.springframework.boot.actuate.metrics.Metric; import org.springframework.boot.actuate.metrics.repository.MultiMetricRepository; +import org.springframework.boot.actuate.metrics.writer.Delta; import org.springframework.data.redis.connection.RedisConnectionFactory; import org.springframework.data.redis.core.BoundZSetOperations; import org.springframework.data.redis.core.RedisOperations; @@ -67,10 +68,10 @@ public class RedisMultiMetricRepository implements MultiMetricRepository { } @Override - public Iterable> findAll(String metricNamePrefix) { + public Iterable> findAll(String group) { BoundZSetOperations zSetOperations = this.redisOperations - .boundZSetOps(keyFor(metricNamePrefix)); + .boundZSetOps(keyFor(group)); Set keys = zSetOperations.range(0, -1); Iterator keysIt = keys.iterator(); @@ -78,14 +79,15 @@ public class RedisMultiMetricRepository implements MultiMetricRepository { List> result = new ArrayList>(keys.size()); List values = this.redisOperations.opsForValue().multiGet(keys); for (String v : values) { - result.add(deserialize(keysIt.next(), v)); + String key = keysIt.next(); + result.add(deserialize(group, key, v, zSetOperations.score(key))); } return result; } @Override - public void save(String group, Collection> values) { + public void set(String group, Collection> values) { String groupKey = keyFor(group); trackMembership(groupKey); BoundZSetOperations zSetOperations = this.redisOperations @@ -93,11 +95,24 @@ public class RedisMultiMetricRepository implements MultiMetricRepository { for (Metric metric : values) { String raw = serialize(metric); String key = keyFor(metric.getName()); - zSetOperations.add(key, 0.0D); + zSetOperations.add(key, metric.getValue().doubleValue()); this.redisOperations.opsForValue().set(key, raw); } } + @Override + public void increment(String group, Delta delta) { + String groupKey = keyFor(group); + trackMembership(groupKey); + BoundZSetOperations zSetOperations = this.redisOperations + .boundZSetOps(groupKey); + String key = keyFor(delta.getName()); + double value = zSetOperations.incrementScore(key, delta.getValue().doubleValue()); + String raw = serialize(new Metric(delta.getName(), value, + delta.getTimestamp())); + this.redisOperations.opsForValue().set(key, raw); + } + @Override public Iterable groups() { Set range = this.zSetOperations.range(0, -1); @@ -128,15 +143,17 @@ public class RedisMultiMetricRepository implements MultiMetricRepository { this.zSetOperations.remove(groupKey); } - private Metric deserialize(String redisKey, String v) { - String[] vals = v.split("@"); - Double value = Double.valueOf(vals[0]); - Date timestamp = vals.length > 1 ? new Date(Long.valueOf(vals[1])) : new Date(); - return new Metric(nameFor(redisKey), value, timestamp); + private Metric deserialize(String group, String redisKey, String v, Double value) { + String prefix = group; + if (!group.endsWith(".")) { + prefix = group + "."; + } + Date timestamp = new Date(Long.valueOf(v)); + return new Metric(prefix + nameFor(redisKey), value, timestamp); } private String serialize(Metric entity) { - return String.valueOf(entity.getValue() + "@" + entity.getTimestamp().getTime()); + return String.valueOf(entity.getTimestamp().getTime()); } private String keyFor(String name) { diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/writer/PrefixMetricWriter.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/writer/PrefixMetricWriter.java index d3594e8117..d358acc805 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/writer/PrefixMetricWriter.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/writer/PrefixMetricWriter.java @@ -33,7 +33,14 @@ public interface PrefixMetricWriter { * @param group the name of the group * @param values the metric values to save */ - void save(String group, Collection> values); + void set(String group, Collection> values); + + /** + * Increment the value of a metric (or decrement if the delta is negative). The name + * of the metric to increment is group + "." + delta.name. + * @param delta the amount to increment by + */ + void increment(String group, Delta delta); /** * Rest the values of all metrics in the group. Implementations may choose to discard diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/export/PrefixMetricGroupExporterTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/export/PrefixMetricGroupExporterTests.java index eb6ee2f358..de4871eec9 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/export/PrefixMetricGroupExporterTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/export/PrefixMetricGroupExporterTests.java @@ -60,7 +60,7 @@ public class PrefixMetricGroupExporterTests { @Test public void multiMetricGroupsCopiedAsDefault() { - this.reader.save("foo", Arrays.> asList(new Metric("bar", 2.3), + this.reader.set("foo", Arrays.> asList(new Metric("bar", 2.3), new Metric("spam", 1.3))); this.exporter.export(); assertEquals(1, this.writer.countGroups()); diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/InMemoryPrefixMetricRepositoryTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/InMemoryPrefixMetricRepositoryTests.java index cbf3b99c04..aaf09410d2 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/InMemoryPrefixMetricRepositoryTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/InMemoryPrefixMetricRepositoryTests.java @@ -47,7 +47,7 @@ public class InMemoryPrefixMetricRepositoryTests { } @Test - public void perfixWithWildcard() { + public void prefixWithWildcard() { this.repository.increment(new Delta("foo.bar", 1)); Set names = new HashSet(); for (Metric metric : this.repository.findAll("foo.*")) { @@ -58,7 +58,7 @@ public class InMemoryPrefixMetricRepositoryTests { } @Test - public void perfixWithPeriod() { + public void prefixWithPeriod() { this.repository.increment(new Delta("foo.bar", 1)); Set names = new HashSet(); for (Metric metric : this.repository.findAll("foo.")) { @@ -80,4 +80,18 @@ public class InMemoryPrefixMetricRepositoryTests { assertTrue(names.contains("foo.bar")); } + @Test + public void incrementGroup() { + this.repository.increment("foo", new Delta("bar", 1)); + this.repository.increment("foo", new Delta("bar", 2)); + this.repository.increment("foo", new Delta("spam", 1)); + Set names = new HashSet(); + for (Metric metric : this.repository.findAll("foo")) { + names.add(metric.getName()); + } + assertEquals(2, names.size()); + assertTrue(names.contains("foo.bar")); + assertEquals(3L, this.repository.findOne("foo.bar").getValue()); + } + } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepositoryTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepositoryTests.java index d8366ddc0e..51d5394692 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepositoryTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/repository/redis/RedisMultiMetricRepositoryTests.java @@ -17,7 +17,9 @@ package org.springframework.boot.actuate.metrics.repository.redis; import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import java.util.Set; import org.junit.After; import org.junit.Before; @@ -29,6 +31,7 @@ import org.junit.runners.Parameterized.Parameter; import org.junit.runners.Parameterized.Parameters; import org.springframework.boot.actuate.metrics.Iterables; import org.springframework.boot.actuate.metrics.Metric; +import org.springframework.boot.actuate.metrics.writer.Delta; import org.springframework.data.redis.core.StringRedisTemplate; import static org.junit.Assert.assertEquals; @@ -78,27 +81,44 @@ public class RedisMultiMetricRepositoryTests { @Test public void setAndGet() { - this.repository.save("foo", Arrays.> asList(new Metric( + this.repository.set("foo", Arrays.> asList(new Metric( "foo.val", 12.3), new Metric("foo.bar", 11.3))); assertEquals(2, Iterables.collection(this.repository.findAll("foo")).size()); } @Test public void groups() { - this.repository.save("foo", Arrays.> asList(new Metric( + this.repository.set("foo", Arrays.> asList(new Metric( "foo.val", 12.3), new Metric("foo.bar", 11.3))); - this.repository.save("bar", Arrays.> asList(new Metric( + this.repository.set("bar", Arrays.> asList(new Metric( "bar.val", 12.3), new Metric("bar.foo", 11.3))); assertEquals(2, Iterables.collection(this.repository.groups()).size()); } @Test public void count() { - this.repository.save("foo", Arrays.> asList(new Metric( + this.repository.set("foo", Arrays.> asList(new Metric( "foo.val", 12.3), new Metric("foo.bar", 11.3))); - this.repository.save("bar", Arrays.> asList(new Metric( + this.repository.set("bar", Arrays.> asList(new Metric( "bar.val", 12.3), new Metric("bar.foo", 11.3))); assertEquals(2, this.repository.countGroups()); } + @Test + public void increment() { + this.repository.increment("foo", new Delta("bar", 1)); + this.repository.increment("foo", new Delta("bar", 2)); + this.repository.increment("foo", new Delta("spam", 1)); + Metric bar = null; + Set names = new HashSet(); + for (Metric metric : this.repository.findAll("foo")) { + names.add(metric.getName()); + if (metric.getName().equals("foo.bar")) { + bar = metric; + } + } + assertEquals(2, names.size()); + assertTrue("Wrong names: " + names, names.contains("foo.bar")); + assertEquals(3d, bar.getValue()); + } }