diff --git a/core/src/main/java/com/uber/m3/tally/AbstractBuckets.java b/core/src/main/java/com/uber/m3/tally/AbstractBuckets.java index fdd60db..d9c2db9 100644 --- a/core/src/main/java/com/uber/m3/tally/AbstractBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/AbstractBuckets.java @@ -21,8 +21,8 @@ package com.uber.m3.tally; import com.uber.m3.util.Duration; +import com.uber.m3.util.ImmutableList; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Iterator; @@ -30,21 +30,30 @@ import java.util.ListIterator; /** - * Abstract {@link Buckets} implementation for common functionality. + * @deprecated DO NOT USE + * + * Please use {@link ImmutableBuckets} instead */ -public abstract class AbstractBuckets implements Buckets { +@Deprecated +public abstract class AbstractBuckets> implements Buckets { protected List buckets; AbstractBuckets(T[] buckets) { if (buckets == null) { - this.buckets = new ArrayList<>(); - } else { - this.buckets = new ArrayList<>(Arrays.asList(buckets)); + throw new IllegalArgumentException("provided buckets could not be null"); } + + validate(buckets); + + this.buckets = new ImmutableList<>(Arrays.asList(buckets)); } - AbstractBuckets() { - this(null); + public void validate(T[] buckets) { + for (int i = 1; i < buckets.length; ++i) { + if (buckets[i - 1].compareTo(buckets[i]) > 0) { + throw new IllegalArgumentException("buckets should be in a non-decreasing order"); + } + } } @Override diff --git a/core/src/main/java/com/uber/m3/tally/BucketPairImpl.java b/core/src/main/java/com/uber/m3/tally/BucketPairImpl.java index 9449dd2..b30d425 100644 --- a/core/src/main/java/com/uber/m3/tally/BucketPairImpl.java +++ b/core/src/main/java/com/uber/m3/tally/BucketPairImpl.java @@ -22,12 +22,10 @@ import com.uber.m3.util.Duration; -import java.util.Collections; - /** * Default implementation of a {@link BucketPair} * - * @deprecated will be removed + * @deprecated DO NOT USE, WILL BE REMOVED IN THE NEXT VERSION */ @Deprecated public class BucketPairImpl implements BucketPair { @@ -60,8 +58,6 @@ public static BucketPair[] bucketPairs(Buckets buckets) { }; } - Collections.sort(buckets); - Double[] asValueBuckets = buckets.asValues(); Duration[] asDurationBuckets = buckets.asDurations(); BucketPair[] pairs = new BucketPair[buckets.size() + 1]; diff --git a/core/src/main/java/com/uber/m3/tally/Buckets.java b/core/src/main/java/com/uber/m3/tally/Buckets.java index 22175ee..ff4d576 100644 --- a/core/src/main/java/com/uber/m3/tally/Buckets.java +++ b/core/src/main/java/com/uber/m3/tally/Buckets.java @@ -25,18 +25,22 @@ import java.util.List; /** - * Buckets of a {@link Histogram}. + * @deprecated DO NOT USE + * + * Please use {@link ImmutableBuckets} instead */ -public interface Buckets extends List { +@Deprecated +public interface Buckets extends ImmutableBuckets, List { /** - * Returns these buckets as {@code double}s. - * @return an array of {@code double}s representing these buckets + * @deprecated DO NOT USE */ + @Deprecated Double[] asValues(); /** - * Returns these buckets as {@link Duration}s. - * @return an array of {@link Duration}s representing these buckets + * @deprecated DO NOT USE */ + @Deprecated Duration[] asDurations(); + } diff --git a/core/src/main/java/com/uber/m3/tally/DurationBuckets.java b/core/src/main/java/com/uber/m3/tally/DurationBuckets.java index 91a9d77..0ca3a48 100644 --- a/core/src/main/java/com/uber/m3/tally/DurationBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/DurationBuckets.java @@ -22,18 +22,62 @@ import com.uber.m3.util.Duration; +import java.util.Collections; +import java.util.List; + /** * {@link Buckets} implementation backed by {@link Duration}s. */ public class DurationBuckets extends AbstractBuckets { + public DurationBuckets(Duration[] durations) { super(durations); } - public DurationBuckets() { - super(); + @Override + public Duration getDurationLowerBoundFor(int bucketIndex) { + return bucketIndex == 0 ? Duration.MIN_VALUE : buckets.get(bucketIndex - 1); + } + + @Override + public Duration getDurationUpperBoundFor(int bucketIndex) { + return bucketIndex < buckets.size() ? buckets.get(bucketIndex) : Duration.MAX_VALUE; + } + + @Override + public int getBucketIndexFor(Duration value) { + return HistogramImpl.toBucketIndex(Collections.binarySearch(buckets, value)); + } + + @Override + public List getDurationUpperBounds() { + return Collections.unmodifiableList(buckets); + } + + @Override + public double getValueLowerBoundFor(int bucketIndex) { + throw new UnsupportedOperationException("not supported"); + } + + @Override + public double getValueUpperBoundFor(int bucketIndex) { + throw new UnsupportedOperationException("not supported"); + } + + @Override + public int getBucketIndexFor(double value) { + throw new UnsupportedOperationException("not supported"); } + @Override + public List getValueUpperBounds() { + throw new UnsupportedOperationException("not supported"); + } + + /** + * @deprecated DO NOT USE + */ + @Deprecated @Override public Double[] asValues() { Double[] values = new Double[buckets.size()]; @@ -45,6 +89,10 @@ public Double[] asValues() { return values; } + /** + * @deprecated DO NOT USE + */ + @Deprecated @Override public Duration[] asDurations() { return buckets.toArray(new Duration[buckets.size()]); diff --git a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java index 93f255f..f7d76fc 100644 --- a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java +++ b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java @@ -23,8 +23,9 @@ import com.uber.m3.util.Duration; import com.uber.m3.util.ImmutableMap; -import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; /** @@ -32,16 +33,15 @@ */ class HistogramImpl extends MetricBase implements Histogram, StopwatchRecorder { private final Type type; + private final ImmutableMap tags; - private final Buckets specification; + + private final ImmutableBuckets specification; // NOTE: Bucket counters are lazily initialized. Since ref updates are atomic in JMM, // no dedicated synchronization is used on the read path, only on the write path private final CounterImpl[] bucketCounters; - private final double[] lookupByValue; - private final Duration[] lookupByDuration; - private final ScopeImpl scope; HistogramImpl( @@ -60,25 +60,17 @@ class HistogramImpl extends MetricBase implements Histogram, StopwatchRecorder { // Each bucket value, serves as a boundary de-marking upper bound // for the bucket to the left, and lower bound for the bucket to the right this.bucketCounters = new CounterImpl[buckets.asValues().length + 1]; - - this.lookupByValue = - Arrays.stream(buckets.asValues()) - .mapToDouble(x -> x) - .toArray(); - - this.lookupByDuration = - Arrays.copyOf(buckets.asDurations(), buckets.asDurations().length); } @Override public void recordValue(double value) { - int index = toBucketIndex(Arrays.binarySearch(lookupByValue, value)); + int index = toBucketIndex(Collections.binarySearch(specification.getValueUpperBounds(), value)); getOrCreateCounter(index).inc(1); } @Override public void recordDuration(Duration duration) { - int index = toBucketIndex(Arrays.binarySearch(lookupByDuration, duration)); + int index = toBucketIndex(Collections.binarySearch(specification.getDurationUpperBounds(), duration)); getOrCreateCounter(index).inc(1); } @@ -87,9 +79,14 @@ private CounterImpl getOrCreateCounter(int index) { return bucketCounters[index]; } + List bucketsBounds = + this.type == Type.VALUE + ? specification.getValueUpperBounds() + : specification.getDurationUpperBounds(); + // To maintain lock granularity we synchronize only on a // particular bucket leveraging bucket's boundary as a sync target - synchronized (lookupByDuration[Math.min(index, lookupByDuration.length - 1)]) { + synchronized (bucketsBounds.get(Math.min(index, bucketsBounds.size() - 1))) { // Check whether bucket has been already set, // while we were waiting for lock if (bucketCounters[index] != null) { @@ -101,7 +98,7 @@ private CounterImpl getOrCreateCounter(int index) { } } - private int toBucketIndex(int binarySearchResult) { + static int toBucketIndex(int binarySearchResult) { // Buckets are defined in the following way: // - Each bucket is inclusive of its lower bound, and exclusive of the upper: [lower, upper) // - All buckets are defined by upper bounds: [2, 4, 8, 16, 32, ...]: therefore i @@ -133,19 +130,19 @@ ImmutableMap getTags() { } private Duration getUpperBoundDurationForBucket(int bucketIndex) { - return bucketIndex < lookupByDuration.length ? lookupByDuration[bucketIndex] : Duration.MAX_VALUE; + return bucketIndex < specification.getDurationUpperBounds().size() ? specification.getDurationUpperBounds().get(bucketIndex) : Duration.MAX_VALUE; } - private double getUpperBoundValueForBucket(int bucketIndex) { - return bucketIndex < lookupByValue.length ? lookupByValue[bucketIndex] : Double.MAX_VALUE; + private Duration getLowerBoundDurationForBucket(int bucketIndex) { + return bucketIndex == 0 ? Duration.MIN_VALUE : specification.getDurationUpperBounds().get(bucketIndex - 1); } - private Duration getLowerBoundDurationForBucket(int bucketIndex) { - return bucketIndex == 0 ? Duration.MIN_VALUE : lookupByDuration[bucketIndex - 1]; + private double getUpperBoundValueForBucket(int bucketIndex) { + return bucketIndex < specification.getValueUpperBounds().size() ? specification.getValueUpperBounds().get(bucketIndex) : Double.MAX_VALUE; } private double getLowerBoundValueForBucket(int bucketIndex) { - return bucketIndex == 0 ? Double.MIN_VALUE : lookupByValue[bucketIndex - 1]; + return bucketIndex == 0 ? Double.MIN_VALUE : specification.getValueUpperBounds().get(bucketIndex - 1); } private long getCounterValue(int index) { @@ -218,7 +215,7 @@ public void report(ImmutableMap tags, StatsReporter reporter) { reporter.reportHistogramValueSamples( getQualifiedName(), tags, - specification, + (Buckets) specification, getLowerBoundValueForBucket(bucketIndex), getUpperBoundValueForBucket(bucketIndex), inc @@ -228,7 +225,7 @@ public void report(ImmutableMap tags, StatsReporter reporter) { reporter.reportHistogramDurationSamples( getQualifiedName(), tags, - specification, + (Buckets) specification, getLowerBoundDurationForBucket(bucketIndex), getUpperBoundDurationForBucket(bucketIndex), inc diff --git a/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java b/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java new file mode 100644 index 0000000..92b7806 --- /dev/null +++ b/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java @@ -0,0 +1,87 @@ +// Copyright (c) 2020 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.uber.m3.tally; + +import com.uber.m3.util.Duration; + +import java.util.List; + +/** + * Abstracts buckets used in {@link Histogram} metrics, + * + * Buckets are defined by the list of upper-bounds in the following way: + * + *
+ * 
+ *      double bounds[] = new double[] { 1, 2, 4, 8, 16 };
+ *
+ *      // For the given set of bounds:
+ *      //    first bucket      [-inf, 1)
+ *      //    second bucket     [1, 2)
+ *      //    ...
+ *      //    last bucket       [16, +inf)
+ *
+ *
+ */ +public interface ImmutableBuckets { + + /** + * Gets corresponding bucket lower bound + */ + double getValueLowerBoundFor(int bucketIndex); + + /** + * Gets corresponding bucket upper bound + */ + double getValueUpperBoundFor(int bucketIndex); + + /** + * Gets corresponding bucket lower bound + */ + Duration getDurationLowerBoundFor(int bucketIndex); + + /** + * Gets corresponding bucket upper bound + */ + Duration getDurationUpperBoundFor(int bucketIndex); + + /** + * Gets index of the corresponding bucket this value would fall under + */ + int getBucketIndexFor(double value); + + /** + * Gets index of the corresponding bucket this value would fall under + */ + int getBucketIndexFor(Duration value); + + /** + * Returns defined buckets' upper-bound values as {@link Double}s. + * @return an immutable list of {@code double}s representing these buckets + */ + List getValueUpperBounds(); + + /** + * Returns defined buckets' upper-bound values as {@link Duration}s. + * @return an immutable list of {@link Duration}s representing these buckets + */ + List getDurationUpperBounds(); +} diff --git a/core/src/main/java/com/uber/m3/tally/ScopeImpl.java b/core/src/main/java/com/uber/m3/tally/ScopeImpl.java index 3601581..7211040 100644 --- a/core/src/main/java/com/uber/m3/tally/ScopeImpl.java +++ b/core/src/main/java/com/uber/m3/tally/ScopeImpl.java @@ -25,6 +25,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentLinkedQueue; @@ -89,7 +90,14 @@ public Timer timer(String name) { public Histogram histogram(String name, Buckets buckets) { return histograms.computeIfAbsent(name, ignored -> // NOTE: This will called at most once - new HistogramImpl(this, fullyQualifiedName(name), tags, buckets)); + new HistogramImpl( + this, + fullyQualifiedName(name), + tags, + Optional.ofNullable(buckets) + .orElse(defaultBuckets) + ) + ); } @Override diff --git a/core/src/main/java/com/uber/m3/tally/ValueBuckets.java b/core/src/main/java/com/uber/m3/tally/ValueBuckets.java index 387a56a..7d944fd 100644 --- a/core/src/main/java/com/uber/m3/tally/ValueBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/ValueBuckets.java @@ -22,6 +22,9 @@ import com.uber.m3.util.Duration; +import java.util.Collections; +import java.util.List; + /** * {@link Buckets} implementation backed by {@code Double} values. */ @@ -30,15 +33,59 @@ public ValueBuckets(Double[] values) { super(values); } - public ValueBuckets() { - super(); + @Override + public double getValueLowerBoundFor(int bucketIndex) { + return bucketIndex == 0 ? Double.MIN_VALUE : buckets.get(bucketIndex - 1); + } + + @Override + public double getValueUpperBoundFor(int bucketIndex) { + return bucketIndex < buckets.size() ? buckets.get(bucketIndex) : Double.MAX_VALUE; + } + + @Override + public int getBucketIndexFor(double value) { + return HistogramImpl.toBucketIndex(Collections.binarySearch(buckets, value)); + } + + @Override + public List getValueUpperBounds() { + return Collections.unmodifiableList(buckets); + } + + @Override + public Duration getDurationLowerBoundFor(int bucketIndex) { + throw new UnsupportedOperationException("not supported"); } + @Override + public Duration getDurationUpperBoundFor(int bucketIndex) { + throw new UnsupportedOperationException("not supported"); + } + + @Override + public int getBucketIndexFor(Duration value) { + throw new UnsupportedOperationException("not supported"); + } + + @Override + public List getDurationUpperBounds() { + throw new UnsupportedOperationException("not supported"); + } + + /** + * @deprecated DO NOT USE + */ + @Deprecated @Override public Double[] asValues() { return buckets.toArray(new Double[buckets.size()]); } + /** + * @deprecated DO NOT USE + */ + @Deprecated @Override public Duration[] asDurations() { Duration[] durations = new Duration[buckets.size()]; diff --git a/core/src/main/java/com/uber/m3/util/ImmutableList.java b/core/src/main/java/com/uber/m3/util/ImmutableList.java index eed1e10..ad0c1ad 100644 --- a/core/src/main/java/com/uber/m3/util/ImmutableList.java +++ b/core/src/main/java/com/uber/m3/util/ImmutableList.java @@ -169,4 +169,9 @@ public boolean equals(Object other) { public int hashCode() { return collection.hashCode(); } + + @Override + public String toString() { + return collection.toString(); + } } diff --git a/core/src/test/java/com/uber/m3/tally/DurationBucketsTest.java b/core/src/test/java/com/uber/m3/tally/DurationBucketsTest.java index 1973063..99a760d 100644 --- a/core/src/test/java/com/uber/m3/tally/DurationBucketsTest.java +++ b/core/src/test/java/com/uber/m3/tally/DurationBucketsTest.java @@ -20,21 +20,17 @@ package com.uber.m3.tally; -import java.util.ArrayList; -import java.util.Iterator; -import java.util.List; -import java.util.ListIterator; - +import com.uber.m3.util.Duration; import org.hamcrest.CoreMatchers; import org.junit.Test; -import com.uber.m3.util.Duration; +import java.util.Iterator; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; public class DurationBucketsTest { @Test @@ -49,80 +45,6 @@ public void durationBuckets() { assertEquals(Duration.ofMillis(100 + 10 * i), iter.next()); } } - - buckets.add(Duration.ofMillis(160)); - assertEquals(Duration.ofMillis(160), buckets.get(6)); - - buckets.set(6, Duration.ofMillis(170)); - assertEquals(Duration.ofMillis(170), buckets.get(6)); - - buckets.add(6, Duration.ofMillis(160)); - assertEquals(Duration.ofMillis(160), buckets.get(6)); - - ArrayList additionalDurations = new ArrayList<>(); - additionalDurations.add(Duration.ofMillis(200)); - additionalDurations.add(Duration.ofMillis(210)); - buckets.addAll(additionalDurations); - - additionalDurations = new ArrayList<>(); - additionalDurations.add(Duration.ofMillis(180)); - additionalDurations.add(Duration.ofMillis(190)); - buckets.addAll(8, additionalDurations); - - buckets.add(Duration.ofMillis(220)); - buckets.add(Duration.ofMillis(220)); - - assertEquals(12, buckets.indexOf(Duration.ofMillis(220))); - assertEquals(13, buckets.lastIndexOf(Duration.ofMillis(220))); - - buckets.remove(12); - assertEquals(13, buckets.size()); - assertEquals(Duration.ofMillis(220), buckets.get(12)); - buckets.remove(12); - assertEquals(12, buckets.size()); - - ListIterator iter = buckets.listIterator(11); - assertTrue(iter.hasNext()); - assertTrue(iter.hasPrevious()); - assertEquals(Duration.ofMillis(210), iter.next()); - assertFalse(iter.hasNext()); - - assertTrue(buckets.containsAll(additionalDurations)); - - List durationList = buckets.subList(0, 2); - assertEquals(2, durationList.size()); - assertEquals(Duration.ofMillis(100), buckets.get(0)); - assertEquals(Duration.ofMillis(110), buckets.get(1)); - - assertTrue(buckets.retainAll(durationList)); - assertEquals(2, buckets.size()); - - assertArrayEquals(new Duration[]{Duration.ofMillis(100), Duration.ofMillis(110)}, buckets.toArray()); - - durationList = new ArrayList<>(); - durationList.add(Duration.ofMillis(100)); - assertTrue(buckets.removeAll(durationList)); - assertEquals(1, buckets.size()); - assertEquals(Duration.ofMillis(110), buckets.get(0)); - - Duration[] durationArr = new Duration[1]; - assertArrayEquals(durationArr, buckets.toArray(durationArr)); - - assertFalse(buckets.remove(Duration.ofMillis(999))); - assertTrue(buckets.remove(Duration.ofMillis(110))); - - assertEquals(0, buckets.size()); - - durationList.add(Duration.ofMillis(110)); - durationList.add(Duration.ofMillis(120)); - durationList.add(Duration.ofMillis(130)); - durationList.add(Duration.ofMillis(140)); - buckets.addAll(durationList); - - assertEquals(5, buckets.size()); - - buckets.clear(); - assertEquals(0, buckets.size()); } @Test @@ -250,9 +172,6 @@ public void testToString() { DurationBuckets buckets = DurationBuckets.linear(Duration.ZERO, Duration.ofMillis(10), 6); assertEquals("[0s, 10ms, 20ms, 30ms, 40ms, 50ms]", buckets.toString()); - buckets = new DurationBuckets(); - assertEquals("[]", buckets.toString()); - buckets = new DurationBuckets(new Duration[]{Duration.ofHours(1)}); assertEquals("[1h]", buckets.toString()); } diff --git a/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java b/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java index f8e49a5..d303fac 100644 --- a/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java @@ -21,17 +21,28 @@ package com.uber.m3.tally; import com.uber.m3.util.Duration; +import com.uber.m3.util.ImmutableMap; import org.junit.Before; import org.junit.Test; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; public class HistogramImplTest { + + private static final double[] BUCKETS = new double[] { + 1.0, 5.0, 10.0, 15.0, 20.0, 25.0, 30.0, 40.0, 50.0, 60.0, 70.0, 80.0, 90.0, 100.0, 125.0, 150.0, 175.0, 200.0, + 225.0, 250.0, 300.0, 350.0, 400.0, 450.0, 500.0, 550.0, 600.0, 650.0, 700.0, 750.0, 800.0, 850.0, 900.0, 950.0, 1000.0 + }; + private TestStatsReporter reporter; private ScopeImpl scope; @@ -46,6 +57,46 @@ public void setUp() { .build(); } + @Test + public void test() { + histogram = new HistogramImpl(scope, "histogram", ImmutableMap.EMPTY, ValueBuckets.custom(BUCKETS)); + + double maxUpperBound = BUCKETS[BUCKETS.length - 1]; + + List values = + IntStream.range(0, 10000) + .mapToDouble(ignored -> Math.random() * (maxUpperBound + 1)) + .mapToObj(a -> a) + .collect(Collectors.toList()); + + Map expected = new HashMap<>(); + + for (int i = 0; i < values.size(); ++i) { + int index = Arrays.binarySearch(BUCKETS, values.get(i)); + double upper; + if (index >= 0) { + upper = index + 1 < BUCKETS.length ? BUCKETS[index + 1] : Double.MAX_VALUE; + } else { + upper = ~index < BUCKETS.length ? BUCKETS[~index] : Double.MAX_VALUE; + } + + expected.put(upper, expected.getOrDefault(upper, 0L) + 1); + + //////////////////////////////////////////////////// + + histogram.recordValue(values.get(i)); + + if (i % 13 == 0) { + scope.report(reporter); + } + + } + + scope.report(reporter); + + assertEquals(reporter.getCumulativeValueSamples(), expected); + } + @Test public void recordValue() { Buckets buckets = ValueBuckets.linear(0, 10, 10); diff --git a/core/src/test/java/com/uber/m3/tally/ScopeImplTest.java b/core/src/test/java/com/uber/m3/tally/ScopeImplTest.java index 89de860..dd413a8 100644 --- a/core/src/test/java/com/uber/m3/tally/ScopeImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/ScopeImplTest.java @@ -76,7 +76,14 @@ public void metricCreation() { ); assertNotNull(histogram); + Histogram histogramDefaultBuckets = scope.histogram( + "new-histogram-default-buckets", + null + ); + assertNotNull(histogramDefaultBuckets); + Histogram sameHistogram = scope.histogram("new-histogram", null); + // Should be the same Histogram object and not a new instance assertTrue(histogram == sameHistogram); } diff --git a/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java b/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java index 86353e9..05d6a32 100644 --- a/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java +++ b/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java @@ -35,6 +35,8 @@ public class TestStatsReporter implements StatsReporter { private Map valueSamples = new HashMap<>(); private Map durationSamples = new HashMap<>(); + private Map cumulativeValueSamples = new HashMap<>(); + @Override public void reportCounter(String name, Map tags, long value) { counters.add(new MetricStruct<>(name, tags, value)); @@ -83,6 +85,10 @@ public void reportHistogramValueSamples( long samples ) { valueSamples.put(bucketUpperBound, samples); + cumulativeValueSamples.put( + bucketUpperBound, + cumulativeValueSamples.getOrDefault(bucketUpperBound, 0L) + samples + ); this.buckets = buckets; } @@ -122,6 +128,10 @@ public Map getValueSamples() { return valueSamples; } + public Map getCumulativeValueSamples() { + return cumulativeValueSamples; + } + public Buckets getBuckets() { return buckets; } diff --git a/core/src/test/java/com/uber/m3/tally/ValueBucketsTest.java b/core/src/test/java/com/uber/m3/tally/ValueBucketsTest.java index 27c9f7b..9f1bac9 100644 --- a/core/src/test/java/com/uber/m3/tally/ValueBucketsTest.java +++ b/core/src/test/java/com/uber/m3/tally/ValueBucketsTest.java @@ -142,9 +142,6 @@ public void testToString() { ValueBuckets buckets = ValueBuckets.linear(0, 10, 6); assertEquals("[0.0, 10.0, 20.0, 30.0, 40.0, 50.0]", buckets.toString()); - buckets = new ValueBuckets(); - assertEquals("[]", buckets.toString()); - buckets = new ValueBuckets(new Double[]{99.99}); assertEquals("[99.99]", buckets.toString()); } diff --git a/m3/src/main/java/com/uber/m3/tally/m3/M3Reporter.java b/m3/src/main/java/com/uber/m3/tally/m3/M3Reporter.java index 174c7c0..8b86edf 100644 --- a/m3/src/main/java/com/uber/m3/tally/m3/M3Reporter.java +++ b/m3/src/main/java/com/uber/m3/tally/m3/M3Reporter.java @@ -20,12 +20,12 @@ package com.uber.m3.tally.m3; -import com.uber.m3.tally.BucketPair; -import com.uber.m3.tally.BucketPairImpl; import com.uber.m3.tally.Buckets; import com.uber.m3.tally.Capabilities; import com.uber.m3.tally.CapableOf; +import com.uber.m3.tally.DurationBuckets; import com.uber.m3.tally.StatsReporter; +import com.uber.m3.tally.ValueBuckets; import com.uber.m3.tally.m3.thrift.TCalcTransport; import com.uber.m3.tally.m3.thrift.TMultiUdpClient; import com.uber.m3.tally.m3.thrift.TUdpClient; @@ -58,7 +58,6 @@ import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -113,8 +112,8 @@ public class M3Reporter implements StatsReporter, AutoCloseable { private final int payloadCapacity; - private final String bucketIdTagName; - private final String bucketTagName; + private final String bucketIdTagKey; + private final String bucketValueTagKey; private final String bucketValFmt; private final Set commonTags; @@ -140,8 +139,8 @@ private M3Reporter(Builder builder) { maxBufferingDelay = Duration.ofMillis(builder.maxProcessorWaitUntilFlushMillis); - bucketIdTagName = builder.histogramBucketIdName; - bucketTagName = builder.histogramBucketName; + bucketIdTagKey = builder.histogramBucketIdName; + bucketValueTagKey = builder.histogramBucketName; bucketValFmt = String.format("%%.%df", builder.histogramBucketTagPrecision); metricQueue = new LinkedBlockingQueue<>(builder.maxQueueSize); @@ -317,61 +316,47 @@ public void reportTimer(String name, Map tags, Duration interval queueSizedMetric(new SizedMetric(metric, PAYLOAD_SIZE_ESTIMATOR.get().evaluateByteSize(metric))); } + /** + * @deprecated DO NOT USE + * + * Please use {@link #reportHistogramValueSamples(String, Map, Buckets, int, long)} instead + */ + @Deprecated @Override public void reportHistogramValueSamples( - String name, - Map tags, - Buckets buckets, - double bucketLowerBound, - double bucketUpperBound, - long samples + String name, + Map tags, + Buckets buckets, + double bucketLowerBound, + double bucketUpperBound, + long samples ) { - // Append histogram bucket-specific tags - int bucketIdLen = String.valueOf(buckets.size()).length(); - bucketIdLen = Math.max(bucketIdLen, MIN_METRIC_BUCKET_ID_TAG_LENGTH); - - String bucketIdFmt = String.format("%%0%sd", bucketIdLen); - - BucketPair[] bucketPairs = BucketPairImpl.bucketPairs(buckets); - - if (tags == null) { - // We know that the HashMap will only contain two items at this point, - // therefore initialCapacity of 2 and loadFactor of 1. - tags = new HashMap<>(2, 1); - } else { - // Copy over the map since it might be unmodifiable and, even if it's - // not, we don't want to modify it. - tags = new HashMap<>(tags); - } - - for (int i = 0; i < bucketPairs.length; i++) { - // Look for the first pair with an upper bound greater than or equal - // to the given upper bound. - if (bucketPairs[i].upperBoundValue() >= bucketUpperBound) { - String idTagValue = String.format(bucketIdFmt, i); - - tags.put(bucketIdTagName, idTagValue); - tags.put(bucketTagName, - String.format("%s-%s", - valueBucketString(bucketPairs[i].lowerBoundValue()), - valueBucketString(bucketPairs[i].upperBoundValue()) - ) - ); - - break; - } - } - - reportCounterInternal(name, tags, samples); + reportHistogramValueSamples(name, tags, buckets, buckets.getBucketIndexFor(bucketLowerBound), samples); } + /** + * @deprecated DO NOT USE + * + * Please use {@link #reportHistogramValueSamples(String, Map, Buckets, int, long)} instead + */ @Override + @Deprecated public void reportHistogramDurationSamples( + String name, + Map tags, + Buckets buckets, + Duration bucketLowerBound, + Duration bucketUpperBound, + long samples + ) { + reportHistogramValueSamples(name, tags, buckets, buckets.getBucketIndexFor(bucketLowerBound), samples); + } + + public void reportHistogramValueSamples( String name, Map tags, Buckets buckets, - Duration bucketLowerBound, - Duration bucketUpperBound, + int bucketIndex, long samples ) { // Append histogram bucket-specific tags @@ -380,37 +365,34 @@ public void reportHistogramDurationSamples( String bucketIdFmt = String.format("%%0%sd", bucketIdLen); - BucketPair[] bucketPairs = BucketPairImpl.bucketPairs(buckets); + ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); - if (tags == null) { - // We know that the HashMap will only contain two items at this point, - // therefore initialCapacity of 2 and loadFactor of 1. - tags = new HashMap<>(2, 1); - } else { - // Copy over the map since it might be unmodifiable and, even if it's - // not, we don't want to modify it. - tags = new HashMap<>(tags); + if (tags != null) { + builder.putAll(tags); } - for (int i = 0; i < bucketPairs.length; i++) { - // Look for the first pair with an upper bound greater than or equal - // to the given upper bound. - if (bucketPairs[i].upperBoundDuration().compareTo(bucketUpperBound) >= 0) { - String idTagValue = String.format(bucketIdFmt, i); - - tags.put(bucketIdTagName, idTagValue); - tags.put(bucketTagName, + String bucketValueTag; + if (buckets instanceof ValueBuckets) { + bucketValueTag = String.format("%s-%s", - durationBucketString(bucketPairs[i].lowerBoundDuration()), - durationBucketString(bucketPairs[i].upperBoundDuration()) - ) - ); - - break; - } + valueBucketString(buckets.getValueLowerBoundFor(bucketIndex)), + valueBucketString(buckets.getValueUpperBoundFor(bucketIndex)) + ); + } else if (buckets instanceof DurationBuckets) { + bucketValueTag = + String.format("%s-%s", + durationBucketString(buckets.getDurationLowerBoundFor(bucketIndex)), + durationBucketString(buckets.getDurationUpperBoundFor(bucketIndex)) + ); + } else { + throw new IllegalArgumentException("unsupported buckets format"); } - reportCounterInternal(name, tags, samples); + builder + .put(bucketIdTagKey, String.format(bucketIdFmt, bucketIndex)) + .put(bucketValueTagKey, bucketValueTag); + + reportCounterInternal(name, builder.build(), samples); } // Relies on the calling function to provide guarantees of the reporter being open diff --git a/m3/src/test/java/com/uber/m3/tally/m3/M3ReporterTest.java b/m3/src/test/java/com/uber/m3/tally/m3/M3ReporterTest.java index 14d616e..9223920 100644 --- a/m3/src/test/java/com/uber/m3/tally/m3/M3ReporterTest.java +++ b/m3/src/test/java/com/uber/m3/tally/m3/M3ReporterTest.java @@ -43,6 +43,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -383,9 +384,13 @@ public void reporterHistogramValues() throws InterruptedException { expectedTags.put("foo", "bar"); expectedTags.put("bucketid", "0001"); expectedTags.put("bucket", "0.000000-25000000.000000"); - for (MetricTag tag : metric.getTags()) { - assertEquals(expectedTags.get(tag.getTagName()), tag.getTagValue()); - } + + Map receivedTags = + metric.getTags() + .stream() + .collect(Collectors.toMap(MetricTag::getTagName, MetricTag::getTagValue)); + + assertEquals(expectedTags, receivedTags); assertTrue(metric.isSetMetricValue());