From f7853290b9c62ce07af648d78322a3c48914f336 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 12:27:50 -0700 Subject: [PATCH 01/12] Fixed backward-compatibility breaking change; Added test --- core/src/main/java/com/uber/m3/tally/ScopeImpl.java | 10 +++++++++- .../src/test/java/com/uber/m3/tally/ScopeImplTest.java | 7 +++++++ 2 files changed, 16 insertions(+), 1 deletion(-) 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/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); } From 2004798a7c48891bd9f7bb23df83a8f135395d9a Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 13:52:52 -0700 Subject: [PATCH 02/12] Deprecated `Buckets`; Introduced `ImmutableBuckets`; --- .../com/uber/m3/tally/AbstractBuckets.java | 5 +++- .../main/java/com/uber/m3/tally/Buckets.java | 15 ++++++---- .../com/uber/m3/tally/ImmutableBuckets.java | 30 +++++++++++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) create mode 100644 core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java 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..0557fb1 100644 --- a/core/src/main/java/com/uber/m3/tally/AbstractBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/AbstractBuckets.java @@ -30,8 +30,11 @@ import java.util.ListIterator; /** - * Abstract {@link Buckets} implementation for common functionality. + * @deprecated DO NOT USE + * + * Please use {@link ImmutableBuckets} instead */ +@Deprecated public abstract class AbstractBuckets implements Buckets { protected List buckets; 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..c40f6bf 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,21 @@ 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 */ Duration[] asDurations(); + } 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..c85364a --- /dev/null +++ b/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java @@ -0,0 +1,30 @@ +package com.uber.m3.tally; + +import com.uber.m3.util.Duration; + +import java.util.List; + +public interface ImmutableBuckets { + + double getValueLowerBoundFor(int bucketIndex); + double getValueUpperBoundFor(int bucketIndex); + + Duration getDurationLowerBoundFor(int bucketIndex); + Duration getDurationUpperBoundFor(int bucketIndex); + + int getBucketIndexFor(double value); + int getBucketIndexFor(Duration value); + + /** + * Returns these buckets as {@code double}s. + * @return an immutable list of {@code double}s representing these buckets + */ + List getValueBuckets(); + + + /** + * Returns these buckets as {@link Duration}s. + * @return an immutable list of {@link Duration}s representing these buckets + */ + List getDurationBuckets(); +} From 3d86581b32decd63a05b461848bb8384f42650bc Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 13:54:08 -0700 Subject: [PATCH 03/12] Refactored `DurationBuckets`, `ValueBuckets` to implement `ImmutableBuckets` --- .../com/uber/m3/tally/DurationBuckets.java | 54 ++++++++++++++++++- .../java/com/uber/m3/tally/ValueBuckets.java | 52 ++++++++++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) 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..ec5612e 100644 --- a/core/src/main/java/com/uber/m3/tally/DurationBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/DurationBuckets.java @@ -22,10 +22,14 @@ 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); } @@ -34,17 +38,65 @@ 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 getDurationBuckets() { + 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 getValueBuckets() { + throw new UnsupportedOperationException("not supported"); + } + + /** + * @deprecated DO NOT USE + */ + @Deprecated @Override public Double[] asValues() { Double[] values = new Double[buckets.size()]; for (int i = 0; i < values.length; i++) { - values[i] = buckets.get(i).getSeconds(); + values[i] = buckets.get(i).getSeconds() * Duration.NANOS_PER_SECOND; } 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/ValueBuckets.java b/core/src/main/java/com/uber/m3/tally/ValueBuckets.java index 387a56a..478ee6a 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. */ @@ -34,11 +37,60 @@ 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 getValueBuckets() { + 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 getDurationBuckets() { + 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()]; From 1edef39e794dca36b02ed6c85303039d7b8661ff Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 13:55:27 -0700 Subject: [PATCH 04/12] Rebased `HistogramImpl` to leverage `ImmutableBuckets` interface; Added additional test; --- .../java/com/uber/m3/tally/HistogramImpl.java | 44 +++++++--------- .../com/uber/m3/tally/HistogramImplTest.java | 50 +++++++++++++++++++ .../com/uber/m3/tally/TestStatsReporter.java | 10 ++++ 3 files changed, 79 insertions(+), 25 deletions(-) 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..4e6931f 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.getValueBuckets(), value)); getOrCreateCounter(index).inc(1); } @Override public void recordDuration(Duration duration) { - int index = toBucketIndex(Arrays.binarySearch(lookupByDuration, duration)); + int index = toBucketIndex(Collections.binarySearch(specification.getDurationBuckets(), duration)); getOrCreateCounter(index).inc(1); } @@ -87,9 +79,11 @@ private CounterImpl getOrCreateCounter(int index) { return bucketCounters[index]; } + List bucketsBounds = specification.getValueBuckets(); + // 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 +95,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 +127,19 @@ ImmutableMap getTags() { } private Duration getUpperBoundDurationForBucket(int bucketIndex) { - return bucketIndex < lookupByDuration.length ? lookupByDuration[bucketIndex] : Duration.MAX_VALUE; + return bucketIndex < specification.getDurationBuckets().size() ? specification.getDurationBuckets().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.getDurationBuckets().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.getValueBuckets().size() ? specification.getValueBuckets().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.getValueBuckets().get(bucketIndex - 1); } private long getCounterValue(int index) { @@ -218,7 +212,7 @@ public void report(ImmutableMap tags, StatsReporter reporter) { reporter.reportHistogramValueSamples( getQualifiedName(), tags, - specification, + (Buckets) specification, getLowerBoundValueForBucket(bucketIndex), getUpperBoundValueForBucket(bucketIndex), inc @@ -228,7 +222,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/test/java/com/uber/m3/tally/HistogramImplTest.java b/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java index f8e49a5..1992c6a 100644 --- a/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java @@ -21,12 +21,17 @@ 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; @@ -46,6 +51,51 @@ public void setUp() { .build(); } + 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 + }; + + @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/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; } From 6e9dd713adb872a1dbf67c1533e7db0ff476c224 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 13:58:35 -0700 Subject: [PATCH 05/12] Refactored reporter to avoid unnecessary allocations --- .../com/uber/m3/tally/BucketPairImpl.java | 2 +- .../java/com/uber/m3/tally/m3/M3Reporter.java | 136 ++++++++---------- .../com/uber/m3/tally/m3/M3ReporterTest.java | 11 +- 3 files changed, 68 insertions(+), 81 deletions(-) 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..38ac845 100644 --- a/core/src/main/java/com/uber/m3/tally/BucketPairImpl.java +++ b/core/src/main/java/com/uber/m3/tally/BucketPairImpl.java @@ -27,7 +27,7 @@ /** * 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 { 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()); From 17e61fb7323636fd66722671cc1ef31bb8c6cbbd Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 14:06:14 -0700 Subject: [PATCH 06/12] Reverted fix in `DurationBuckets` to preserve backward-compatibility; --- core/src/main/java/com/uber/m3/tally/DurationBuckets.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ec5612e..5acf4e7 100644 --- a/core/src/main/java/com/uber/m3/tally/DurationBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/DurationBuckets.java @@ -87,7 +87,7 @@ public Double[] asValues() { Double[] values = new Double[buckets.size()]; for (int i = 0; i < values.length; i++) { - values[i] = buckets.get(i).getSeconds() * Duration.NANOS_PER_SECOND; + values[i] = buckets.get(i).getSeconds(); } return values; From 75d15c4e934bc8dd563571f1d943d01eccf803b3 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 14:06:36 -0700 Subject: [PATCH 07/12] Fixed buckets access --- core/src/main/java/com/uber/m3/tally/HistogramImpl.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 4e6931f..2daee35 100644 --- a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java +++ b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java @@ -79,7 +79,10 @@ private CounterImpl getOrCreateCounter(int index) { return bucketCounters[index]; } - List bucketsBounds = specification.getValueBuckets(); + List bucketsBounds = + this.type == Type.VALUE + ? specification.getValueBuckets() + : specification.getDurationBuckets(); // To maintain lock granularity we synchronize only on a // particular bucket leveraging bucket's boundary as a sync target From afdea13190eee90170614aaafc2a40b999f8c5c2 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 14:11:39 -0700 Subject: [PATCH 08/12] Tidying up --- .../main/java/com/uber/m3/tally/Buckets.java | 1 + .../com/uber/m3/tally/ImmutableBuckets.java | 21 ++++++++++++++++++- .../java/com/uber/m3/tally/ValueBuckets.java | 1 - 3 files changed, 21 insertions(+), 2 deletions(-) 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 c40f6bf..ff4d576 100644 --- a/core/src/main/java/com/uber/m3/tally/Buckets.java +++ b/core/src/main/java/com/uber/m3/tally/Buckets.java @@ -40,6 +40,7 @@ public interface Buckets extends ImmutableBuckets, List { /** * @deprecated DO NOT USE */ + @Deprecated Duration[] asDurations(); } diff --git a/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java b/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java index c85364a..7f39c2d 100644 --- a/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java @@ -1,3 +1,23 @@ +// 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; @@ -21,7 +41,6 @@ public interface ImmutableBuckets { */ List getValueBuckets(); - /** * Returns these buckets as {@link Duration}s. * @return an immutable list of {@link Duration}s representing these buckets 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 478ee6a..40c586e 100644 --- a/core/src/main/java/com/uber/m3/tally/ValueBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/ValueBuckets.java @@ -67,7 +67,6 @@ public Duration getDurationUpperBoundFor(int bucketIndex) { throw new UnsupportedOperationException("not supported"); } - @Override public int getBucketIndexFor(Duration value) { throw new UnsupportedOperationException("not supported"); From edb4b70682f429180d386a6ce45be6f3c7fdc708 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 14:16:46 -0700 Subject: [PATCH 09/12] NOTE: Introduced a breaking change making `DurationBuckets` and `ValueBuckets` immutable; --- .../com/uber/m3/tally/AbstractBuckets.java | 10 +-- .../com/uber/m3/tally/DurationBuckets.java | 4 - .../java/com/uber/m3/tally/ValueBuckets.java | 4 - .../java/com/uber/m3/util/ImmutableList.java | 5 ++ .../uber/m3/tally/DurationBucketsTest.java | 87 +------------------ .../com/uber/m3/tally/ValueBucketsTest.java | 3 - 6 files changed, 11 insertions(+), 102 deletions(-) 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 0557fb1..f105edc 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; @@ -40,14 +40,10 @@ public abstract class AbstractBuckets implements 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"); } - } - AbstractBuckets() { - this(null); + this.buckets = new ImmutableList<>(Arrays.asList(buckets)); } @Override 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 5acf4e7..6022372 100644 --- a/core/src/main/java/com/uber/m3/tally/DurationBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/DurationBuckets.java @@ -34,10 +34,6 @@ 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); 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 40c586e..4d067aa 100644 --- a/core/src/main/java/com/uber/m3/tally/ValueBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/ValueBuckets.java @@ -33,10 +33,6 @@ 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); 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/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()); } From c50e2c2f0589b23a99fafa418a058960f44c9669 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 14:33:57 -0700 Subject: [PATCH 10/12] Added a check that provided buckets are in a non-decreasing order --- .../main/java/com/uber/m3/tally/AbstractBuckets.java | 12 +++++++++++- .../main/java/com/uber/m3/tally/BucketPairImpl.java | 2 -- 2 files changed, 11 insertions(+), 3 deletions(-) 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 f105edc..d9c2db9 100644 --- a/core/src/main/java/com/uber/m3/tally/AbstractBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/AbstractBuckets.java @@ -35,7 +35,7 @@ * Please use {@link ImmutableBuckets} instead */ @Deprecated -public abstract class AbstractBuckets implements Buckets { +public abstract class AbstractBuckets> implements Buckets { protected List buckets; AbstractBuckets(T[] buckets) { @@ -43,9 +43,19 @@ public abstract class AbstractBuckets implements Buckets { throw new IllegalArgumentException("provided buckets could not be null"); } + validate(buckets); + this.buckets = new ImmutableList<>(Arrays.asList(buckets)); } + 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 public abstract Double[] asValues(); 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 38ac845..0234255 100644 --- a/core/src/main/java/com/uber/m3/tally/BucketPairImpl.java +++ b/core/src/main/java/com/uber/m3/tally/BucketPairImpl.java @@ -60,8 +60,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]; From 86b58c00de03621fcb5506654bb538f6b1df1928 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 14:42:52 -0700 Subject: [PATCH 11/12] Added java-docs; Tidying up; --- .../com/uber/m3/tally/BucketPairImpl.java | 2 - .../com/uber/m3/tally/DurationBuckets.java | 4 +- .../java/com/uber/m3/tally/HistogramImpl.java | 16 +++---- .../com/uber/m3/tally/ImmutableBuckets.java | 46 +++++++++++++++++-- .../java/com/uber/m3/tally/ValueBuckets.java | 4 +- 5 files changed, 54 insertions(+), 18 deletions(-) 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 0234255..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,8 +22,6 @@ import com.uber.m3.util.Duration; -import java.util.Collections; - /** * Default implementation of a {@link BucketPair} * 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 6022372..0ca3a48 100644 --- a/core/src/main/java/com/uber/m3/tally/DurationBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/DurationBuckets.java @@ -50,7 +50,7 @@ public int getBucketIndexFor(Duration value) { } @Override - public List getDurationBuckets() { + public List getDurationUpperBounds() { return Collections.unmodifiableList(buckets); } @@ -70,7 +70,7 @@ public int getBucketIndexFor(double value) { } @Override - public List getValueBuckets() { + public List getValueUpperBounds() { throw new UnsupportedOperationException("not supported"); } 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 2daee35..f7d76fc 100644 --- a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java +++ b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java @@ -64,13 +64,13 @@ class HistogramImpl extends MetricBase implements Histogram, StopwatchRecorder { @Override public void recordValue(double value) { - int index = toBucketIndex(Collections.binarySearch(specification.getValueBuckets(), value)); + int index = toBucketIndex(Collections.binarySearch(specification.getValueUpperBounds(), value)); getOrCreateCounter(index).inc(1); } @Override public void recordDuration(Duration duration) { - int index = toBucketIndex(Collections.binarySearch(specification.getDurationBuckets(), duration)); + int index = toBucketIndex(Collections.binarySearch(specification.getDurationUpperBounds(), duration)); getOrCreateCounter(index).inc(1); } @@ -81,8 +81,8 @@ private CounterImpl getOrCreateCounter(int index) { List bucketsBounds = this.type == Type.VALUE - ? specification.getValueBuckets() - : specification.getDurationBuckets(); + ? specification.getValueUpperBounds() + : specification.getDurationUpperBounds(); // To maintain lock granularity we synchronize only on a // particular bucket leveraging bucket's boundary as a sync target @@ -130,19 +130,19 @@ ImmutableMap getTags() { } private Duration getUpperBoundDurationForBucket(int bucketIndex) { - return bucketIndex < specification.getDurationBuckets().size() ? specification.getDurationBuckets().get(bucketIndex) : Duration.MAX_VALUE; + return bucketIndex < specification.getDurationUpperBounds().size() ? specification.getDurationUpperBounds().get(bucketIndex) : Duration.MAX_VALUE; } private Duration getLowerBoundDurationForBucket(int bucketIndex) { - return bucketIndex == 0 ? Duration.MIN_VALUE : specification.getDurationBuckets().get(bucketIndex - 1); + return bucketIndex == 0 ? Duration.MIN_VALUE : specification.getDurationUpperBounds().get(bucketIndex - 1); } private double getUpperBoundValueForBucket(int bucketIndex) { - return bucketIndex < specification.getValueBuckets().size() ? specification.getValueBuckets().get(bucketIndex) : Double.MAX_VALUE; + return bucketIndex < specification.getValueUpperBounds().size() ? specification.getValueUpperBounds().get(bucketIndex) : Double.MAX_VALUE; } private double getLowerBoundValueForBucket(int bucketIndex) { - return bucketIndex == 0 ? Double.MIN_VALUE : specification.getValueBuckets().get(bucketIndex - 1); + return bucketIndex == 0 ? Double.MIN_VALUE : specification.getValueUpperBounds().get(bucketIndex - 1); } private long getCounterValue(int index) { diff --git a/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java b/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java index 7f39c2d..69dbda5 100644 --- a/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java @@ -24,26 +24,64 @@ 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 these buckets as {@code double}s. + * Returns defined buckets' upper-bound values as {@link Double}s. * @return an immutable list of {@code double}s representing these buckets */ - List getValueBuckets(); + List getValueUpperBounds(); /** - * Returns these buckets as {@link Duration}s. + * Returns defined buckets' upper-bound values as {@link Duration}s. * @return an immutable list of {@link Duration}s representing these buckets */ - List getDurationBuckets(); + List getDurationUpperBounds(); } 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 4d067aa..7d944fd 100644 --- a/core/src/main/java/com/uber/m3/tally/ValueBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/ValueBuckets.java @@ -49,7 +49,7 @@ public int getBucketIndexFor(double value) { } @Override - public List getValueBuckets() { + public List getValueUpperBounds() { return Collections.unmodifiableList(buckets); } @@ -69,7 +69,7 @@ public int getBucketIndexFor(Duration value) { } @Override - public List getDurationBuckets() { + public List getDurationUpperBounds() { throw new UnsupportedOperationException("not supported"); } From 6151fe09f5003e3f5e1ad04e75d597ecb66e2f2b Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 30 Jul 2020 14:45:50 -0700 Subject: [PATCH 12/12] Tidying up --- .../main/java/com/uber/m3/tally/ImmutableBuckets.java | 2 +- .../java/com/uber/m3/tally/HistogramImplTest.java | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java b/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java index 69dbda5..92b7806 100644 --- a/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java +++ b/core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java @@ -38,7 +38,7 @@ * // second bucket [1, 2) * // ... * // last bucket [16, +inf) - * 
+ *
* */ public interface ImmutableBuckets { 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 1992c6a..d303fac 100644 --- a/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java @@ -37,6 +37,12 @@ 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; @@ -51,11 +57,6 @@ public void setUp() { .build(); } - 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 - }; - @Test public void test() { histogram = new HistogramImpl(scope, "histogram", ImmutableMap.EMPTY, ValueBuckets.custom(BUCKETS));