From 2d37e47cc9381a9b62b2c8595e3491882531c671 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Wed, 15 Jul 2020 17:59:14 -0700 Subject: [PATCH 01/13] Simplify --- .../java/com/uber/m3/tally/ScopeImpl.java | 68 ++----------------- 1 file changed, 4 insertions(+), 64 deletions(-) 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 fe42e82..d15ed31 100644 --- a/core/src/main/java/com/uber/m3/tally/ScopeImpl.java +++ b/core/src/main/java/com/uber/m3/tally/ScopeImpl.java @@ -65,82 +65,22 @@ class ScopeImpl implements Scope { @Override public Counter counter(String name) { - CounterImpl counter = counters.get(name); - - if (counter != null) { - return counter; - } - - synchronized (counters) { - if (!counters.containsKey(name)) { - counters.put(name, new CounterImpl()); - } - - counter = counters.get(name); - } - - return counter; + return counters.computeIfAbsent(name, ignored -> new CounterImpl()); } @Override public Gauge gauge(String name) { - GaugeImpl gauge = gauges.get(name); - - if (gauge != null) { - return gauge; - } - - synchronized (gauges) { - if (!gauges.containsKey(name)) { - gauges.put(name, new GaugeImpl()); - } - - gauge = gauges.get(name); - } - - return gauge; + return gauges.computeIfAbsent(name, ignored -> new GaugeImpl()); } @Override public Timer timer(String name) { - TimerImpl timer = timers.get(name); - - if (timer != null) { - return timer; - } - - synchronized (timers) { - if (!timers.containsKey(name)) { - timers.put(name, new TimerImpl(fullyQualifiedName(name), tags, reporter)); - } - - timer = timers.get(name); - } - - return timer; + return timers.computeIfAbsent(name, ignored -> new TimerImpl(fullyQualifiedName(name), tags, reporter)); } @Override public Histogram histogram(String name, Buckets buckets) { - if (buckets == null) { - buckets = defaultBuckets; - } - - HistogramImpl histogram = histograms.get(name); - - if (histogram != null) { - return histogram; - } - - synchronized (histograms) { - if (!histograms.containsKey(name)) { - histograms.put(name, new HistogramImpl(fullyQualifiedName(name), tags, reporter, buckets)); - } - - histogram = histograms.get(name); - } - - return histogram; + return histograms.computeIfAbsent(name, ignored -> new HistogramImpl(fullyQualifiedName(name), tags, reporter, buckets)); } @Override From d05a4a62f8971677c1d679e125b392d133248d07 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Wed, 15 Jul 2020 19:06:10 -0700 Subject: [PATCH 02/13] Introduced `Metric`/`MetricBase` to persist metrics' fqn --- .../main/java/com/uber/m3/tally/Counter.java | 2 +- .../java/com/uber/m3/tally/CounterImpl.java | 11 ++++++++++- .../main/java/com/uber/m3/tally/Gauge.java | 2 +- .../java/com/uber/m3/tally/GaugeImpl.java | 11 ++++++++++- .../java/com/uber/m3/tally/Histogram.java | 2 +- .../java/com/uber/m3/tally/HistogramImpl.java | 19 ++++++++++--------- .../main/java/com/uber/m3/tally/Metric.java | 10 ++++++++++ .../java/com/uber/m3/tally/MetricBase.java | 14 ++++++++++++++ .../com/uber/m3/tally/HistogramImplTest.java | 6 +++--- 9 files changed, 60 insertions(+), 17 deletions(-) create mode 100644 core/src/main/java/com/uber/m3/tally/Metric.java create mode 100644 core/src/main/java/com/uber/m3/tally/MetricBase.java diff --git a/core/src/main/java/com/uber/m3/tally/Counter.java b/core/src/main/java/com/uber/m3/tally/Counter.java index 851c535..7b438bd 100644 --- a/core/src/main/java/com/uber/m3/tally/Counter.java +++ b/core/src/main/java/com/uber/m3/tally/Counter.java @@ -23,7 +23,7 @@ /** * A counter metric. */ -public interface Counter { +public interface Counter extends Metric { /** * Increment this {@link Counter} by delta. * @param delta amount to increment this {@link Counter} by diff --git a/core/src/main/java/com/uber/m3/tally/CounterImpl.java b/core/src/main/java/com/uber/m3/tally/CounterImpl.java index bb56e96..d40fcb5 100644 --- a/core/src/main/java/com/uber/m3/tally/CounterImpl.java +++ b/core/src/main/java/com/uber/m3/tally/CounterImpl.java @@ -27,15 +27,24 @@ /** * Default implementation of a {@link Counter}. */ -class CounterImpl implements Counter { +class CounterImpl extends MetricBase implements Counter { private AtomicLong prev = new AtomicLong(0); private AtomicLong curr = new AtomicLong(0); + protected CounterImpl(String fqn) { + super(fqn); + } + @Override public void inc(long delta) { curr.getAndAdd(delta); } + @Override + public String getQualifiedName() { + return super.getQualifiedName(); + } + long value() { long current = curr.get(); long previous = prev.get(); diff --git a/core/src/main/java/com/uber/m3/tally/Gauge.java b/core/src/main/java/com/uber/m3/tally/Gauge.java index 2034d4b..eab08d7 100644 --- a/core/src/main/java/com/uber/m3/tally/Gauge.java +++ b/core/src/main/java/com/uber/m3/tally/Gauge.java @@ -23,7 +23,7 @@ /** * A gauge metric. */ -public interface Gauge { +public interface Gauge extends Metric { /** * Update this {@link Gauge} to a value. * @param value value to update this {@link Gauge} to diff --git a/core/src/main/java/com/uber/m3/tally/GaugeImpl.java b/core/src/main/java/com/uber/m3/tally/GaugeImpl.java index e4b75de..cc8025f 100644 --- a/core/src/main/java/com/uber/m3/tally/GaugeImpl.java +++ b/core/src/main/java/com/uber/m3/tally/GaugeImpl.java @@ -28,16 +28,25 @@ /** * Default implementation of a {@link Gauge}. */ -class GaugeImpl implements Gauge { +class GaugeImpl extends MetricBase implements Gauge { private AtomicBoolean updated = new AtomicBoolean(false); private AtomicLong curr = new AtomicLong(0); + protected GaugeImpl(String fqn) { + super(fqn); + } + @Override public void update(double value) { curr.set(Double.doubleToLongBits(value)); updated.set(true); } + @Override + public String getQualifiedName() { + return super.getQualifiedName(); + } + double value() { return Double.longBitsToDouble(curr.get()); } diff --git a/core/src/main/java/com/uber/m3/tally/Histogram.java b/core/src/main/java/com/uber/m3/tally/Histogram.java index 625c9f8..baafcda 100644 --- a/core/src/main/java/com/uber/m3/tally/Histogram.java +++ b/core/src/main/java/com/uber/m3/tally/Histogram.java @@ -25,7 +25,7 @@ /** * A histogram metric. */ -public interface Histogram { +public interface Histogram extends Metric { /** * Record using a {@code double}. * @param value value to record 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 e559733..863b37e 100644 --- a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java +++ b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java @@ -32,9 +32,8 @@ /** * Default implementation of a {@link Histogram}. */ -class HistogramImpl implements Histogram, StopwatchRecorder { +class HistogramImpl extends MetricBase implements Histogram, StopwatchRecorder { private Type type; - private String name; private ImmutableMap tags; private Buckets specification; private List buckets; @@ -42,11 +41,13 @@ class HistogramImpl implements Histogram, StopwatchRecorder { private List lookupByDuration; HistogramImpl( - String name, + String fqn, ImmutableMap tags, StatsReporter reporter, Buckets buckets ) { + super(fqn); + if (buckets instanceof DurationBuckets) { type = Type.DURATION; } else { @@ -56,7 +57,6 @@ class HistogramImpl implements Histogram, StopwatchRecorder { BucketPair[] pairs = BucketPairImpl.bucketPairs(buckets); int pairsLen = pairs.length; - this.name = name; this.tags = tags; specification = buckets; this.buckets = new ArrayList<>(pairsLen); @@ -122,10 +122,6 @@ public Stopwatch start() { return new Stopwatch(System.nanoTime(), this); } - String getName() { - return name; - } - ImmutableMap getTags() { return tags; } @@ -186,6 +182,11 @@ public void recordStopwatch(long stopwatchStart) { recordDuration(Duration.between(stopwatchStart, System.nanoTime())); } + @Override + public String getQualifiedName() { + return super.getQualifiedName(); + } + class HistogramBucket { CounterImpl samples; double valueLowerBound; @@ -199,7 +200,7 @@ class HistogramBucket { Duration durationLowerBound, Duration durationUpperBound ) { - samples = new CounterImpl(); + samples = new CounterImpl(getQualifiedName()); this.valueLowerBound = valueLowerBound; this.valueUpperBound = valueUpperBound; this.durationLowerBound = durationLowerBound; diff --git a/core/src/main/java/com/uber/m3/tally/Metric.java b/core/src/main/java/com/uber/m3/tally/Metric.java new file mode 100644 index 0000000..3413f1d --- /dev/null +++ b/core/src/main/java/com/uber/m3/tally/Metric.java @@ -0,0 +1,10 @@ +package com.uber.m3.tally; + +public interface Metric { + + /** + * Returns metric's fully-qualified name + */ + String getQualifiedName(); + +} diff --git a/core/src/main/java/com/uber/m3/tally/MetricBase.java b/core/src/main/java/com/uber/m3/tally/MetricBase.java new file mode 100644 index 0000000..5cc9a49 --- /dev/null +++ b/core/src/main/java/com/uber/m3/tally/MetricBase.java @@ -0,0 +1,14 @@ +package com.uber.m3.tally; + +public abstract class MetricBase { + + private final String fullyQualifiedName; + + protected MetricBase(String fqn) { + this.fullyQualifiedName = fqn; + } + + String getQualifiedName() { + return fullyQualifiedName; + } +} 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 2f633e9..9b9c0b5 100644 --- a/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java @@ -57,7 +57,7 @@ public void recordValue() { histogram.recordValue(50 + Math.random() * 10); } - histogram.report(histogram.getName(), histogram.getTags(), reporter); + histogram.report(histogram.getQualifiedName(), histogram.getTags(), reporter); assertEquals(new Long(3L), reporter.getValueSamples().get(10d)); assertEquals(new Long(5L), reporter.getValueSamples().get(60d)); @@ -83,7 +83,7 @@ public void recordDuration() { histogram.recordDuration(Duration.ofMillis(50).add(Duration.ofMillis(Math.random() * 10))); } - histogram.report(histogram.getName(), histogram.getTags(), reporter); + histogram.report(histogram.getQualifiedName(), histogram.getTags(), reporter); assertEquals(new Long(3L), reporter.getDurationSamples().get(Duration.ofMillis(10))); assertEquals(new Long(5L), reporter.getDurationSamples().get(Duration.ofMillis(60))); @@ -105,7 +105,7 @@ public void recordStopwatch() { assertNotNull(stopwatch); stopwatch.stop(); - histogram.report(histogram.getName(), histogram.getTags(), reporter); + histogram.report(histogram.getQualifiedName(), histogram.getTags(), reporter); assertEquals(new Long(1L), reporter.getDurationSamples().get(Duration.ofMillis(10))); } From 67a11499234687a64e2d22edf3ab6cd9e7f1de96 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Wed, 15 Jul 2020 19:06:54 -0700 Subject: [PATCH 03/13] Refactored reporting procedure to use persisted fqn, and avoid string formatting on the hot-path --- .../java/com/uber/m3/tally/ScopeImpl.java | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) 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 d15ed31..5f03ebc 100644 --- a/core/src/main/java/com/uber/m3/tally/ScopeImpl.java +++ b/core/src/main/java/com/uber/m3/tally/ScopeImpl.java @@ -42,14 +42,10 @@ class ScopeImpl implements Scope { private ScheduledExecutorService scheduler; private Registry registry; - // ConcurrentHashMap nearly always allowing read operations seems like a good - // performance upside to the consequence of reporting a newly-made metric in - // the middle of looping and reporting through all metrics. Therefore, we only - // synchronize on these maps when having to allocate new metrics. - private final Map counters = new ConcurrentHashMap<>(); - private final Map gauges = new ConcurrentHashMap<>(); - private final Map timers = new ConcurrentHashMap<>(); - private final Map histograms = new ConcurrentHashMap<>(); + private final ConcurrentHashMap counters = new ConcurrentHashMap<>(); + private final ConcurrentHashMap gauges = new ConcurrentHashMap<>(); + private final ConcurrentHashMap timers = new ConcurrentHashMap<>(); + private final ConcurrentHashMap histograms = new ConcurrentHashMap<>(); // Private ScopeImpl constructor. Root scopes should be built using the RootScopeBuilder class ScopeImpl(ScheduledExecutorService scheduler, Registry registry, ScopeBuilder builder) { @@ -65,12 +61,12 @@ class ScopeImpl implements Scope { @Override public Counter counter(String name) { - return counters.computeIfAbsent(name, ignored -> new CounterImpl()); + return counters.computeIfAbsent(name, ignored -> new CounterImpl(fullyQualifiedName(name))); } @Override public Gauge gauge(String name) { - return gauges.computeIfAbsent(name, ignored -> new GaugeImpl()); + return gauges.computeIfAbsent(name, ignored -> new GaugeImpl(fullyQualifiedName(name))); } @Override @@ -123,19 +119,19 @@ public void close() { * @param reporter the reporter to report */ void report(StatsReporter reporter) { - for (Map.Entry counter : counters.entrySet()) { - counter.getValue().report(fullyQualifiedName(counter.getKey()), tags, reporter); + for (CounterImpl counter : counters.values()) { + counter.report(counter.getQualifiedName(), tags, reporter); } - for (Map.Entry gauge : gauges.entrySet()) { - gauge.getValue().report(fullyQualifiedName(gauge.getKey()), tags, reporter); + for (GaugeImpl gauge : gauges.values()) { + gauge.report(gauge.getQualifiedName(), tags, reporter); } // No operations on timers required here; they report directly to the StatsReporter // i.e. they are not buffered - for (Map.Entry histogram : histograms.entrySet()) { - histogram.getValue().report(fullyQualifiedName(histogram.getKey()), tags, reporter); + for (HistogramImpl histogram : histograms.values()) { + histogram.report(histogram.getQualifiedName(), tags, reporter); } } @@ -282,7 +278,7 @@ private Scope subScopeHelper(String prefix, Map tags) { } // One iteration of reporting this scope and all its subscopes - private void reportLoopIteration() { + void reportLoopIteration() { Collection subscopes = registry.subscopes.values(); if (reporter != null) { From 03985ef1ea1eaabd7534b63fdaee2184d59299ca Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Wed, 15 Jul 2020 19:19:16 -0700 Subject: [PATCH 04/13] Simplified hierarchy --- core/src/main/java/com/uber/m3/tally/Counter.java | 2 +- core/src/main/java/com/uber/m3/tally/Gauge.java | 2 +- core/src/main/java/com/uber/m3/tally/Histogram.java | 2 +- .../src/main/java/com/uber/m3/tally/HistogramImpl.java | 6 +----- core/src/main/java/com/uber/m3/tally/Metric.java | 10 ---------- core/src/main/java/com/uber/m3/tally/MetricBase.java | 6 +++++- 6 files changed, 9 insertions(+), 19 deletions(-) delete mode 100644 core/src/main/java/com/uber/m3/tally/Metric.java diff --git a/core/src/main/java/com/uber/m3/tally/Counter.java b/core/src/main/java/com/uber/m3/tally/Counter.java index 7b438bd..851c535 100644 --- a/core/src/main/java/com/uber/m3/tally/Counter.java +++ b/core/src/main/java/com/uber/m3/tally/Counter.java @@ -23,7 +23,7 @@ /** * A counter metric. */ -public interface Counter extends Metric { +public interface Counter { /** * Increment this {@link Counter} by delta. * @param delta amount to increment this {@link Counter} by diff --git a/core/src/main/java/com/uber/m3/tally/Gauge.java b/core/src/main/java/com/uber/m3/tally/Gauge.java index eab08d7..2034d4b 100644 --- a/core/src/main/java/com/uber/m3/tally/Gauge.java +++ b/core/src/main/java/com/uber/m3/tally/Gauge.java @@ -23,7 +23,7 @@ /** * A gauge metric. */ -public interface Gauge extends Metric { +public interface Gauge { /** * Update this {@link Gauge} to a value. * @param value value to update this {@link Gauge} to diff --git a/core/src/main/java/com/uber/m3/tally/Histogram.java b/core/src/main/java/com/uber/m3/tally/Histogram.java index baafcda..625c9f8 100644 --- a/core/src/main/java/com/uber/m3/tally/Histogram.java +++ b/core/src/main/java/com/uber/m3/tally/Histogram.java @@ -25,7 +25,7 @@ /** * A histogram metric. */ -public interface Histogram extends Metric { +public interface Histogram { /** * Record using a {@code double}. * @param value value to record 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 863b37e..ec54d2f 100644 --- a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java +++ b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java @@ -126,6 +126,7 @@ ImmutableMap getTags() { return tags; } + @Override void report(String name, ImmutableMap tags, StatsReporter reporter) { for (HistogramBucket bucket : buckets) { long samples = bucket.samples.value(); @@ -182,11 +183,6 @@ public void recordStopwatch(long stopwatchStart) { recordDuration(Duration.between(stopwatchStart, System.nanoTime())); } - @Override - public String getQualifiedName() { - return super.getQualifiedName(); - } - class HistogramBucket { CounterImpl samples; double valueLowerBound; diff --git a/core/src/main/java/com/uber/m3/tally/Metric.java b/core/src/main/java/com/uber/m3/tally/Metric.java deleted file mode 100644 index 3413f1d..0000000 --- a/core/src/main/java/com/uber/m3/tally/Metric.java +++ /dev/null @@ -1,10 +0,0 @@ -package com.uber.m3.tally; - -public interface Metric { - - /** - * Returns metric's fully-qualified name - */ - String getQualifiedName(); - -} diff --git a/core/src/main/java/com/uber/m3/tally/MetricBase.java b/core/src/main/java/com/uber/m3/tally/MetricBase.java index 5cc9a49..fec20a5 100644 --- a/core/src/main/java/com/uber/m3/tally/MetricBase.java +++ b/core/src/main/java/com/uber/m3/tally/MetricBase.java @@ -1,6 +1,8 @@ package com.uber.m3.tally; -public abstract class MetricBase { +import com.uber.m3.util.ImmutableMap; + +abstract class MetricBase { private final String fullyQualifiedName; @@ -11,4 +13,6 @@ protected MetricBase(String fqn) { String getQualifiedName() { return fullyQualifiedName; } + + abstract void report(String name, ImmutableMap tags, StatsReporter reporter); } From 1d7e3ae585800148563efd4a1ca8ae68b530cf48 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Wed, 15 Jul 2020 19:21:37 -0700 Subject: [PATCH 05/13] Traverse single queue in the reporting seq instead of multiple concurrent HMs --- .../java/com/uber/m3/tally/ScopeImpl.java | 42 +++++++++++-------- 1 file changed, 25 insertions(+), 17 deletions(-) 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 5f03ebc..9e36e80 100644 --- a/core/src/main/java/com/uber/m3/tally/ScopeImpl.java +++ b/core/src/main/java/com/uber/m3/tally/ScopeImpl.java @@ -27,6 +27,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.ScheduledExecutorService; /** @@ -44,9 +45,13 @@ class ScopeImpl implements Scope { private final ConcurrentHashMap counters = new ConcurrentHashMap<>(); private final ConcurrentHashMap gauges = new ConcurrentHashMap<>(); - private final ConcurrentHashMap timers = new ConcurrentHashMap<>(); private final ConcurrentHashMap histograms = new ConcurrentHashMap<>(); + // TODO validate if needs to be concurrent at all + private final ConcurrentLinkedQueue reportingQueue = new ConcurrentLinkedQueue<>(); + + private final ConcurrentHashMap timers = new ConcurrentHashMap<>(); + // Private ScopeImpl constructor. Root scopes should be built using the RootScopeBuilder class ScopeImpl(ScheduledExecutorService scheduler, Registry registry, ScopeBuilder builder) { this.scheduler = scheduler; @@ -61,22 +66,31 @@ class ScopeImpl implements Scope { @Override public Counter counter(String name) { - return counters.computeIfAbsent(name, ignored -> new CounterImpl(fullyQualifiedName(name))); + return counters.computeIfAbsent(name, ignored -> + // NOTE: This will called at most once + addToReportingQueue(new CounterImpl(fullyQualifiedName(name))) + ); } @Override public Gauge gauge(String name) { - return gauges.computeIfAbsent(name, ignored -> new GaugeImpl(fullyQualifiedName(name))); + return gauges.computeIfAbsent(name, ignored -> + // NOTE: This will called at most once + addToReportingQueue(new GaugeImpl(fullyQualifiedName(name)))); } @Override public Timer timer(String name) { + // Timers report directly to the {@code StatsReporter}, and therefore not added to reporting queue + // i.e. they are not buffered return timers.computeIfAbsent(name, ignored -> new TimerImpl(fullyQualifiedName(name), tags, reporter)); } @Override public Histogram histogram(String name, Buckets buckets) { - return histograms.computeIfAbsent(name, ignored -> new HistogramImpl(fullyQualifiedName(name), tags, reporter, buckets)); + return histograms.computeIfAbsent(name, ignored -> + // NOTE: This will called at most once + addToReportingQueue(new HistogramImpl(fullyQualifiedName(name), tags, reporter, buckets))); } @Override @@ -114,24 +128,18 @@ public void close() { } } + private T addToReportingQueue(T metric) { + reportingQueue.add(metric); + return metric; + } + /** * Reports using the specified reporter. * @param reporter the reporter to report */ void report(StatsReporter reporter) { - for (CounterImpl counter : counters.values()) { - counter.report(counter.getQualifiedName(), tags, reporter); - } - - for (GaugeImpl gauge : gauges.values()) { - gauge.report(gauge.getQualifiedName(), tags, reporter); - } - - // No operations on timers required here; they report directly to the StatsReporter - // i.e. they are not buffered - - for (HistogramImpl histogram : histograms.values()) { - histogram.report(histogram.getQualifiedName(), tags, reporter); + for (MetricBase metric : reportingQueue) { + metric.report(metric.getQualifiedName(), tags, reporter); } } From e603d1ced9a4eeea95785e930b1457c548c2072e Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Wed, 15 Jul 2020 19:48:46 -0700 Subject: [PATCH 06/13] Tidying up --- .../java/com/uber/m3/tally/HistogramImpl.java | 41 ++++++++----------- .../java/com/uber/m3/tally/ScopeImpl.java | 2 +- 2 files changed, 17 insertions(+), 26 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 ec54d2f..3bdffc4 100644 --- a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java +++ b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java @@ -28,6 +28,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; /** * Default implementation of a {@link Histogram}. @@ -43,7 +44,6 @@ class HistogramImpl extends MetricBase implements Histogram, StopwatchRecorder { HistogramImpl( String fqn, ImmutableMap tags, - StatsReporter reporter, Buckets buckets ) { super(fqn); @@ -58,10 +58,11 @@ class HistogramImpl extends MetricBase implements Histogram, StopwatchRecorder { int pairsLen = pairs.length; this.tags = tags; - specification = buckets; + this.specification = buckets; + this.buckets = new ArrayList<>(pairsLen); - lookupByValue = new ArrayList<>(pairsLen); - lookupByDuration = new ArrayList<>(pairsLen); + this.lookupByValue = new ArrayList<>(pairsLen); + this.lookupByDuration = new ArrayList<>(pairsLen); for (BucketPair pair : pairs) { addBucket(new HistogramBucket( @@ -81,40 +82,30 @@ private void addBucket(HistogramBucket bucket) { @Override public void recordValue(double value) { - int index = Collections.binarySearch(lookupByValue, value); - - if (index < 0) { - // binarySearch returns the index of the search key if it is contained in the list; - // otherwise, (-(insertion point) - 1). - index = -(index + 1); - } - - // binarySearch can return collections.size(), guarding against that. - // pointing to last bucket is fine in that case because it's [_,infinity). - if (index >= buckets.size()) { - index = buckets.size() - 1; - } - + int index = toBucketIndex(Collections.binarySearch(lookupByValue, value)); buckets.get(index).samples.inc(1); } @Override public void recordDuration(Duration duration) { - int index = Collections.binarySearch(lookupByDuration, duration); + int index = toBucketIndex(Collections.binarySearch(lookupByDuration, duration)); + buckets.get(index).samples.inc(1); + } - if (index < 0) { + private int toBucketIndex(int binarySearchResult) { + if (binarySearchResult < 0) { // binarySearch returns the index of the search key if it is contained in the list; // otherwise, (-(insertion point) - 1). - index = -(index + 1); + binarySearchResult = -(binarySearchResult + 1); } // binarySearch can return collections.size(), guarding against that. // pointing to last bucket is fine in that case because it's [_,infinity). - if (index >= buckets.size()) { - index = buckets.size() - 1; + if (binarySearchResult >= buckets.size()) { + binarySearchResult = buckets.size() - 1; } - buckets.get(index).samples.inc(1); + return binarySearchResult; } @Override @@ -196,7 +187,7 @@ class HistogramBucket { Duration durationLowerBound, Duration durationUpperBound ) { - samples = new CounterImpl(getQualifiedName()); + this.samples = new CounterImpl(getQualifiedName()); this.valueLowerBound = valueLowerBound; this.valueUpperBound = valueUpperBound; this.durationLowerBound = durationLowerBound; 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 9e36e80..1ef56ac 100644 --- a/core/src/main/java/com/uber/m3/tally/ScopeImpl.java +++ b/core/src/main/java/com/uber/m3/tally/ScopeImpl.java @@ -90,7 +90,7 @@ public Timer timer(String name) { public Histogram histogram(String name, Buckets buckets) { return histograms.computeIfAbsent(name, ignored -> // NOTE: This will called at most once - addToReportingQueue(new HistogramImpl(fullyQualifiedName(name), tags, reporter, buckets))); + addToReportingQueue(new HistogramImpl(fullyQualifiedName(name), tags, buckets))); } @Override From c2c830366315626c45829d5301ce51204e109f3d Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 16 Jul 2020 12:30:32 -0700 Subject: [PATCH 07/13] Added phony benchmark --- core/build.gradle | 36 +++++++ .../com/uber/m3/tally/ScopeImplBenchmark.java | 99 +++++++++++++++++++ m3/build.gradle | 14 --- 3 files changed, 135 insertions(+), 14 deletions(-) create mode 100644 core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java diff --git a/core/build.gradle b/core/build.gradle index e7630c9..8ea862c 100644 --- a/core/build.gradle +++ b/core/build.gradle @@ -19,3 +19,39 @@ // THE SOFTWARE. description = 'Interfaces and utilities to report metrics to M3' + +sourceSets { + jmh { + java.srcDirs = ['src/jmh/java'] + resources.srcDirs = ['src/jmh/resources'] + compileClasspath += sourceSets.main.runtimeClasspath + compileClasspath += sourceSets.test.runtimeClasspath + } +} + +dependencies { + jmhImplementation 'org.openjdk.jmh:jmh-core:1.23' + jmhImplementation 'org.openjdk.jmh:jmh-generator-annprocess:1.23' +} + +task jmh(type: JavaExec, dependsOn: jmhClasses) { + main = 'org.openjdk.jmh.Main' + classpath = sourceSets.jmh.compileClasspath + sourceSets.jmh.runtimeClasspath +} + +classes.finalizedBy(jmhClasses) + +//jmh { +// benchmarkMode = ['Throughput'] +// +// warmupIterations = 5 +// +// iterations = 5 +// batchSize = 1 +// +// fork = 1 +// forceGC = true +// includeTests = true +// +// duplicateClassesStrategy = 'warn' +//} diff --git a/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java new file mode 100644 index 0000000..f72d2a3 --- /dev/null +++ b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java @@ -0,0 +1,99 @@ +package com.uber.m3.tally; + +import com.uber.m3.util.Duration; +import com.uber.m3.util.ImmutableMap; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; + +import java.util.Random; +import java.util.concurrent.TimeUnit; + +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.MILLISECONDS) +@Fork(jvmArgsAppend = "-server") +public class ScopeImplBenchmark { + + private static final DurationBuckets EXPONENTIAL_BUCKETS = DurationBuckets.linear(Duration.ofMillis(1), Duration.ofMillis(10), 128); + + private static final String[] COUNTER_NAMES = { + "first-counter", + "second-counter", + "third-counter", + "fourth-counter", + "fifth-counter", + }; + + private static final String[] GAUGE_NAMES = { + "first-gauge", + "second-gauge", + "third-gauge", + "fourth-gauge", + "fifth-gauge", + }; + + private static final String[] HISTOGRAM_NAMES = { + "first-histogram", + "second-histogram", + "third-histogram", + "fourth-histogram", + "fifth-histogram", + }; + + @State(org.openjdk.jmh.annotations.Scope.Benchmark) + public static class BenchmarkState { + + private ScopeImpl scope; + + @Setup + public void setup() { + this.scope = + (ScopeImpl) new RootScopeBuilder() + .reporter(new TestStatsReporter()) + .tags( + ImmutableMap.of( + "service", "some-service", + "application", "some-application", + "instance", "some-instance" + ) + ) + .reportEvery(Duration.MAX_VALUE); + + for (String counterName : COUNTER_NAMES) { + scope.counter(counterName).inc(1); + } + + for (String gaugeName : GAUGE_NAMES) { + scope.gauge(gaugeName).update(0.); + } + + for (String histogramName : HISTOGRAM_NAMES) { + Histogram h = scope.histogram(histogramName, EXPONENTIAL_BUCKETS); + + Random r = new Random(); + + // Populate at least 20% of the buckets + int bucketsCount = EXPONENTIAL_BUCKETS.buckets.size(); + for (int i = 0; i < bucketsCount / 10; ++i) { + h.recordDuration(EXPONENTIAL_BUCKETS.buckets.get(r.nextInt(bucketsCount))); + } + } + } + + @TearDown + public void teardown() { + scope.close(); + } + + } + + @Benchmark + public void scopeReportingBenchmark(BenchmarkState state) { + state.scope.reportLoopIteration(); + } +} diff --git a/m3/build.gradle b/m3/build.gradle index 5e17944..8c0e542 100644 --- a/m3/build.gradle +++ b/m3/build.gradle @@ -20,7 +20,6 @@ plugins { id "org.jruyi.thrift" version "0.4.0" - id "me.champeau.gradle.jmh" version "0.4.3" } description = 'tally M3 reporter' @@ -42,16 +41,3 @@ compileThrift { project.hasProperty('genThrift') } } - -jmh { - benchmarkMode = ['Throughput'] - - warmupIterations = 0 - - iterations = 5 - batchSize = 1 - - fork = 1 - forceGC = true - includeTests = true -} From 3f5f3b246549cf3bcd278618bb79e9543ae07c32 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 16 Jul 2020 13:49:02 -0700 Subject: [PATCH 08/13] Adjusted benchmark params --- core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java index f72d2a3..c9538ec 100644 --- a/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java +++ b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java @@ -16,7 +16,7 @@ @BenchmarkMode(Mode.Throughput) @OutputTimeUnit(TimeUnit.MILLISECONDS) -@Fork(jvmArgsAppend = "-server") +@Fork(value = 2, jvmArgsAppend = { "-server", "-XX:+UseG1GC" }) public class ScopeImplBenchmark { private static final DurationBuckets EXPONENTIAL_BUCKETS = DurationBuckets.linear(Duration.ofMillis(1), Duration.ofMillis(10), 128); @@ -79,7 +79,7 @@ public void setup() { // Populate at least 20% of the buckets int bucketsCount = EXPONENTIAL_BUCKETS.buckets.size(); - for (int i = 0; i < bucketsCount / 10; ++i) { + for (int i = 0; i < bucketsCount / 5; ++i) { h.recordDuration(EXPONENTIAL_BUCKETS.buckets.get(r.nextInt(bucketsCount))); } } From 4f8932d9f82838949a96b6245be2dc7cebf6398e Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Wed, 15 Jul 2020 22:29:33 -0700 Subject: [PATCH 09/13] Fixed tests --- core/src/test/java/com/uber/m3/tally/CounterImplTest.java | 2 +- core/src/test/java/com/uber/m3/tally/GaugeImplTest.java | 2 +- .../test/java/com/uber/m3/tally/TestStatsReporter.java | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/core/src/test/java/com/uber/m3/tally/CounterImplTest.java b/core/src/test/java/com/uber/m3/tally/CounterImplTest.java index d5600c5..cd4c26f 100644 --- a/core/src/test/java/com/uber/m3/tally/CounterImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/CounterImplTest.java @@ -32,7 +32,7 @@ public class CounterImplTest { @Before public void setUp() { reporter = new TestStatsReporter(); - counter = new CounterImpl(); + counter = new CounterImpl("counter"); } @Test diff --git a/core/src/test/java/com/uber/m3/tally/GaugeImplTest.java b/core/src/test/java/com/uber/m3/tally/GaugeImplTest.java index cf578fe..4614069 100644 --- a/core/src/test/java/com/uber/m3/tally/GaugeImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/GaugeImplTest.java @@ -34,7 +34,7 @@ public class GaugeImplTest { @Before public void setUp() { reporter = new TestStatsReporter(); - gauge = new GaugeImpl(); + gauge = new GaugeImpl("gauge"); } @Test 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 7252987..86353e9 100644 --- a/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java +++ b/core/src/test/java/com/uber/m3/tally/TestStatsReporter.java @@ -25,12 +25,12 @@ import java.util.HashMap; import java.util.Map; import java.util.Queue; -import java.util.concurrent.LinkedBlockingDeque; +import java.util.concurrent.ConcurrentLinkedQueue; public class TestStatsReporter implements StatsReporter { - private Queue> counters = new LinkedBlockingDeque<>(); - private Queue> gauges = new LinkedBlockingDeque<>(); - private Queue> timers = new LinkedBlockingDeque<>(); + private Queue> counters = new ConcurrentLinkedQueue<>(); + private Queue> gauges = new ConcurrentLinkedQueue<>(); + private Queue> timers = new ConcurrentLinkedQueue<>(); private Buckets buckets; private Map valueSamples = new HashMap<>(); private Map durationSamples = new HashMap<>(); From b6c69173072f0ad089e717e57ed06a7ec7ddb187 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 16 Jul 2020 14:22:23 -0700 Subject: [PATCH 10/13] Fixed compilation --- core/src/test/java/com/uber/m3/tally/HistogramImplTest.java | 5 ----- 1 file changed, 5 deletions(-) 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 9b9c0b5..cacd62c 100644 --- a/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java +++ b/core/src/test/java/com/uber/m3/tally/HistogramImplTest.java @@ -45,7 +45,6 @@ public void recordValue() { histogram = new HistogramImpl( "", null, - reporter, buckets ); @@ -71,7 +70,6 @@ public void recordDuration() { histogram = new HistogramImpl( "", null, - reporter, buckets ); @@ -97,7 +95,6 @@ public void recordStopwatch() { histogram = new HistogramImpl( "", null, - reporter, buckets ); @@ -117,7 +114,6 @@ public void snapshotValues() { histogram = new HistogramImpl( "", null, - reporter, buckets ); @@ -152,7 +148,6 @@ public void snapshotDurations() { histogram = new HistogramImpl( "", null, - reporter, buckets ); From 245147430df9cd9be9135774eeeeaf3a6d24433b Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 16 Jul 2020 14:29:13 -0700 Subject: [PATCH 11/13] `lint` --- .../com/uber/m3/tally/ScopeImplBenchmark.java | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java index c9538ec..6e4d10c 100644 --- a/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java +++ b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java @@ -22,29 +22,34 @@ public class ScopeImplBenchmark { private static final DurationBuckets EXPONENTIAL_BUCKETS = DurationBuckets.linear(Duration.ofMillis(1), Duration.ofMillis(10), 128); private static final String[] COUNTER_NAMES = { - "first-counter", - "second-counter", - "third-counter", - "fourth-counter", - "fifth-counter", + "first-counter", + "second-counter", + "third-counter", + "fourth-counter", + "fifth-counter", }; private static final String[] GAUGE_NAMES = { - "first-gauge", - "second-gauge", - "third-gauge", - "fourth-gauge", - "fifth-gauge", + "first-gauge", + "second-gauge", + "third-gauge", + "fourth-gauge", + "fifth-gauge", }; private static final String[] HISTOGRAM_NAMES = { - "first-histogram", - "second-histogram", - "third-histogram", - "fourth-histogram", - "fifth-histogram", + "first-histogram", + "second-histogram", + "third-histogram", + "fourth-histogram", + "fifth-histogram", }; + @Benchmark + public void scopeReportingBenchmark(BenchmarkState state) { + state.scope.reportLoopIteration(); + } + @State(org.openjdk.jmh.annotations.Scope.Benchmark) public static class BenchmarkState { @@ -91,9 +96,4 @@ public void teardown() { } } - - @Benchmark - public void scopeReportingBenchmark(BenchmarkState state) { - state.scope.reportLoopIteration(); - } } From 64dca6cc02410a63287bda6792f782217fdbc8f3 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Thu, 16 Jul 2020 14:34:04 -0700 Subject: [PATCH 12/13] Copyright --- .../com/uber/m3/tally/ScopeImplBenchmark.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java index 6e4d10c..0e60cab 100644 --- a/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.java +++ b/core/src/jmh/java/com/uber/m3/tally/ScopeImplBenchmark.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; From a41f8a08270ce67e381bfc60c04a1e3b966e6084 Mon Sep 17 00:00:00 2001 From: Alexey Kudinkin Date: Mon, 27 Jul 2020 12:48:37 -0700 Subject: [PATCH 13/13] Tidying up --- .../java/com/uber/m3/tally/HistogramImpl.java | 1 - .../java/com/uber/m3/tally/MetricBase.java | 20 +++++++++++++++++++ 2 files changed, 20 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 3bdffc4..027a7a5 100644 --- a/core/src/main/java/com/uber/m3/tally/HistogramImpl.java +++ b/core/src/main/java/com/uber/m3/tally/HistogramImpl.java @@ -28,7 +28,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.atomic.AtomicReference; /** * Default implementation of a {@link Histogram}. diff --git a/core/src/main/java/com/uber/m3/tally/MetricBase.java b/core/src/main/java/com/uber/m3/tally/MetricBase.java index fec20a5..36956ee 100644 --- a/core/src/main/java/com/uber/m3/tally/MetricBase.java +++ b/core/src/main/java/com/uber/m3/tally/MetricBase.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.ImmutableMap;