diff --git a/metrics-api/src/main/java/software/amazon/swage/metrics/MetricContext.java b/metrics-api/src/main/java/software/amazon/swage/metrics/MetricContext.java index e31bc2b..bd5f5ce 100644 --- a/metrics-api/src/main/java/software/amazon/swage/metrics/MetricContext.java +++ b/metrics-api/src/main/java/software/amazon/swage/metrics/MetricContext.java @@ -53,9 +53,10 @@ default void record(Metric label, Number value, Unit unit) { * is often used to record occurrences of an event, such as the number of times * a method is called, an error occurred, or the amount of data sent. *

- * Changes to the count are not timestamped as only the total value of all - * counts for a metric have any meaning. If the individual change needs to - * be tracked, it should be recorded as a gauged event. + * Changes to the count are timestamped with the time of context closure, + * since only the total value of all counts for a metric at the end of a context + * have any meaning. If the individual change needs to be tracked, it should be + * recorded as a gauged event. * * @param label the metric being recorded * @param delta the change in the value diff --git a/metrics-api/src/main/java/software/amazon/swage/metrics/record/MetricRecorder.java b/metrics-api/src/main/java/software/amazon/swage/metrics/record/MetricRecorder.java index 5e3dee5..f7c505f 100644 --- a/metrics-api/src/main/java/software/amazon/swage/metrics/record/MetricRecorder.java +++ b/metrics-api/src/main/java/software/amazon/swage/metrics/record/MetricRecorder.java @@ -15,6 +15,8 @@ package software.amazon.swage.metrics.record; import java.time.Instant; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import software.amazon.swage.collection.TypedMap; import software.amazon.swage.metrics.Metric; @@ -121,6 +123,8 @@ public TypedMap attributes() { public MetricContext context(TypedMap attributes) { R context = newRecorderContext(attributes); return new MetricContext() { + private final Map counts = new ConcurrentHashMap<>(); + @Override public TypedMap attributes() { return context.attributes(); @@ -133,11 +137,12 @@ public void record(Metric label, Number value, Unit unit, Instant time) { @Override public void count(Metric label, long delta) { - MetricRecorder.this.count(label, delta, context); + counts.compute(label, (x, previous) -> (previous == null) ? delta : previous + delta); } @Override public void close() { + counts.forEach((key, value) -> MetricRecorder.this.count(key, value, context)); MetricRecorder.this.close(context); } }; @@ -172,7 +177,7 @@ protected abstract void record( R context); /** - * Count the increase or decrease of a metric in the given context. + * Record the value of a count metric at the closure of a context. * * These are explicitly aggregated values, where only the total number of * occurrences in a context matter. @@ -180,21 +185,20 @@ protected abstract void record( * track of the number of errors encountered, or the amount of attributes sent * while in the given context. * + * Changes to the count are timestamped with the time of context closure, + * as only the total value of all counts for a metric have any meaning. * If you need to distinguish between occurrences of an event, or care that * an event did not occur (for ratios of success for example) then you want * to record those individually and not use a count. - * Changes to the count are not timestamped as only the total value of all - * counts for a metric have any meaning - if the individual change needs to - * be tracked, you probably want to record the change as a gauged event. * * This method will fail silently, as application code is not expected * to depend on success or failure. * * @param label the metric to capture - * @param delta the increase (or decrease) in the value + * @param value the total value of the count at the end of the context * @param context the context the value belongs in. */ - protected abstract void count(Metric label, long delta, R context); + protected abstract void count(Metric label, long value, R context); /** diff --git a/metrics-api/src/test/java/software/amazon/swage/metrics/record/MetricRecorderTest.java b/metrics-api/src/test/java/software/amazon/swage/metrics/record/MetricRecorderTest.java index 1293714..df2f228 100644 --- a/metrics-api/src/test/java/software/amazon/swage/metrics/record/MetricRecorderTest.java +++ b/metrics-api/src/test/java/software/amazon/swage/metrics/record/MetricRecorderTest.java @@ -16,12 +16,16 @@ import java.time.Instant; +import org.junit.Before; import org.junit.Test; import static org.junit.Assert.assertSame; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import software.amazon.swage.collection.TypedMap; @@ -31,31 +35,55 @@ public class MetricRecorderTest { private static final Metric METRIC = Metric.define("Metric"); + private TypedMap attributes = TypedMap.empty(); + private MetricRecorder.RecorderContext recorderContext = new MetricRecorder.RecorderContext(attributes); + private MetricRecorder recorder = spy(new NullRecorder()); + + @Before + public void setup() { + when(recorder.newRecorderContext(attributes)).thenReturn(recorderContext); + } @Test public void recorderContextReturnsSuppliedAttributes() { - TypedMap attributes = TypedMap.empty(); MetricRecorder.RecorderContext context = new MetricRecorder.RecorderContext(attributes); assertSame(attributes, context.attributes()); } @Test - public void recorderDelegatesToImplementation() throws Exception { - TypedMap attributes = TypedMap.empty(); - MetricRecorder.RecorderContext recorderContext = new MetricRecorder.RecorderContext(attributes); - - MetricRecorder recorder = spy(new NullRecorder()); - when(recorder.newRecorderContext(attributes)).thenReturn(recorderContext); - + public void recorderDelegatesRecordToImplementation() throws Exception { MetricContext context = recorder.context(attributes); - Instant timestamp = Instant.now(); context.record(METRIC, 42L, Unit.NONE, timestamp); + verify(recorder).record(eq(METRIC), eq(42L), eq(Unit.NONE), eq(timestamp), same(recorderContext)); + } + + @Test + public void recorderDoesNotDelegateCountToImplementationBeforeClose() throws Exception { + MetricContext context = recorder.context(attributes); context.count(METRIC, 4L); - context.close(); + context.count(METRIC, -2L); + context.count(METRIC, 40L); + verify(recorder, times(0)).count(any(Metric.class), anyLong(), any(MetricRecorder.RecorderContext.class)); + } - verify(recorder).record(eq(METRIC), eq(42L), eq(Unit.NONE), eq(timestamp), same(recorderContext)); + @Test + public void recorderDelegatesCountToImplementationOnClose() throws Exception { + MetricContext context = recorder.context(attributes); + context.count(METRIC, 4L); + context.close(); verify(recorder).count(eq(METRIC), eq(4L), same(recorderContext)); verify(recorder).close(same(recorderContext)); } + + @Test + public void recorderAggregatesCountsAndDelegatesOnce() throws Exception { + MetricContext context = recorder.context(attributes); + context.count(METRIC, 4L); + context.count(METRIC, -2L); + context.count(METRIC, 40L); + context.close(); + verify(recorder, times(1)).count(eq(METRIC), eq(42L), same(recorderContext)); + verify(recorder).close(same(recorderContext)); + } } diff --git a/metrics-api/src/test/java/software/amazon/swage/metrics/record/MultiRecorderTest.java b/metrics-api/src/test/java/software/amazon/swage/metrics/record/MultiRecorderTest.java index 1473c01..a484a97 100644 --- a/metrics-api/src/test/java/software/amazon/swage/metrics/record/MultiRecorderTest.java +++ b/metrics-api/src/test/java/software/amazon/swage/metrics/record/MultiRecorderTest.java @@ -7,8 +7,11 @@ import org.junit.Test; import org.junit.runner.RunWith; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; @@ -43,9 +46,18 @@ public void recordIsSentToAllDelegates() { } @Test - public void countIsSentToAllDelegates() { + public void countIsNotSentToDelegatesBeforeClose() { MetricContext context = recorder.context(attributes); context.count(METRIC, 42L); + verify(delegate1, times(0)).count(any(Metric.class), anyLong(), any(MetricRecorder.RecorderContext.class)); + verify(delegate2, times(0)).count(any(Metric.class), anyLong(), any(MetricRecorder.RecorderContext.class)); + } + + @Test + public void countIsSentToAllDelegatesOnClose() { + MetricContext context = recorder.context(attributes); + context.count(METRIC, 42L); + context.close(); verify(delegate1).count(eq(METRIC), eq(42L), argThat(t -> attributes == t.attributes())); verify(delegate2).count(eq(METRIC), eq(42L), argThat(t -> attributes == t.attributes())); } diff --git a/metrics-core/src/test/java/software/amazon/swage/metrics/record/cloudwatch/CloudWatchRecorderTest.java b/metrics-core/src/test/java/software/amazon/swage/metrics/record/cloudwatch/CloudWatchRecorderTest.java index 804c33d..cdaa817 100644 --- a/metrics-core/src/test/java/software/amazon/swage/metrics/record/cloudwatch/CloudWatchRecorderTest.java +++ b/metrics-core/src/test/java/software/amazon/swage/metrics/record/cloudwatch/CloudWatchRecorderTest.java @@ -197,10 +197,10 @@ public void aggregation() throws Exception { StandardUnit.Percent)); expected.add(makeDatum(id, M_FAIL.toString(), - Arrays.stream(failCnts).sum(), - Arrays.stream(failCnts).min().getAsInt(), - Arrays.stream(failCnts).max().getAsInt(), - failCnts.length, + 46, + 46, + 46, + 1, StandardUnit.Count));