From 22beaeedbfc0f433f94abad68a47925920ccac06 Mon Sep 17 00:00:00 2001 From: beanliu Date: Fri, 22 Jan 2021 15:34:19 +1100 Subject: [PATCH 01/17] add MetricDataType.HISTOGRAM and related data container types --- .../sdk/metrics/data/DoubleHistogramData.java | 34 ++++++++ .../data/DoubleHistogramPointData.java | 78 +++++++++++++++++++ .../sdk/metrics/data/MetricData.java | 38 +++++++++ .../sdk/metrics/data/MetricDataType.java | 6 ++ .../sdk/metrics/data/MetricDataTest.java | 46 +++++++++++ 5 files changed, 202 insertions(+) create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramData.java create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramData.java new file mode 100644 index 00000000000..a5f90422faf --- /dev/null +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramData.java @@ -0,0 +1,34 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.data; + +import com.google.auto.value.AutoValue; +import java.util.Collection; +import javax.annotation.concurrent.Immutable; + +@Immutable +@AutoValue +public abstract class DoubleHistogramData implements Data { + DoubleHistogramData() {} + + public static DoubleHistogramData create( + AggregationTemporality temporality, Collection points) { + return new AutoValue_DoubleHistogramData(temporality, points); + } + + /** + * Returns the {@code AggregationTemporality} of this metric, + * + *

AggregationTemporality describes if the aggregator reports delta changes since last report + * time, or cumulative changes since a fixed start time. + * + * @return the {@code AggregationTemporality} of this metric + */ + public abstract AggregationTemporality getAggregationTemporality(); + + @Override + public abstract Collection getPoints(); +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java new file mode 100644 index 00000000000..111be05fcf4 --- /dev/null +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java @@ -0,0 +1,78 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.data; + +import com.google.auto.value.AutoValue; +import io.opentelemetry.api.common.Labels; +import java.util.List; +import java.util.function.BiConsumer; +import javax.annotation.concurrent.Immutable; + +/** + * DoubleHistogramPointData represents an approximate representation of the distribution of + * measurements. + */ +@Immutable +@AutoValue +public abstract class DoubleHistogramPointData implements PointData { + /** + * Creates a DoubleHistogramPointData. + * + * @return a DoubleHistogramPointData. + */ + public static DoubleHistogramPointData create( + long startEpochNanos, + long epochNanos, + Labels labels, + double sum, + long count, + List boundaries, + List counts) { + return new AutoValue_DoubleHistogramPointData( + startEpochNanos, epochNanos, labels, sum, count, boundaries, counts); + } + + DoubleHistogramPointData() {} + + /** + * The sum of all measurements recorded. + * + * @return the sum of recorded measurements. + */ + public abstract double getSum(); + + /** + * The number of measurements taken. + * + * @return the count of recorded measurements. + */ + public abstract long getCount(); + + /** + * The bucket boundaries. For a Histogram with N defined boundaries, e.g, [x, y, z]. There are N+1 + * counts: [-inf, x), [x, y), [y, z), [z, +inf]. + * + * @return the bucket boundaries in increasing order. + */ + public abstract List getBoundaries(); + + /** + * The counts in each bucket. + * + * @return the counts in each bucket. + */ + public abstract List getCounts(); + + /** Iterates over all the bucket boundaries and counts in this histogram. */ + public void forEach(BiConsumer action) { + List boundaries = getBoundaries(); + List counts = getCounts(); + for (int i = 0; i < boundaries.size(); ++i) { + action.accept(boundaries.get(i), counts.get(i)); + } + action.accept(Double.POSITIVE_INFINITY, counts.get(boundaries.size())); + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricData.java index d7e2939635a..61df82f5db5 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricData.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricData.java @@ -29,6 +29,8 @@ public abstract class MetricData { /* isMonotonic= */ false, AggregationTemporality.CUMULATIVE, Collections.emptyList()); private static final DoubleSummaryData DEFAULT_DOUBLE_SUMMARY_DATA = DoubleSummaryData.create(Collections.emptyList()); + private static final DoubleHistogramData DEFAULT_DOUBLE_HISTOGRAM_DATA = + DoubleHistogramData.create(AggregationTemporality.CUMULATIVE, Collections.emptyList()); /** * Returns a new MetricData wih a {@link MetricDataType#DOUBLE_GAUGE} type. @@ -140,6 +142,28 @@ public static MetricData createDoubleSummary( data); } + /** + * Returns a new MetricData with a {@link MetricDataType#HISTOGRAM} type. + * + * @return a new MetricData wih a {@link MetricDataType#HISTOGRAM} type. + */ + public static MetricData createDoubleHistogram( + Resource resource, + InstrumentationLibraryInfo instrumentationLibraryInfo, + String name, + String description, + String unit, + DoubleHistogramData data) { + return new AutoValue_MetricData( + resource, + instrumentationLibraryInfo, + name, + description, + unit, + MetricDataType.HISTOGRAM, + data); + } + MetricData() {} /** @@ -265,4 +289,18 @@ public final DoubleSummaryData getDoubleSummaryData() { } return DEFAULT_DOUBLE_SUMMARY_DATA; } + + /** + * Returns the {@code DoubleHistogramData} if type is {@link MetricDataType#HISTOGRAM}, otherwise + * a default empty data. + * + * @return the {@code DoubleHistogramData} if type is {@link MetricDataType#HISTOGRAM}, otherwise + * a default empty data. + */ + public final DoubleHistogramData getDoubleHistogramData() { + if (getType() == MetricDataType.HISTOGRAM) { + return (DoubleHistogramData) getData(); + } + return DEFAULT_DOUBLE_HISTOGRAM_DATA; + } } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricDataType.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricDataType.java index 50adc159fbd..0749807848f 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricDataType.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/MetricDataType.java @@ -30,4 +30,10 @@ public enum MetricDataType { * value recorded, the sum of all measurements and the total number of measurements recorded. */ SUMMARY, + + /** + * A Histogram represents an approximate representation of the distribution of measurements + * recorded. + */ + HISTOGRAM, } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java index b66a6e3312a..5fc164e4946 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java @@ -10,8 +10,10 @@ import io.opentelemetry.api.common.Labels; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; import io.opentelemetry.sdk.resources.Resource; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.List; import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; @@ -40,6 +42,15 @@ class MetricDataTest { Arrays.asList( ValueAtPercentile.create(0.0, DOUBLE_VALUE), ValueAtPercentile.create(100, DOUBLE_VALUE))); + private static final DoubleHistogramPointData HISTOGRAM_POINT = + DoubleHistogramPointData.create( + START_EPOCH_NANOS, + EPOCH_NANOS, + Labels.of("key", "value"), + DOUBLE_VALUE, + LONG_VALUE, + Collections.singletonList(1.0), + Arrays.asList(1L, 1L)); @Test void metricData_Getters() { @@ -146,6 +157,39 @@ void metricData_SummaryPoints() { assertThat(metricData.getDoubleSummaryData().getPoints()).containsExactly(SUMMARY_POINT); } + @Test + void metricData_HistogramPoints() { + assertThat(HISTOGRAM_POINT.getStartEpochNanos()).isEqualTo(START_EPOCH_NANOS); + assertThat(HISTOGRAM_POINT.getEpochNanos()).isEqualTo(EPOCH_NANOS); + assertThat(HISTOGRAM_POINT.getLabels().size()).isEqualTo(1); + assertThat(HISTOGRAM_POINT.getLabels().get("key")).isEqualTo("value"); + assertThat(HISTOGRAM_POINT.getCount()).isEqualTo(LONG_VALUE); + assertThat(HISTOGRAM_POINT.getSum()).isEqualTo(DOUBLE_VALUE); + assertThat(HISTOGRAM_POINT.getBoundaries()).isEqualTo(Collections.singletonList(1.0)); + assertThat(HISTOGRAM_POINT.getCounts()).isEqualTo(Arrays.asList(1L, 1L)); + + List boundaries = new ArrayList<>(); + List counts = new ArrayList<>(); + HISTOGRAM_POINT.forEach( + (b, c) -> { + boundaries.add(b); + counts.add(c); + }); + assertThat(boundaries).isEqualTo(Arrays.asList(1.0, Double.POSITIVE_INFINITY)); + assertThat(counts).isEqualTo(HISTOGRAM_POINT.getCounts()); + + MetricData metricData = + MetricData.createDoubleHistogram( + Resource.getEmpty(), + InstrumentationLibraryInfo.getEmpty(), + "metric_name", + "metric_description", + "ms", + DoubleHistogramData.create( + AggregationTemporality.DELTA, Collections.singleton(HISTOGRAM_POINT))); + assertThat(metricData.getDoubleHistogramData().getPoints()).containsExactly(HISTOGRAM_POINT); + } + @Test void metricData_GetDefault() { MetricData metricData = @@ -160,6 +204,7 @@ void metricData_GetDefault() { assertThat(metricData.getLongGaugeData().getPoints()).isEmpty(); assertThat(metricData.getDoubleSumData().getPoints()).isEmpty(); assertThat(metricData.getLongGaugeData().getPoints()).isEmpty(); + assertThat(metricData.getDoubleHistogramData().getPoints()).isEmpty(); assertThat(metricData.getDoubleSummaryData().getPoints()).containsExactly(SUMMARY_POINT); metricData = @@ -174,6 +219,7 @@ void metricData_GetDefault() { assertThat(metricData.getLongGaugeData().getPoints()).isEmpty(); assertThat(metricData.getDoubleSumData().getPoints()).isEmpty(); assertThat(metricData.getLongGaugeData().getPoints()).isEmpty(); + assertThat(metricData.getDoubleHistogramData().getPoints()).isEmpty(); assertThat(metricData.getDoubleSummaryData().getPoints()).isEmpty(); } } From ed0251b62ff04eb4d51e74f725e36a116a4a1827 Mon Sep 17 00:00:00 2001 From: beanliu Date: Tue, 26 Jan 2021 09:19:48 +1100 Subject: [PATCH 02/17] add no-op histogram support for the OTLP/Prometheus exporters --- .../opentelemetry/exporter/prometheus/MetricAdapter.java | 7 +++++++ .../opentelemetry/sdk/extension/otproto/MetricAdapter.java | 3 +++ 2 files changed, 10 insertions(+) diff --git a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/MetricAdapter.java b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/MetricAdapter.java index f0856846437..e62d601601b 100644 --- a/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/MetricAdapter.java +++ b/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/MetricAdapter.java @@ -85,6 +85,8 @@ static Collector.Type toMetricFamilyType(MetricData metricData) { return Collector.Type.GAUGE; case SUMMARY: return Collector.Type.SUMMARY; + case HISTOGRAM: + return Collector.Type.HISTOGRAM; } return Collector.Type.UNTYPED; } @@ -122,6 +124,9 @@ static List toSamples( addSummarySamples( (DoubleSummaryPointData) pointData, name, labelNames, labelValues, samples); break; + case HISTOGRAM: + // TODO: no-op, will add in the following PRs + break; } } return samples; @@ -189,6 +194,8 @@ private static Collection getPoints(MetricData metricData) return metricData.getLongSumData().getPoints(); case SUMMARY: return metricData.getDoubleSummaryData().getPoints(); + case HISTOGRAM: + return metricData.getDoubleHistogramData().getPoints(); } return Collections.emptyList(); } diff --git a/sdk-extensions/otproto/src/main/java/io/opentelemetry/sdk/extension/otproto/MetricAdapter.java b/sdk-extensions/otproto/src/main/java/io/opentelemetry/sdk/extension/otproto/MetricAdapter.java index 965bd80ad5e..3a07c28af49 100644 --- a/sdk-extensions/otproto/src/main/java/io/opentelemetry/sdk/extension/otproto/MetricAdapter.java +++ b/sdk-extensions/otproto/src/main/java/io/opentelemetry/sdk/extension/otproto/MetricAdapter.java @@ -149,6 +149,9 @@ static Metric toProtoMetric(MetricData metricData) { .addAllDataPoints(toDoubleDataPoints(doubleGaugeData.getPoints())) .build()); break; + case HISTOGRAM: + // TODO: no-op, will add in the following PRs + break; } return builder.build(); } From c5697b6f2b991488dd2f9965df361c2e7c375dde Mon Sep 17 00:00:00 2001 From: beanliu Date: Fri, 22 Jan 2021 16:43:13 +1100 Subject: [PATCH 03/17] add Histogram aggregator and accumulation --- .../sdk/metrics/aggregator/BucketSearch.java | 244 ++++++++++++++++++ .../aggregator/DoubleHistogramBenchmark.java | 74 ++++++ .../metrics/aggregator/AggregatorFactory.java | 12 + .../aggregator/DoubleHistogramAggregator.java | 168 ++++++++++++ .../aggregator/HistogramAccumulation.java | 63 +++++ .../HistogramAggregatorFactory.java | 50 ++++ .../metrics/aggregator/MetricDataUtils.java | 10 + .../aggregator/AggregatorFactoryTest.java | 70 +++++ .../DoubleHistogramAggregatorTest.java | 182 +++++++++++++ .../aggregator/HistogramAccumulationTest.java | 49 ++++ 10 files changed, 922 insertions(+) create mode 100644 sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/BucketSearch.java create mode 100644 sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java create mode 100644 sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java create mode 100644 sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/BucketSearch.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/BucketSearch.java new file mode 100644 index 00000000000..6f0b39594d7 --- /dev/null +++ b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/BucketSearch.java @@ -0,0 +1,244 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.aggregator; + +import java.util.Arrays; +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +@State(Scope.Benchmark) +public class BucketSearch { + private static final double[] arr5 = new double[] {5, 10, 25, 50, 100}; + private static final double[] arr10 = + new double[] {10, 20, 40, 80, 160, 320, 640, 1280, 2560, 5120}; + private static final double[] arrLarge = + new double[] { + 1, + 2, + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 12, + 14, + 16, + 18, + 20, + 25, + 30, + 35, + 40, + 45, + 50, + 60, + 70, + 80, + 90, + 100, + 120, + 140, + 160, + 180, + 200, + 250, + 300, + 350, + 400, + 450, + 500, + 600, + 700, + 800, + 900, + 1000, + 1200, + 1400, + 1600, + 1800, + 2000, + 2500, + 3000, + 3500, + 4000, + 4500, + 5000, + 6000, + 7000, + 8000, + 9000, + 10000, + 12000, + 14000, + 16000, + 18000, + 20000, + 25000, + 30000, + 35000, + 40000, + 45000, + 50000, + 60000, + 70000, + 80000, + 90000, + 100000, + 120000, + 140000, + 160000, + 180000, + 200000, + 250000, + 300000, + 350000, + 400000, + 450000, + 500000, + 600000, + 700000, + 800000, + 900000, + 1000000, + 1200000, + 1400000, + 1600000, + 1800000, + 2000000, + 2500000, + 3000000, + 3500000, + 4000000, + 4500000, + 5000000, + 6000000, + 7000000, + 8000000, + 9000000, + 10000000, + 12000000, + 14000000, + 16000000, + 18000000, + 20000000, + 25000000, + 30000000, + 35000000, + 40000000, + 45000000, + 50000000, + 60000000, + 70000000, + 80000000, + 90000000, + 100000000, + 120000000, + 140000000, + 160000000, + 180000000, + 200000000, + 250000000, + 300000000, + 350000000, + 400000000, + 450000000, + 500000000, + 600000000, + 700000000, + 800000000, + 900000000, + 1000000000, + 1200000000, + 1400000000, + 1600000000, + 1800000000, + 2000000000, + 2500000000.0, + 3000000000.0, + 3500000000.0, + 4000000000.0, + 4500000000.0, + 5000000000.0, + 6000000000.0, + 7000000000.0, + 8000000000.0, + 9000000000.0, + 1e200 + }; + + private static int findBucketIndex(double[] values, double target) { + for (int i = 0; i < values.length; ++i) { + if (target < values[i]) { + return i; + } + } + return values.length; + } + + @Benchmark + @Warmup(iterations = 5, time = 1) + @Measurement(iterations = 10, time = 1) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + @Threads(value = 1) + public void linearArr5() { + int ignored = findBucketIndex(arr5, ThreadLocalRandom.current().nextDouble(150)); + } + + @Benchmark + @Warmup(iterations = 5, time = 1) + @Measurement(iterations = 10, time = 1) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + @Threads(value = 1) + public void linearArr10() { + int ignored = findBucketIndex(arr10, ThreadLocalRandom.current().nextDouble(5000)); + } + + @Benchmark + @Warmup(iterations = 5, time = 1) + @Measurement(iterations = 10, time = 1) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + @Threads(value = 1) + public void linearArrLarge() { + int ignored = findBucketIndex(arrLarge, ThreadLocalRandom.current().nextDouble()); + } + + @Benchmark + @Warmup(iterations = 5, time = 1) + @Measurement(iterations = 10, time = 1) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + @Threads(value = 1) + public void binaryArr5() { + int ignored = Arrays.binarySearch(arr5, ThreadLocalRandom.current().nextDouble(150)); + } + + @Benchmark + @Warmup(iterations = 5, time = 1) + @Measurement(iterations = 10, time = 1) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + @Threads(value = 1) + public void binaryArr10() { + int ignored = Arrays.binarySearch(arr10, ThreadLocalRandom.current().nextDouble(5000)); + } + + @Benchmark + @Warmup(iterations = 5, time = 1) + @Measurement(iterations = 10, time = 1) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + @Threads(value = 1) + public void binaryArrLarge() { + int ignored = Arrays.binarySearch(arrLarge, ThreadLocalRandom.current().nextDouble()); + } +} diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java new file mode 100644 index 00000000000..c35b2d41f45 --- /dev/null +++ b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java @@ -0,0 +1,74 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.aggregator; + +import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; +import io.opentelemetry.sdk.metrics.common.InstrumentType; +import io.opentelemetry.sdk.metrics.common.InstrumentValueType; +import io.opentelemetry.sdk.resources.Resource; +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +@State(Scope.Benchmark) +public class DoubleHistogramBenchmark { + private static final Aggregator aggregator = + AggregatorFactory.histogram(new double[] {10, 100, 1_000}, /* stateful= */ false) + .create( + Resource.getDefault(), + InstrumentationLibraryInfo.getEmpty(), + InstrumentDescriptor.create( + "name", + "description", + "1", + InstrumentType.VALUE_RECORDER, + InstrumentValueType.DOUBLE)); + private AggregatorHandle aggregatorHandle; + + @Setup(Level.Trial) + public final void setup() { + aggregatorHandle = aggregator.createHandle(); + } + + @Benchmark + @Fork(1) + @Warmup(iterations = 5, time = 1) + @Measurement(iterations = 10, time = 1) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + @Threads(value = 10) + public void aggregate_10Threads() { + aggregatorHandle.recordDouble(100.0056); + } + + @Benchmark + @Fork(1) + @Warmup(iterations = 5, time = 1) + @Measurement(iterations = 10, time = 1) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + @Threads(value = 5) + public void aggregate_5Threads() { + aggregatorHandle.recordDouble(100.0056); + } + + @Benchmark + @Fork(1) + @Warmup(iterations = 5, time = 1) + @Measurement(iterations = 10, time = 1) + @OutputTimeUnit(TimeUnit.MILLISECONDS) + @Threads(value = 1) + public void aggregate_1Threads() { + aggregatorHandle.recordDouble(100.0056); + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java index 05887a3fd20..920e1ea2f52 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java @@ -45,6 +45,18 @@ static AggregatorFactory count(AggregationTemporality temporality) { return new CountAggregatorFactory(temporality); } + /** + * Returns an {@code AggregatorFactory} that calculates an approximation of the distribution of + * the measurements taken. + * + * @param stateful configures if the aggregator is stateful. + * @param boundary configures the fixed bucket boundaries. + * @return an {@code AggregationFactory} that calculates histogram of recorded measurements. + */ + static AggregatorFactory histogram(double[] boundary, boolean stateful) { + return new HistogramAggregatorFactory(boundary, stateful); + } + /** * Returns an {@code AggregationFactory} that calculates the last value of all recorded * measurements. diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java new file mode 100644 index 00000000000..05f0ef567ad --- /dev/null +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -0,0 +1,168 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.aggregator; + +import io.opentelemetry.api.common.Labels; +import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; +import io.opentelemetry.sdk.metrics.data.AggregationTemporality; +import io.opentelemetry.sdk.metrics.data.DoubleHistogramData; +import io.opentelemetry.sdk.metrics.data.MetricData; +import io.opentelemetry.sdk.resources.Resource; +import java.util.Arrays; +import java.util.Collections; +import java.util.Map; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.stream.Collectors; +import javax.annotation.concurrent.GuardedBy; + +final class DoubleHistogramAggregator extends AbstractAggregator { + private final double[] boundaries; + + DoubleHistogramAggregator( + Resource resource, + InstrumentationLibraryInfo instrumentationLibraryInfo, + InstrumentDescriptor instrumentDescriptor, + double[] boundaries, + boolean stateful) { + super(resource, instrumentationLibraryInfo, instrumentDescriptor, stateful); + this.boundaries = boundaries; + } + + @Override + public AggregatorHandle createHandle() { + return new Handle(this.boundaries); + } + + @Override + public final HistogramAccumulation merge(HistogramAccumulation x, HistogramAccumulation y) { + if (!x.getBoundaries().equals(y.getBoundaries())) { + throw new IllegalArgumentException("can't merge histograms with different boundaries"); + } + + long[] mergedCounts = new long[x.getCounts().size()]; + for (int i = 0; i < x.getCounts().size(); ++i) { + mergedCounts[i] = x.getCounts().get(i) + y.getCounts().get(i); + } + return HistogramAccumulation.create( + x.getCount() + y.getCount(), + x.getSum() + y.getSum(), + x.getBoundaries(), + Arrays.stream(mergedCounts).boxed().collect(Collectors.toList())); + } + + @Override + public final MetricData toMetricData( + Map accumulationByLabels, + long startEpochNanos, + long lastCollectionEpoch, + long epochNanos) { + return MetricData.createDoubleHistogram( + getResource(), + getInstrumentationLibraryInfo(), + getInstrumentDescriptor().getName(), + getInstrumentDescriptor().getDescription(), + getInstrumentDescriptor().getUnit(), + DoubleHistogramData.create( + isStateful() ? AggregationTemporality.CUMULATIVE : AggregationTemporality.DELTA, + MetricDataUtils.toDoubleHistogramPointList( + accumulationByLabels, + isStateful() ? startEpochNanos : lastCollectionEpoch, + epochNanos))); + } + + @Override + public HistogramAccumulation accumulateDouble(double value) { + return HistogramAccumulation.create( + 1, value, Collections.emptyList(), Collections.singletonList(1L)); + } + + @Override + public HistogramAccumulation accumulateLong(long value) { + return HistogramAccumulation.create( + 1, value, Collections.emptyList(), Collections.singletonList(1L)); + } + + static final class Handle extends AggregatorHandle { + private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + + @GuardedBy("lock") + private final State current; + + Handle(double[] boundaries) { + current = new State(boundaries); + } + + @Override + protected HistogramAccumulation doAccumulateThenReset() { + lock.writeLock().lock(); + try { + HistogramAccumulation result = + HistogramAccumulation.create( + current.count, + current.sum, + Arrays.stream(current.boundaries).boxed().collect(Collectors.toList()), + Arrays.stream(current.counts).boxed().collect(Collectors.toList())); + current.reset(); + return result; + } finally { + lock.writeLock().unlock(); + } + } + + @Override + protected void doRecordDouble(double value) { + int bucketIndex = current.findBucketIndex(value); + + lock.writeLock().lock(); + try { + current.record(bucketIndex, value); + } finally { + lock.writeLock().unlock(); + } + } + + @Override + protected void doRecordLong(long value) { + doRecordDouble((double) value); + } + + private static final class State { + private long count; + private double sum; + private final double[] boundaries; + private final long[] counts; + + public State(double[] boundaries) { + this.boundaries = Arrays.copyOf(boundaries, boundaries.length); + this.counts = new long[this.boundaries.length + 1]; + reset(); + } + + // Benchmark shows that linear search performs better with ordinary buckets. + private int findBucketIndex(double value) { + for (int i = 0; i < this.boundaries.length; ++i) { + if (value < this.boundaries[i]) { + return i; + } + } + return this.boundaries.length; + } + + private void reset() { + this.count = 0; + this.sum = 0; + Arrays.fill(this.counts, 0); + } + + private void record(int bucketIndex, double value) { + this.count++; + this.sum += value; + this.counts[bucketIndex]++; + } + } + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java new file mode 100644 index 00000000000..87d04ccd24b --- /dev/null +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java @@ -0,0 +1,63 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.aggregator; + +import com.google.auto.value.AutoValue; +import io.opentelemetry.api.common.Labels; +import io.opentelemetry.sdk.metrics.data.DoubleHistogramPointData; +import java.util.List; +import javax.annotation.concurrent.Immutable; + +@Immutable +@AutoValue +public abstract class HistogramAccumulation { + /** + * Creates a new {@link HistogramAccumulation} with the given values. + * + * @return a new {@link HistogramAccumulation} with the given values. + */ + static HistogramAccumulation create( + long count, double sum, List boundaries, List counts) { + // TODO make it immutable? + return new AutoValue_HistogramAccumulation(count, sum, boundaries, counts); + } + + HistogramAccumulation() {} + + /** + * The number of measurements taken. + * + * @return the count of recorded measurements. + */ + abstract long getCount(); + + /** + * The sum of all measurements recorded. + * + * @return the sum of recorded measurements. + */ + abstract double getSum(); + + /** + * The bucket boundaries. For a Histogram with N defined boundaries, e.g, [x, y, z]. There are N+1 + * counts: [-inf, x), [x, y), [y, z), [z, +inf]. + * + * @return the bucket boundaries in increasing order. + */ + abstract List getBoundaries(); + + /** + * The counts in each bucket. + * + * @return the counts in each bucket. + */ + abstract List getCounts(); + + final DoubleHistogramPointData toPoint(long startEpochNanos, long epochNanos, Labels labels) { + return DoubleHistogramPointData.create( + startEpochNanos, epochNanos, labels, getSum(), getCount(), getBoundaries(), getCounts()); + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java new file mode 100644 index 00000000000..3d33895eefe --- /dev/null +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java @@ -0,0 +1,50 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.aggregator; + +import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; +import io.opentelemetry.sdk.resources.Resource; + +final class HistogramAggregatorFactory implements AggregatorFactory { + private final double[] boundaries; + private final boolean stateful; + + HistogramAggregatorFactory(double[] boundaries, boolean stateful) { + for (int i = 1; i < boundaries.length; ++i) { + if (Double.compare(boundaries[i - 1], boundaries[i]) >= 0) { + throw new IllegalArgumentException( + "invalid bucket boundary: " + boundaries[i - 1] + " >= " + boundaries[i]); + } + } + if (boundaries.length > 0) { + if (boundaries[0] == Double.NEGATIVE_INFINITY) { + throw new IllegalArgumentException("invalid bucket boundary: -Inf"); + } + if (boundaries[boundaries.length - 1] == Double.POSITIVE_INFINITY) { + throw new IllegalArgumentException("invalid bucket boundary: +Inf"); + } + } + this.boundaries = boundaries; + this.stateful = stateful; + } + + @Override + @SuppressWarnings("unchecked") + public Aggregator create( + Resource resource, + InstrumentationLibraryInfo instrumentationLibraryInfo, + InstrumentDescriptor descriptor) { + switch (descriptor.getValueType()) { + case LONG: + case DOUBLE: + return (Aggregator) + new DoubleHistogramAggregator( + resource, instrumentationLibraryInfo, descriptor, this.boundaries, this.stateful); + } + throw new IllegalArgumentException("Invalid instrument value type"); + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java index d21f4965638..8efa36e203f 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java @@ -6,6 +6,7 @@ package io.opentelemetry.sdk.metrics.aggregator; import io.opentelemetry.api.common.Labels; +import io.opentelemetry.sdk.metrics.data.DoubleHistogramPointData; import io.opentelemetry.sdk.metrics.data.DoublePointData; import io.opentelemetry.sdk.metrics.data.DoubleSummaryPointData; import io.opentelemetry.sdk.metrics.data.LongPointData; @@ -44,4 +45,13 @@ static List toDoubleSummaryPointList( points.add(aggregator.toPoint(startEpochNanos, epochNanos, labels))); return points; } + + static List toDoubleHistogramPointList( + Map accumulationMap, long startEpochNanos, long epochNanos) { + List points = new ArrayList<>(accumulationMap.size()); + accumulationMap.forEach( + (labels, aggregator) -> + points.add(aggregator.toPoint(startEpochNanos, epochNanos, labels))); + return points; + } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java index 4c3f0df9b8d..42a4408cf6d 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java @@ -13,6 +13,7 @@ import io.opentelemetry.sdk.metrics.common.InstrumentValueType; import io.opentelemetry.sdk.metrics.data.AggregationTemporality; import io.opentelemetry.sdk.resources.Resource; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; class AggregatorFactoryTest { @@ -123,4 +124,73 @@ void getSumAggregatorFactory() { InstrumentValueType.DOUBLE))) .isInstanceOf(DoubleSumAggregator.class); } + + @Test + void getHistogramAggregatorFactory() { + AggregatorFactory histogram = + AggregatorFactory.histogram(new double[] {1.0}, /* stateful= */ false); + assertThat( + histogram.create( + Resource.getDefault(), + InstrumentationLibraryInfo.getEmpty(), + InstrumentDescriptor.create( + "name", + "description", + "unit", + InstrumentType.VALUE_RECORDER, + InstrumentValueType.LONG))) + .isInstanceOf(DoubleHistogramAggregator.class); + assertThat( + histogram.create( + Resource.getDefault(), + InstrumentationLibraryInfo.getEmpty(), + InstrumentDescriptor.create( + "name", + "description", + "unit", + InstrumentType.VALUE_RECORDER, + InstrumentValueType.DOUBLE))) + .isInstanceOf(DoubleHistogramAggregator.class); + + assertThat( + histogram + .create( + Resource.getDefault(), + InstrumentationLibraryInfo.getEmpty(), + InstrumentDescriptor.create( + "name", + "description", + "unit", + InstrumentType.VALUE_RECORDER, + InstrumentValueType.LONG)) + .isStateful()) + .isFalse(); + assertThat( + AggregatorFactory.histogram(new double[] {1.0}, /* stateful= */ true) + .create( + Resource.getDefault(), + InstrumentationLibraryInfo.getEmpty(), + InstrumentDescriptor.create( + "name", + "description", + "unit", + InstrumentType.VALUE_RECORDER, + InstrumentValueType.DOUBLE)) + .isStateful()) + .isTrue(); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + AggregatorFactory.histogram( + new double[] {Double.NEGATIVE_INFINITY}, /* stateful= */ false)); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + AggregatorFactory.histogram( + new double[] {1, Double.POSITIVE_INFINITY}, /* stateful= */ false)); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> AggregatorFactory.histogram(new double[] {2, 1, 3}, /* stateful= */ false)); + } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java new file mode 100644 index 00000000000..0d8f1ec9e4b --- /dev/null +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java @@ -0,0 +1,182 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.aggregator; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.annotations.concurrent.GuardedBy; +import io.opentelemetry.api.common.Labels; +import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; +import io.opentelemetry.sdk.metrics.common.InstrumentType; +import io.opentelemetry.sdk.metrics.common.InstrumentValueType; +import io.opentelemetry.sdk.metrics.data.AggregationTemporality; +import io.opentelemetry.sdk.metrics.data.MetricData; +import io.opentelemetry.sdk.metrics.data.MetricDataType; +import io.opentelemetry.sdk.resources.Resource; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import javax.annotation.Nullable; +import org.junit.jupiter.api.Test; + +public class DoubleHistogramAggregatorTest { + private static final ImmutableList BUCKET_BOUNDARIES = + ImmutableList.of(10.0, 100.0, 1000.0); + private static final DoubleHistogramAggregator aggregator = + new DoubleHistogramAggregator( + Resource.getDefault(), + InstrumentationLibraryInfo.getEmpty(), + InstrumentDescriptor.create( + "name", + "description", + "unit", + InstrumentType.VALUE_RECORDER, + InstrumentValueType.LONG), + BUCKET_BOUNDARIES.stream().mapToDouble(i -> i).toArray(), + /* stateful= */ false); + + @Test + void createHandle() { + assertThat(aggregator.createHandle()).isInstanceOf(DoubleHistogramAggregator.Handle.class); + } + + @Test + void testRecordings() { + AggregatorHandle aggregatorHandle = aggregator.createHandle(); + aggregatorHandle.recordLong(20); + aggregatorHandle.recordLong(5); + aggregatorHandle.recordLong(150); + assertThat(aggregatorHandle.accumulateThenReset()) + .isEqualTo( + HistogramAccumulation.create( + 3, 175, BUCKET_BOUNDARIES, ImmutableList.of(1L, 1L, 1L, 0L))); + } + + @Test + void toAccumulationAndReset() { + AggregatorHandle aggregatorHandle = aggregator.createHandle(); + assertThat(aggregatorHandle.accumulateThenReset()).isNull(); + + aggregatorHandle.recordLong(100); + assertThat(aggregatorHandle.accumulateThenReset()) + .isEqualTo( + HistogramAccumulation.create( + 1, 100, BUCKET_BOUNDARIES, ImmutableList.of(0L, 0L, 1L, 0L))); + assertThat(aggregatorHandle.accumulateThenReset()).isNull(); + + aggregatorHandle.recordLong(0); + assertThat(aggregatorHandle.accumulateThenReset()) + .isEqualTo( + HistogramAccumulation.create( + 1, 0, BUCKET_BOUNDARIES, ImmutableList.of(1L, 0L, 0L, 0L))); + assertThat(aggregatorHandle.accumulateThenReset()).isNull(); + } + + @Test + void toMetricData() { + AggregatorHandle aggregatorHandle = aggregator.createHandle(); + aggregatorHandle.recordLong(10); + + MetricData metricData = + aggregator.toMetricData( + Collections.singletonMap(Labels.empty(), aggregatorHandle.accumulateThenReset()), + 0, + 10, + 100); + assertThat(metricData).isNotNull(); + assertThat(metricData.getType()).isEqualTo(MetricDataType.HISTOGRAM); + assertThat(metricData.getDoubleHistogramData().getAggregationTemporality()) + .isEqualTo(AggregationTemporality.DELTA); + } + + @Test + void accumulateData() { + assertThat(aggregator.accumulateDouble(2.0)) + .isEqualTo( + HistogramAccumulation.create(1, 2.0, Collections.emptyList(), ImmutableList.of(1L))); + assertThat(aggregator.accumulateLong(10)) + .isEqualTo( + HistogramAccumulation.create(1, 10.0, Collections.emptyList(), ImmutableList.of(1L))); + } + + @Test + void testMultithreadedUpdates() throws Exception { + final AggregatorHandle aggregatorHandle = aggregator.createHandle(); + final Histogram summarizer = new Histogram(); + int numberOfThreads = 10; + final long[] updates = new long[] {1, 2, 3, 5, 7, 11, 13, 17, 19, 23}; + final int numberOfUpdates = 1000; + final CountDownLatch startingGun = new CountDownLatch(numberOfThreads); + List workers = new ArrayList<>(); + for (int i = 0; i < numberOfThreads; i++) { + final int index = i; + Thread t = + new Thread( + () -> { + long update = updates[index]; + try { + startingGun.await(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + for (int j = 0; j < numberOfUpdates; j++) { + aggregatorHandle.recordLong(update); + if (ThreadLocalRandom.current().nextInt(10) == 0) { + summarizer.process(aggregatorHandle.accumulateThenReset()); + } + } + }); + workers.add(t); + t.start(); + } + for (int i = 0; i <= numberOfThreads; i++) { + startingGun.countDown(); + } + + for (Thread worker : workers) { + worker.join(); + } + // make sure everything gets merged when all the aggregation is done. + summarizer.process(aggregatorHandle.accumulateThenReset()); + + assertThat(summarizer.accumulation) + .isEqualTo( + HistogramAccumulation.create( + numberOfThreads * numberOfUpdates, + 101000, + BUCKET_BOUNDARIES, + ImmutableList.of(5000L, 5000L, 0L, 0L))); + } + + private static final class Histogram { + private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + + @GuardedBy("lock") + @Nullable + private HistogramAccumulation accumulation; + + void process(@Nullable HistogramAccumulation other) { + if (other == null) { + return; + } + lock.writeLock().lock(); + try { + if (accumulation == null) { + accumulation = other; + return; + } + accumulation = aggregator.merge(accumulation, other); + } finally { + lock.writeLock().unlock(); + } + } + } +} diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java new file mode 100644 index 00000000000..ec32f9f9c63 --- /dev/null +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java @@ -0,0 +1,49 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.aggregator; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.google.common.collect.ImmutableList; +import io.opentelemetry.api.common.Labels; +import io.opentelemetry.sdk.metrics.data.DoubleHistogramPointData; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.Test; + +public class HistogramAccumulationTest { + @Test + void toPoint() { + HistogramAccumulation accumulation = + HistogramAccumulation.create(12, 25, ImmutableList.of(1.0), ImmutableList.of(1L, 2L)); + DoubleHistogramPointData point = getPoint(accumulation); + assertThat(point.getCount()).isEqualTo(12); + assertThat(point.getSum()).isEqualTo(25); + assertThat(point.getBoundaries()).isEqualTo(ImmutableList.of(1.0)); + assertThat(point.getCounts()).isEqualTo(ImmutableList.of(1L, 2L)); + + List boundaries = new ArrayList<>(); + List counts = new ArrayList<>(); + point.forEach( + (b, c) -> { + boundaries.add(b); + counts.add(c); + }); + assertThat(boundaries).isEqualTo(ImmutableList.of(1.0, Double.POSITIVE_INFINITY)); + assertThat(counts).isEqualTo(point.getCounts()); + } + + private static DoubleHistogramPointData getPoint(HistogramAccumulation accumulation) { + DoubleHistogramPointData point = accumulation.toPoint(12345, 12358, Labels.of("key", "value")); + assertThat(point).isNotNull(); + assertThat(point.getStartEpochNanos()).isEqualTo(12345); + assertThat(point.getEpochNanos()).isEqualTo(12358); + assertThat(point.getLabels().size()).isEqualTo(1); + assertThat(point.getLabels().get("key")).isEqualTo("value"); + assertThat(point).isInstanceOf(DoubleHistogramPointData.class); + return point; + } +} From 264979b53f8b663e7e924154656b50b8b3b96423 Mon Sep 17 00:00:00 2001 From: beanliu Date: Wed, 27 Jan 2021 15:26:47 +1100 Subject: [PATCH 04/17] fixup! add MetricDataType.HISTOGRAM and related data container types --- .../sdk/metrics/data/DoubleHistogramPointData.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java index 111be05fcf4..b76237b03e5 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java @@ -19,8 +19,11 @@ @AutoValue public abstract class DoubleHistogramPointData implements PointData { /** - * Creates a DoubleHistogramPointData. + * Creates a DoubleHistogramPointData. It's the caller's responsibility to make sure that the + * `boundaries` and `counts` are unmodifiable. * + * @param boundaries the bucket boundaries in unmodifiable mode. + * @param counts the bucket count in unmodifiable mode. * @return a DoubleHistogramPointData. */ public static DoubleHistogramPointData create( @@ -53,16 +56,17 @@ public static DoubleHistogramPointData create( /** * The bucket boundaries. For a Histogram with N defined boundaries, e.g, [x, y, z]. There are N+1 - * counts: [-inf, x), [x, y), [y, z), [z, +inf]. + * counts: [-inf, x), [x, y), [y, z), [z, +inf]. The returned object is unmodifiable so do not + * mutate it. * - * @return the bucket boundaries in increasing order. + * @return the unmodifiable bucket boundaries in increasing order. */ public abstract List getBoundaries(); /** - * The counts in each bucket. + * The counts in each bucket. The returned object is unmodifiable so do not mutate it. * - * @return the counts in each bucket. + * @return the unmodifiable counts in each bucket. */ public abstract List getCounts(); From 085955382c9cc9e4bb8b6d586322d973a9ee6762 Mon Sep 17 00:00:00 2001 From: beanliu Date: Wed, 27 Jan 2021 15:37:52 +1100 Subject: [PATCH 05/17] fixup! add Histogram aggregator and accumulation --- .../sdk/metrics/aggregator/BucketSearch.java | 244 ------------------ .../metrics/aggregator/AggregatorFactory.java | 6 +- .../aggregator/DoubleHistogramAggregator.java | 59 ++++- .../aggregator/HistogramAccumulation.java | 15 +- 4 files changed, 58 insertions(+), 266 deletions(-) delete mode 100644 sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/BucketSearch.java diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/BucketSearch.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/BucketSearch.java deleted file mode 100644 index 6f0b39594d7..00000000000 --- a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/BucketSearch.java +++ /dev/null @@ -1,244 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.sdk.metrics.aggregator; - -import java.util.Arrays; -import java.util.concurrent.ThreadLocalRandom; -import java.util.concurrent.TimeUnit; -import org.openjdk.jmh.annotations.Benchmark; -import org.openjdk.jmh.annotations.Measurement; -import org.openjdk.jmh.annotations.OutputTimeUnit; -import org.openjdk.jmh.annotations.Scope; -import org.openjdk.jmh.annotations.State; -import org.openjdk.jmh.annotations.Threads; -import org.openjdk.jmh.annotations.Warmup; - -@State(Scope.Benchmark) -public class BucketSearch { - private static final double[] arr5 = new double[] {5, 10, 25, 50, 100}; - private static final double[] arr10 = - new double[] {10, 20, 40, 80, 160, 320, 640, 1280, 2560, 5120}; - private static final double[] arrLarge = - new double[] { - 1, - 2, - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 12, - 14, - 16, - 18, - 20, - 25, - 30, - 35, - 40, - 45, - 50, - 60, - 70, - 80, - 90, - 100, - 120, - 140, - 160, - 180, - 200, - 250, - 300, - 350, - 400, - 450, - 500, - 600, - 700, - 800, - 900, - 1000, - 1200, - 1400, - 1600, - 1800, - 2000, - 2500, - 3000, - 3500, - 4000, - 4500, - 5000, - 6000, - 7000, - 8000, - 9000, - 10000, - 12000, - 14000, - 16000, - 18000, - 20000, - 25000, - 30000, - 35000, - 40000, - 45000, - 50000, - 60000, - 70000, - 80000, - 90000, - 100000, - 120000, - 140000, - 160000, - 180000, - 200000, - 250000, - 300000, - 350000, - 400000, - 450000, - 500000, - 600000, - 700000, - 800000, - 900000, - 1000000, - 1200000, - 1400000, - 1600000, - 1800000, - 2000000, - 2500000, - 3000000, - 3500000, - 4000000, - 4500000, - 5000000, - 6000000, - 7000000, - 8000000, - 9000000, - 10000000, - 12000000, - 14000000, - 16000000, - 18000000, - 20000000, - 25000000, - 30000000, - 35000000, - 40000000, - 45000000, - 50000000, - 60000000, - 70000000, - 80000000, - 90000000, - 100000000, - 120000000, - 140000000, - 160000000, - 180000000, - 200000000, - 250000000, - 300000000, - 350000000, - 400000000, - 450000000, - 500000000, - 600000000, - 700000000, - 800000000, - 900000000, - 1000000000, - 1200000000, - 1400000000, - 1600000000, - 1800000000, - 2000000000, - 2500000000.0, - 3000000000.0, - 3500000000.0, - 4000000000.0, - 4500000000.0, - 5000000000.0, - 6000000000.0, - 7000000000.0, - 8000000000.0, - 9000000000.0, - 1e200 - }; - - private static int findBucketIndex(double[] values, double target) { - for (int i = 0; i < values.length; ++i) { - if (target < values[i]) { - return i; - } - } - return values.length; - } - - @Benchmark - @Warmup(iterations = 5, time = 1) - @Measurement(iterations = 10, time = 1) - @OutputTimeUnit(TimeUnit.MILLISECONDS) - @Threads(value = 1) - public void linearArr5() { - int ignored = findBucketIndex(arr5, ThreadLocalRandom.current().nextDouble(150)); - } - - @Benchmark - @Warmup(iterations = 5, time = 1) - @Measurement(iterations = 10, time = 1) - @OutputTimeUnit(TimeUnit.MILLISECONDS) - @Threads(value = 1) - public void linearArr10() { - int ignored = findBucketIndex(arr10, ThreadLocalRandom.current().nextDouble(5000)); - } - - @Benchmark - @Warmup(iterations = 5, time = 1) - @Measurement(iterations = 10, time = 1) - @OutputTimeUnit(TimeUnit.MILLISECONDS) - @Threads(value = 1) - public void linearArrLarge() { - int ignored = findBucketIndex(arrLarge, ThreadLocalRandom.current().nextDouble()); - } - - @Benchmark - @Warmup(iterations = 5, time = 1) - @Measurement(iterations = 10, time = 1) - @OutputTimeUnit(TimeUnit.MILLISECONDS) - @Threads(value = 1) - public void binaryArr5() { - int ignored = Arrays.binarySearch(arr5, ThreadLocalRandom.current().nextDouble(150)); - } - - @Benchmark - @Warmup(iterations = 5, time = 1) - @Measurement(iterations = 10, time = 1) - @OutputTimeUnit(TimeUnit.MILLISECONDS) - @Threads(value = 1) - public void binaryArr10() { - int ignored = Arrays.binarySearch(arr10, ThreadLocalRandom.current().nextDouble(5000)); - } - - @Benchmark - @Warmup(iterations = 5, time = 1) - @Measurement(iterations = 10, time = 1) - @OutputTimeUnit(TimeUnit.MILLISECONDS) - @Threads(value = 1) - public void binaryArrLarge() { - int ignored = Arrays.binarySearch(arrLarge, ThreadLocalRandom.current().nextDouble()); - } -} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java index 920e1ea2f52..5ae686e19c8 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java @@ -50,11 +50,11 @@ static AggregatorFactory count(AggregationTemporality temporality) { * the measurements taken. * * @param stateful configures if the aggregator is stateful. - * @param boundary configures the fixed bucket boundaries. + * @param boundaries configures the fixed bucket boundaries. * @return an {@code AggregationFactory} that calculates histogram of recorded measurements. */ - static AggregatorFactory histogram(double[] boundary, boolean stateful) { - return new HistogramAggregatorFactory(boundary, stateful); + static AggregatorFactory histogram(double[] boundaries, boolean stateful) { + return new HistogramAggregatorFactory(boundaries, stateful); } /** diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index 05f0ef567ad..0e930793708 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -12,14 +12,21 @@ import io.opentelemetry.sdk.metrics.data.DoubleHistogramData; import io.opentelemetry.sdk.metrics.data.MetricData; import io.opentelemetry.sdk.resources.Resource; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.concurrent.locks.ReentrantReadWriteLock; -import java.util.stream.Collectors; +import java.util.logging.Level; +import java.util.logging.Logger; import javax.annotation.concurrent.GuardedBy; final class DoubleHistogramAggregator extends AbstractAggregator { + private static final Logger logger = Logger.getLogger(DoubleHistogramAggregator.class.getName()); + + private static boolean LoggedMergingInvalidBoundaries = false; + private final double[] boundaries; DoubleHistogramAggregator( @@ -32,6 +39,22 @@ final class DoubleHistogramAggregator extends AbstractAggregator asUnmodifiableDoubleList(double[] xs) { + List result = new ArrayList<>(xs.length); + for (double x : xs) { + result.add(x); + } + return result; + } + + private static List asUnmodifiableLongList(long[] xs) { + List result = new ArrayList<>(xs.length); + for (long x : xs) { + result.add(x); + } + return result; + } + @Override public AggregatorHandle createHandle() { return new Handle(this.boundaries); @@ -40,7 +63,19 @@ public AggregatorHandle createHandle() { @Override public final HistogramAccumulation merge(HistogramAccumulation x, HistogramAccumulation y) { if (!x.getBoundaries().equals(y.getBoundaries())) { - throw new IllegalArgumentException("can't merge histograms with different boundaries"); + // If this happens, it's a pretty severe bug in the SDK. + if (!LoggedMergingInvalidBoundaries) { + logger.log( + Level.SEVERE, + "can't merge histograms with different boundaries, something's very wrong: " + + "x.boundaries=" + + x.getBoundaries() + + " y.boundaries=" + + y.getBoundaries()); + LoggedMergingInvalidBoundaries = true; + } + return HistogramAccumulation.create( + 0, 0, Collections.emptyList(), Collections.singletonList(0L)); } long[] mergedCounts = new long[x.getCounts().size()]; @@ -51,7 +86,7 @@ public final HistogramAccumulation merge(HistogramAccumulation x, HistogramAccum x.getCount() + y.getCount(), x.getSum() + y.getSum(), x.getBoundaries(), - Arrays.stream(mergedCounts).boxed().collect(Collectors.toList())); + asUnmodifiableLongList(mergedCounts)); } @Override @@ -98,19 +133,20 @@ static final class Handle extends AggregatorHandle { @Override protected HistogramAccumulation doAccumulateThenReset() { + List counts; lock.writeLock().lock(); try { - HistogramAccumulation result = - HistogramAccumulation.create( - current.count, - current.sum, - Arrays.stream(current.boundaries).boxed().collect(Collectors.toList()), - Arrays.stream(current.counts).boxed().collect(Collectors.toList())); + counts = asUnmodifiableLongList(current.counts); current.reset(); - return result; } finally { lock.writeLock().unlock(); } + + return HistogramAccumulation.create( + counts.stream().mapToLong(i -> i).sum(), + current.sum, + asUnmodifiableDoubleList(current.boundaries), + asUnmodifiableLongList(current.counts)); } @Override @@ -131,7 +167,6 @@ protected void doRecordLong(long value) { } private static final class State { - private long count; private double sum; private final double[] boundaries; private final long[] counts; @@ -153,13 +188,11 @@ private int findBucketIndex(double value) { } private void reset() { - this.count = 0; this.sum = 0; Arrays.fill(this.counts, 0); } private void record(int bucketIndex, double value) { - this.count++; this.sum += value; this.counts[bucketIndex]++; } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java index 87d04ccd24b..a14edd1ffb4 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java @@ -15,13 +15,15 @@ @AutoValue public abstract class HistogramAccumulation { /** - * Creates a new {@link HistogramAccumulation} with the given values. + * Creates a new {@link HistogramAccumulation} with the given values. It's the caller's + * responsibility to make sure that the `boundaries` and `counts` are unmodifiable. * + * @param boundaries the bucket boundaries in unmodifiable mode. + * @param counts the bucket count in unmodifiable mode. * @return a new {@link HistogramAccumulation} with the given values. */ static HistogramAccumulation create( long count, double sum, List boundaries, List counts) { - // TODO make it immutable? return new AutoValue_HistogramAccumulation(count, sum, boundaries, counts); } @@ -43,16 +45,17 @@ static HistogramAccumulation create( /** * The bucket boundaries. For a Histogram with N defined boundaries, e.g, [x, y, z]. There are N+1 - * counts: [-inf, x), [x, y), [y, z), [z, +inf]. + * counts: [-inf, x), [x, y), [y, z), [z, +inf]. The returned object is unmodifiable so do not + * mutate it. * - * @return the bucket boundaries in increasing order. + * @return the unmodifiable bucket boundaries in increasing order. */ abstract List getBoundaries(); /** - * The counts in each bucket. + * The counts in each bucket. The returned object is unmodifiable so do not mutate it. * - * @return the counts in each bucket. + * @return the unmodifiable counts in each bucket. */ abstract List getCounts(); From 088f6157eb76b6573f2ae268ba306788860bee78 Mon Sep 17 00:00:00 2001 From: beanliu Date: Wed, 27 Jan 2021 16:01:02 +1100 Subject: [PATCH 06/17] fixup! add Histogram aggregator and accumulation --- .../sdk/metrics/aggregator/DoubleHistogramAggregator.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index 0e930793708..9415fb6a0e8 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -25,7 +25,7 @@ final class DoubleHistogramAggregator extends AbstractAggregator { private static final Logger logger = Logger.getLogger(DoubleHistogramAggregator.class.getName()); - private static boolean LoggedMergingInvalidBoundaries = false; + private static volatile boolean loggedMergingInvalidBoundaries = false; private final double[] boundaries; @@ -64,7 +64,7 @@ public AggregatorHandle createHandle() { public final HistogramAccumulation merge(HistogramAccumulation x, HistogramAccumulation y) { if (!x.getBoundaries().equals(y.getBoundaries())) { // If this happens, it's a pretty severe bug in the SDK. - if (!LoggedMergingInvalidBoundaries) { + if (!loggedMergingInvalidBoundaries) { logger.log( Level.SEVERE, "can't merge histograms with different boundaries, something's very wrong: " @@ -72,7 +72,7 @@ public final HistogramAccumulation merge(HistogramAccumulation x, HistogramAccum + x.getBoundaries() + " y.boundaries=" + y.getBoundaries()); - LoggedMergingInvalidBoundaries = true; + loggedMergingInvalidBoundaries = true; } return HistogramAccumulation.create( 0, 0, Collections.emptyList(), Collections.singletonList(0L)); From a7e91422e1023b11a95e09e55a20aaad75573e28 Mon Sep 17 00:00:00 2001 From: beanliu Date: Wed, 27 Jan 2021 16:17:49 +1100 Subject: [PATCH 07/17] fixup! add Histogram aggregator and accumulation --- .../sdk/metrics/aggregator/DoubleHistogramAggregator.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index 9415fb6a0e8..74619b1f8e4 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -133,9 +133,11 @@ static final class Handle extends AggregatorHandle { @Override protected HistogramAccumulation doAccumulateThenReset() { + double sum; List counts; lock.writeLock().lock(); try { + sum = current.sum; counts = asUnmodifiableLongList(current.counts); current.reset(); } finally { @@ -144,9 +146,9 @@ protected HistogramAccumulation doAccumulateThenReset() { return HistogramAccumulation.create( counts.stream().mapToLong(i -> i).sum(), - current.sum, + sum, asUnmodifiableDoubleList(current.boundaries), - asUnmodifiableLongList(current.counts)); + counts); } @Override From 7b8cebdd75445bcaa744208e757d3c2a85c6c4b3 Mon Sep 17 00:00:00 2001 From: beanliu Date: Wed, 27 Jan 2021 16:38:01 +1100 Subject: [PATCH 08/17] fixup! add Histogram aggregator and accumulation --- .../aggregator/DoubleHistogramAggregatorTest.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java index 0d8f1ec9e4b..3d5591546ae 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java @@ -80,6 +80,17 @@ void toAccumulationAndReset() { assertThat(aggregatorHandle.accumulateThenReset()).isNull(); } + @Test + void invalidMergeReturnsEmptyAccumulation() { + HistogramAccumulation x = + HistogramAccumulation.create(1, 1, ImmutableList.of(1.0), ImmutableList.of(0L, 1L)); + HistogramAccumulation y = + HistogramAccumulation.create(1, 2, ImmutableList.of(2.0), ImmutableList.of(1L)); + assertThat(aggregator.merge(x, y)) + .isEqualTo( + HistogramAccumulation.create(0, 0, Collections.emptyList(), ImmutableList.of(0L))); + } + @Test void toMetricData() { AggregatorHandle aggregatorHandle = aggregator.createHandle(); From 75c9e4bbdfd1c03ae6932e7c09b57d77fa1351c8 Mon Sep 17 00:00:00 2001 From: beanliu Date: Fri, 29 Jan 2021 09:19:20 +1100 Subject: [PATCH 09/17] fixup! add Histogram aggregator and accumulation --- .../sdk/metrics/aggregator/DoubleHistogramAggregator.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index 74619b1f8e4..5e8a7eaacf1 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -179,7 +179,8 @@ public State(double[] boundaries) { reset(); } - // Benchmark shows that linear search performs better with ordinary buckets. + // Benchmark shows that linear search performs better than binary search with ordinary + // buckets. private int findBucketIndex(double value) { for (int i = 0; i < this.boundaries.length; ++i) { if (value < this.boundaries[i]) { From 79c1e0edd283ffd0eee988eefaa065721e345ada Mon Sep 17 00:00:00 2001 From: beanliu Date: Fri, 29 Jan 2021 10:13:18 +1100 Subject: [PATCH 10/17] use Immutable(Long|Double)Array for histogram aggregation --- .../aggregator/DoubleHistogramBenchmark.java | 4 +- .../metrics/aggregator/AggregatorFactory.java | 3 +- .../aggregator/DoubleHistogramAggregator.java | 68 +++++------ .../aggregator/HistogramAccumulation.java | 23 ++-- .../HistogramAggregatorFactory.java | 17 +-- .../metrics/common/ImmutableDoubleArray.java | 107 ++++++++++++++++++ .../metrics/common/ImmutableLongArray.java | 107 ++++++++++++++++++ .../data/DoubleHistogramPointData.java | 43 +++---- .../aggregator/AggregatorFactoryTest.java | 17 ++- .../DoubleHistogramAggregatorTest.java | 32 +++--- .../aggregator/HistogramAccumulationTest.java | 11 +- .../sdk/metrics/data/MetricDataTest.java | 13 ++- 12 files changed, 333 insertions(+), 112 deletions(-) create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java index c35b2d41f45..e2924db86e5 100644 --- a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java +++ b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java @@ -6,6 +6,7 @@ package io.opentelemetry.sdk.metrics.aggregator; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.common.InstrumentType; import io.opentelemetry.sdk.metrics.common.InstrumentValueType; @@ -25,7 +26,8 @@ @State(Scope.Benchmark) public class DoubleHistogramBenchmark { private static final Aggregator aggregator = - AggregatorFactory.histogram(new double[] {10, 100, 1_000}, /* stateful= */ false) + AggregatorFactory.histogram( + ImmutableDoubleArray.copyOf(new double[] {10, 100, 1_000}), /* stateful= */ false) .create( Resource.getDefault(), InstrumentationLibraryInfo.getEmpty(), diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java index 5ae686e19c8..84649669948 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java @@ -6,6 +6,7 @@ package io.opentelemetry.sdk.metrics.aggregator; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.data.AggregationTemporality; import io.opentelemetry.sdk.resources.Resource; @@ -53,7 +54,7 @@ static AggregatorFactory count(AggregationTemporality temporality) { * @param boundaries configures the fixed bucket boundaries. * @return an {@code AggregationFactory} that calculates histogram of recorded measurements. */ - static AggregatorFactory histogram(double[] boundaries, boolean stateful) { + static AggregatorFactory histogram(ImmutableDoubleArray boundaries, boolean stateful) { return new HistogramAggregatorFactory(boundaries, stateful); } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index 5e8a7eaacf1..012a9bd157f 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -7,15 +7,14 @@ import io.opentelemetry.api.common.Labels; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; +import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.data.AggregationTemporality; import io.opentelemetry.sdk.metrics.data.DoubleHistogramData; import io.opentelemetry.sdk.metrics.data.MetricData; import io.opentelemetry.sdk.resources.Resource; -import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; -import java.util.List; import java.util.Map; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.logging.Level; @@ -27,34 +26,18 @@ final class DoubleHistogramAggregator extends AbstractAggregator asUnmodifiableDoubleList(double[] xs) { - List result = new ArrayList<>(xs.length); - for (double x : xs) { - result.add(x); - } - return result; - } - - private static List asUnmodifiableLongList(long[] xs) { - List result = new ArrayList<>(xs.length); - for (long x : xs) { - result.add(x); - } - return result; - } - @Override public AggregatorHandle createHandle() { return new Handle(this.boundaries); @@ -75,18 +58,18 @@ public final HistogramAccumulation merge(HistogramAccumulation x, HistogramAccum loggedMergingInvalidBoundaries = true; } return HistogramAccumulation.create( - 0, 0, Collections.emptyList(), Collections.singletonList(0L)); + 0, 0, ImmutableDoubleArray.of(), ImmutableLongArray.of(0)); } - long[] mergedCounts = new long[x.getCounts().size()]; - for (int i = 0; i < x.getCounts().size(); ++i) { + long[] mergedCounts = new long[x.getCounts().length()]; + for (int i = 0; i < x.getCounts().length(); ++i) { mergedCounts[i] = x.getCounts().get(i) + y.getCounts().get(i); } return HistogramAccumulation.create( x.getCount() + y.getCount(), x.getSum() + y.getSum(), x.getBoundaries(), - asUnmodifiableLongList(mergedCounts)); + ImmutableLongArray.copyOf(mergedCounts)); } @Override @@ -112,13 +95,13 @@ public final MetricData toMetricData( @Override public HistogramAccumulation accumulateDouble(double value) { return HistogramAccumulation.create( - 1, value, Collections.emptyList(), Collections.singletonList(1L)); + 1, value, ImmutableDoubleArray.of(), ImmutableLongArray.of(1)); } @Override public HistogramAccumulation accumulateLong(long value) { return HistogramAccumulation.create( - 1, value, Collections.emptyList(), Collections.singletonList(1L)); + 1, value, ImmutableDoubleArray.of(), ImmutableLongArray.of(1)); } static final class Handle extends AggregatorHandle { @@ -127,28 +110,29 @@ static final class Handle extends AggregatorHandle { @GuardedBy("lock") private final State current; - Handle(double[] boundaries) { + Handle(ImmutableDoubleArray boundaries) { current = new State(boundaries); } @Override protected HistogramAccumulation doAccumulateThenReset() { double sum; - List counts; + ImmutableLongArray counts; lock.writeLock().lock(); try { sum = current.sum; - counts = asUnmodifiableLongList(current.counts); + counts = ImmutableLongArray.copyOf(current.counts); current.reset(); } finally { lock.writeLock().unlock(); } - return HistogramAccumulation.create( - counts.stream().mapToLong(i -> i).sum(), - sum, - asUnmodifiableDoubleList(current.boundaries), - counts); + long total_count = 0; + for (int i = 0; i < counts.length(); ++i) { + total_count += counts.get(i); + } + + return HistogramAccumulation.create(total_count, sum, current.boundaries, counts); } @Override @@ -170,24 +154,24 @@ protected void doRecordLong(long value) { private static final class State { private double sum; - private final double[] boundaries; + private final ImmutableDoubleArray boundaries; private final long[] counts; - public State(double[] boundaries) { - this.boundaries = Arrays.copyOf(boundaries, boundaries.length); - this.counts = new long[this.boundaries.length + 1]; + public State(ImmutableDoubleArray boundaries) { + this.boundaries = boundaries; + this.counts = new long[this.boundaries.length() + 1]; reset(); } // Benchmark shows that linear search performs better than binary search with ordinary // buckets. private int findBucketIndex(double value) { - for (int i = 0; i < this.boundaries.length; ++i) { - if (value < this.boundaries[i]) { + for (int i = 0; i < this.boundaries.length(); ++i) { + if (value < this.boundaries.get(i)) { return i; } } - return this.boundaries.length; + return this.boundaries.length(); } private void reset() { diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java index a14edd1ffb4..41ffe3de139 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java @@ -7,23 +7,21 @@ import com.google.auto.value.AutoValue; import io.opentelemetry.api.common.Labels; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; +import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import io.opentelemetry.sdk.metrics.data.DoubleHistogramPointData; -import java.util.List; import javax.annotation.concurrent.Immutable; @Immutable @AutoValue public abstract class HistogramAccumulation { /** - * Creates a new {@link HistogramAccumulation} with the given values. It's the caller's - * responsibility to make sure that the `boundaries` and `counts` are unmodifiable. + * Creates a new {@link HistogramAccumulation} with the given values. * - * @param boundaries the bucket boundaries in unmodifiable mode. - * @param counts the bucket count in unmodifiable mode. * @return a new {@link HistogramAccumulation} with the given values. */ static HistogramAccumulation create( - long count, double sum, List boundaries, List counts) { + long count, double sum, ImmutableDoubleArray boundaries, ImmutableLongArray counts) { return new AutoValue_HistogramAccumulation(count, sum, boundaries, counts); } @@ -45,19 +43,18 @@ static HistogramAccumulation create( /** * The bucket boundaries. For a Histogram with N defined boundaries, e.g, [x, y, z]. There are N+1 - * counts: [-inf, x), [x, y), [y, z), [z, +inf]. The returned object is unmodifiable so do not - * mutate it. + * counts: [-inf, x), [x, y), [y, z), [z, +inf]. * - * @return the unmodifiable bucket boundaries in increasing order. + * @return the bucket boundaries in increasing order. */ - abstract List getBoundaries(); + abstract ImmutableDoubleArray getBoundaries(); /** - * The counts in each bucket. The returned object is unmodifiable so do not mutate it. + * The counts in each bucket. * - * @return the unmodifiable counts in each bucket. + * @return the counts in each bucket. */ - abstract List getCounts(); + abstract ImmutableLongArray getCounts(); final DoubleHistogramPointData toPoint(long startEpochNanos, long epochNanos, Labels labels) { return DoubleHistogramPointData.create( diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java index 3d33895eefe..8ac37000676 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java @@ -6,25 +6,26 @@ package io.opentelemetry.sdk.metrics.aggregator; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.resources.Resource; final class HistogramAggregatorFactory implements AggregatorFactory { - private final double[] boundaries; + private final ImmutableDoubleArray boundaries; private final boolean stateful; - HistogramAggregatorFactory(double[] boundaries, boolean stateful) { - for (int i = 1; i < boundaries.length; ++i) { - if (Double.compare(boundaries[i - 1], boundaries[i]) >= 0) { + HistogramAggregatorFactory(ImmutableDoubleArray boundaries, boolean stateful) { + for (int i = 1; i < boundaries.length(); ++i) { + if (Double.compare(boundaries.get(i - 1), boundaries.get(i)) >= 0) { throw new IllegalArgumentException( - "invalid bucket boundary: " + boundaries[i - 1] + " >= " + boundaries[i]); + "invalid bucket boundary: " + boundaries.get(i - 1) + " >= " + boundaries.get(i)); } } - if (boundaries.length > 0) { - if (boundaries[0] == Double.NEGATIVE_INFINITY) { + if (boundaries.length() > 0) { + if (boundaries.get(0) == Double.NEGATIVE_INFINITY) { throw new IllegalArgumentException("invalid bucket boundary: -Inf"); } - if (boundaries[boundaries.length - 1] == Double.POSITIVE_INFINITY) { + if (boundaries.get(boundaries.length() - 1) == Double.POSITIVE_INFINITY) { throw new IllegalArgumentException("invalid bucket boundary: +Inf"); } } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java new file mode 100644 index 00000000000..7098a530660 --- /dev/null +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java @@ -0,0 +1,107 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.common; + +import java.util.Arrays; +import javax.annotation.Nullable; +import javax.annotation.concurrent.Immutable; + +@Immutable +public class ImmutableDoubleArray { + private static final ImmutableDoubleArray EMPTY = new ImmutableDoubleArray(new double[0]); + + /** Returns the empty array. */ + public static ImmutableDoubleArray of() { + return EMPTY; + } + + /** Returns an immutable array containing a single value. */ + public static ImmutableDoubleArray of(double e0) { + return new ImmutableDoubleArray(new double[] {e0}); + } + + /** Returns an immutable array containing the given values, in order. */ + public static ImmutableDoubleArray copyOf(double[] values) { + return values.length == 0 + ? EMPTY + : new ImmutableDoubleArray(Arrays.copyOf(values, values.length)); + } + + private final double[] array; + + private ImmutableDoubleArray(double[] array) { + this.array = array; + } + + /** Returns the number of values in this array. */ + public int length() { + return array.length; + } + + /** + * Returns the {@code double} value present at the given index. + * + * @throws IndexOutOfBoundsException if {@code index} is negative, or greater than or equal to + * {@link #length} + */ + public double get(int index) { + return array[index]; + } + + /** + * Returns {@code true} if {@code object} is an {@code ImmutableDoubleArray} containing the same + * values as this one, in the same order. + */ + @Override + public boolean equals(@Nullable Object object) { + if (object == this) { + return true; + } + if (!(object instanceof ImmutableDoubleArray)) { + return false; + } + ImmutableDoubleArray that = (ImmutableDoubleArray) object; + if (this.length() != that.length()) { + return false; + } + for (int i = 0; i < length(); i++) { + if (this.get(i) != that.get(i)) { + return false; + } + } + return true; + } + + /** Returns an unspecified hash code for the contents of this immutable array. */ + @Override + public int hashCode() { + int hash = 1; + for (double value : array) { + hash *= 31; + hash += ((Double) value).hashCode(); + } + return hash; + } + + /** + * Returns a string representation of this array in the same form as {@link + * Arrays#toString(double[])}, for example {@code "[1, 2, 3]"}. + */ + @Override + public String toString() { + if (length() == 0) { + return "[]"; + } + StringBuilder builder = new StringBuilder(length() * 5); // rough estimate is fine + builder.append('[').append(array[0]); + + for (int i = 1; i < length(); i++) { + builder.append(", ").append(array[i]); + } + builder.append(']'); + return builder.toString(); + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java new file mode 100644 index 00000000000..c1b9625006a --- /dev/null +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java @@ -0,0 +1,107 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.common; + +import java.util.Arrays; +import javax.annotation.Nullable; +import javax.annotation.concurrent.Immutable; + +@Immutable +public class ImmutableLongArray { + private static final ImmutableLongArray EMPTY = new ImmutableLongArray(new long[0]); + + /** Returns the empty array. */ + public static ImmutableLongArray of() { + return EMPTY; + } + + /** Returns an immutable array containing a single value. */ + public static ImmutableLongArray of(long e0) { + return new ImmutableLongArray(new long[] {e0}); + } + + /** Returns an immutable array containing the given values, in order. */ + public static ImmutableLongArray copyOf(long[] values) { + return values.length == 0 + ? EMPTY + : new ImmutableLongArray(Arrays.copyOf(values, values.length)); + } + + private final long[] array; + + private ImmutableLongArray(long[] array) { + this.array = array; + } + + /** Returns the number of values in this array. */ + public int length() { + return array.length; + } + + /** + * Returns the {@code long} value present at the given index. + * + * @throws IndexOutOfBoundsException if {@code index} is negative, or greater than or equal to + * {@link #length} + */ + public long get(int index) { + return array[index]; + } + + /** + * Returns {@code true} if {@code object} is an {@code ImmutableLongArray} containing the same + * values as this one, in the same order. + */ + @Override + public boolean equals(@Nullable Object object) { + if (object == this) { + return true; + } + if (!(object instanceof ImmutableLongArray)) { + return false; + } + ImmutableLongArray that = (ImmutableLongArray) object; + if (this.length() != that.length()) { + return false; + } + for (int i = 0; i < length(); i++) { + if (this.get(i) != that.get(i)) { + return false; + } + } + return true; + } + + /** Returns an unspecified hash code for the contents of this immutable array. */ + @Override + public int hashCode() { + int hash = 1; + for (long value : array) { + hash *= 31; + hash += (int) (value ^ (value >>> 32)); + } + return hash; + } + + /** + * Returns a string representation of this array in the same form as {@link + * Arrays#toString(long[])}, for example {@code "[1, 2, 3]"}. + */ + @Override + public String toString() { + if (length() == 0) { + return "[]"; + } + StringBuilder builder = new StringBuilder(length() * 5); // rough estimate is fine + builder.append('[').append(array[0]); + + for (int i = 1; i < length(); i++) { + builder.append(", ").append(array[i]); + } + builder.append(']'); + return builder.toString(); + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java index b76237b03e5..d0f9ed92779 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java @@ -7,8 +7,8 @@ import com.google.auto.value.AutoValue; import io.opentelemetry.api.common.Labels; -import java.util.List; -import java.util.function.BiConsumer; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; +import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import javax.annotation.concurrent.Immutable; /** @@ -19,11 +19,15 @@ @AutoValue public abstract class DoubleHistogramPointData implements PointData { /** - * Creates a DoubleHistogramPointData. It's the caller's responsibility to make sure that the - * `boundaries` and `counts` are unmodifiable. + * Functional interface for consuming bucket boundaries and counts as a sequence of pair values. + */ + public interface BucketConsumer { + void accept(double upperBound, long count); + } + + /** + * Creates a DoubleHistogramPointData. * - * @param boundaries the bucket boundaries in unmodifiable mode. - * @param counts the bucket count in unmodifiable mode. * @return a DoubleHistogramPointData. */ public static DoubleHistogramPointData create( @@ -32,8 +36,8 @@ public static DoubleHistogramPointData create( Labels labels, double sum, long count, - List boundaries, - List counts) { + ImmutableDoubleArray boundaries, + ImmutableLongArray counts) { return new AutoValue_DoubleHistogramPointData( startEpochNanos, epochNanos, labels, sum, count, boundaries, counts); } @@ -56,27 +60,26 @@ public static DoubleHistogramPointData create( /** * The bucket boundaries. For a Histogram with N defined boundaries, e.g, [x, y, z]. There are N+1 - * counts: [-inf, x), [x, y), [y, z), [z, +inf]. The returned object is unmodifiable so do not - * mutate it. + * counts: [-inf, x), [x, y), [y, z), [z, +inf]. * - * @return the unmodifiable bucket boundaries in increasing order. + * @return the bucket boundaries in increasing order. */ - public abstract List getBoundaries(); + public abstract ImmutableDoubleArray getBoundaries(); /** - * The counts in each bucket. The returned object is unmodifiable so do not mutate it. + * The counts in each bucket. * - * @return the unmodifiable counts in each bucket. + * @return the counts in each bucket. */ - public abstract List getCounts(); + public abstract ImmutableLongArray getCounts(); /** Iterates over all the bucket boundaries and counts in this histogram. */ - public void forEach(BiConsumer action) { - List boundaries = getBoundaries(); - List counts = getCounts(); - for (int i = 0; i < boundaries.size(); ++i) { + public void forEach(BucketConsumer action) { + ImmutableDoubleArray boundaries = getBoundaries(); + ImmutableLongArray counts = getCounts(); + for (int i = 0; i < boundaries.length(); ++i) { action.accept(boundaries.get(i), counts.get(i)); } - action.accept(Double.POSITIVE_INFINITY, counts.get(boundaries.size())); + action.accept(Double.POSITIVE_INFINITY, counts.get(boundaries.length())); } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java index 42a4408cf6d..c624b6ae5a0 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java @@ -8,6 +8,7 @@ import static org.assertj.core.api.Assertions.assertThat; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.common.InstrumentType; import io.opentelemetry.sdk.metrics.common.InstrumentValueType; @@ -128,7 +129,8 @@ void getSumAggregatorFactory() { @Test void getHistogramAggregatorFactory() { AggregatorFactory histogram = - AggregatorFactory.histogram(new double[] {1.0}, /* stateful= */ false); + AggregatorFactory.histogram( + ImmutableDoubleArray.copyOf(new double[] {1.0}), /* stateful= */ false); assertThat( histogram.create( Resource.getDefault(), @@ -166,7 +168,8 @@ void getHistogramAggregatorFactory() { .isStateful()) .isFalse(); assertThat( - AggregatorFactory.histogram(new double[] {1.0}, /* stateful= */ true) + AggregatorFactory.histogram( + ImmutableDoubleArray.copyOf(new double[] {1.0}), /* stateful= */ true) .create( Resource.getDefault(), InstrumentationLibraryInfo.getEmpty(), @@ -183,14 +186,18 @@ void getHistogramAggregatorFactory() { IllegalArgumentException.class, () -> AggregatorFactory.histogram( - new double[] {Double.NEGATIVE_INFINITY}, /* stateful= */ false)); + ImmutableDoubleArray.copyOf(new double[] {Double.NEGATIVE_INFINITY}), + /* stateful= */ false)); Assertions.assertThrows( IllegalArgumentException.class, () -> AggregatorFactory.histogram( - new double[] {1, Double.POSITIVE_INFINITY}, /* stateful= */ false)); + ImmutableDoubleArray.copyOf(new double[] {1, Double.POSITIVE_INFINITY}), + /* stateful= */ false)); Assertions.assertThrows( IllegalArgumentException.class, - () -> AggregatorFactory.histogram(new double[] {2, 1, 3}, /* stateful= */ false)); + () -> + AggregatorFactory.histogram( + ImmutableDoubleArray.copyOf(new double[] {2, 1, 3}), /* stateful= */ false)); } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java index 3d5591546ae..82456299dd0 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java @@ -7,10 +7,11 @@ import static org.assertj.core.api.Assertions.assertThat; -import com.google.common.collect.ImmutableList; import com.google.errorprone.annotations.concurrent.GuardedBy; import io.opentelemetry.api.common.Labels; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; +import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.common.InstrumentType; import io.opentelemetry.sdk.metrics.common.InstrumentValueType; @@ -28,8 +29,8 @@ import org.junit.jupiter.api.Test; public class DoubleHistogramAggregatorTest { - private static final ImmutableList BUCKET_BOUNDARIES = - ImmutableList.of(10.0, 100.0, 1000.0); + private static final ImmutableDoubleArray BUCKET_BOUNDARIES = + ImmutableDoubleArray.copyOf(new double[] {10.0, 100.0, 1000.0}); private static final DoubleHistogramAggregator aggregator = new DoubleHistogramAggregator( Resource.getDefault(), @@ -40,7 +41,7 @@ public class DoubleHistogramAggregatorTest { "unit", InstrumentType.VALUE_RECORDER, InstrumentValueType.LONG), - BUCKET_BOUNDARIES.stream().mapToDouble(i -> i).toArray(), + BUCKET_BOUNDARIES, /* stateful= */ false); @Test @@ -54,10 +55,11 @@ void testRecordings() { aggregatorHandle.recordLong(20); aggregatorHandle.recordLong(5); aggregatorHandle.recordLong(150); + aggregatorHandle.recordLong(2000); assertThat(aggregatorHandle.accumulateThenReset()) .isEqualTo( HistogramAccumulation.create( - 3, 175, BUCKET_BOUNDARIES, ImmutableList.of(1L, 1L, 1L, 0L))); + 4, 2175, BUCKET_BOUNDARIES, ImmutableLongArray.copyOf(new long[] {1, 1, 1, 1}))); } @Test @@ -69,26 +71,28 @@ void toAccumulationAndReset() { assertThat(aggregatorHandle.accumulateThenReset()) .isEqualTo( HistogramAccumulation.create( - 1, 100, BUCKET_BOUNDARIES, ImmutableList.of(0L, 0L, 1L, 0L))); + 1, 100, BUCKET_BOUNDARIES, ImmutableLongArray.copyOf(new long[] {0, 0, 1, 0}))); assertThat(aggregatorHandle.accumulateThenReset()).isNull(); aggregatorHandle.recordLong(0); assertThat(aggregatorHandle.accumulateThenReset()) .isEqualTo( HistogramAccumulation.create( - 1, 0, BUCKET_BOUNDARIES, ImmutableList.of(1L, 0L, 0L, 0L))); + 1, 0, BUCKET_BOUNDARIES, ImmutableLongArray.copyOf(new long[] {1, 0, 0, 0}))); assertThat(aggregatorHandle.accumulateThenReset()).isNull(); } @Test void invalidMergeReturnsEmptyAccumulation() { HistogramAccumulation x = - HistogramAccumulation.create(1, 1, ImmutableList.of(1.0), ImmutableList.of(0L, 1L)); + HistogramAccumulation.create( + 1, 1, ImmutableDoubleArray.of(1), ImmutableLongArray.copyOf(new long[] {0, 1})); HistogramAccumulation y = - HistogramAccumulation.create(1, 2, ImmutableList.of(2.0), ImmutableList.of(1L)); + HistogramAccumulation.create(1, 2, ImmutableDoubleArray.of(2), ImmutableLongArray.of(1)); assertThat(aggregator.merge(x, y)) .isEqualTo( - HistogramAccumulation.create(0, 0, Collections.emptyList(), ImmutableList.of(0L))); + HistogramAccumulation.create( + 0, 0, ImmutableDoubleArray.of(), ImmutableLongArray.of(0))); } @Test @@ -112,10 +116,12 @@ void toMetricData() { void accumulateData() { assertThat(aggregator.accumulateDouble(2.0)) .isEqualTo( - HistogramAccumulation.create(1, 2.0, Collections.emptyList(), ImmutableList.of(1L))); + HistogramAccumulation.create( + 1, 2.0, ImmutableDoubleArray.of(), ImmutableLongArray.of(1))); assertThat(aggregator.accumulateLong(10)) .isEqualTo( - HistogramAccumulation.create(1, 10.0, Collections.emptyList(), ImmutableList.of(1L))); + HistogramAccumulation.create( + 1, 10.0, ImmutableDoubleArray.of(), ImmutableLongArray.of(1))); } @Test @@ -164,7 +170,7 @@ void testMultithreadedUpdates() throws Exception { numberOfThreads * numberOfUpdates, 101000, BUCKET_BOUNDARIES, - ImmutableList.of(5000L, 5000L, 0L, 0L))); + ImmutableLongArray.copyOf(new long[] {5000, 5000, 0, 0}))); } private static final class Histogram { diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java index ec32f9f9c63..3c3e28a06be 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java @@ -9,6 +9,8 @@ import com.google.common.collect.ImmutableList; import io.opentelemetry.api.common.Labels; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; +import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import io.opentelemetry.sdk.metrics.data.DoubleHistogramPointData; import java.util.ArrayList; import java.util.List; @@ -18,12 +20,13 @@ public class HistogramAccumulationTest { @Test void toPoint() { HistogramAccumulation accumulation = - HistogramAccumulation.create(12, 25, ImmutableList.of(1.0), ImmutableList.of(1L, 2L)); + HistogramAccumulation.create( + 12, 25, ImmutableDoubleArray.of(1), ImmutableLongArray.copyOf(new long[] {1, 2})); DoubleHistogramPointData point = getPoint(accumulation); assertThat(point.getCount()).isEqualTo(12); assertThat(point.getSum()).isEqualTo(25); - assertThat(point.getBoundaries()).isEqualTo(ImmutableList.of(1.0)); - assertThat(point.getCounts()).isEqualTo(ImmutableList.of(1L, 2L)); + assertThat(point.getBoundaries()).isEqualTo(ImmutableDoubleArray.of(1)); + assertThat(point.getCounts()).isEqualTo(ImmutableLongArray.copyOf(new long[] {1, 2})); List boundaries = new ArrayList<>(); List counts = new ArrayList<>(); @@ -33,7 +36,7 @@ void toPoint() { counts.add(c); }); assertThat(boundaries).isEqualTo(ImmutableList.of(1.0, Double.POSITIVE_INFINITY)); - assertThat(counts).isEqualTo(point.getCounts()); + assertThat(counts).isEqualTo(ImmutableList.of(1L, 2L)); } private static DoubleHistogramPointData getPoint(HistogramAccumulation accumulation) { diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java index 5fc164e4946..dcf2275a2b0 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java @@ -7,8 +7,11 @@ import static org.assertj.core.api.Assertions.assertThat; +import com.google.common.collect.ImmutableList; import io.opentelemetry.api.common.Labels; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; +import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import io.opentelemetry.sdk.resources.Resource; import java.util.ArrayList; import java.util.Arrays; @@ -49,8 +52,8 @@ class MetricDataTest { Labels.of("key", "value"), DOUBLE_VALUE, LONG_VALUE, - Collections.singletonList(1.0), - Arrays.asList(1L, 1L)); + ImmutableDoubleArray.of(1), + ImmutableLongArray.copyOf(new long[] {1, 1})); @Test void metricData_Getters() { @@ -165,8 +168,8 @@ void metricData_HistogramPoints() { assertThat(HISTOGRAM_POINT.getLabels().get("key")).isEqualTo("value"); assertThat(HISTOGRAM_POINT.getCount()).isEqualTo(LONG_VALUE); assertThat(HISTOGRAM_POINT.getSum()).isEqualTo(DOUBLE_VALUE); - assertThat(HISTOGRAM_POINT.getBoundaries()).isEqualTo(Collections.singletonList(1.0)); - assertThat(HISTOGRAM_POINT.getCounts()).isEqualTo(Arrays.asList(1L, 1L)); + assertThat(HISTOGRAM_POINT.getBoundaries()).isEqualTo(ImmutableDoubleArray.of(1)); + assertThat(HISTOGRAM_POINT.getCounts()).isEqualTo(ImmutableLongArray.copyOf(new long[] {1, 1})); List boundaries = new ArrayList<>(); List counts = new ArrayList<>(); @@ -176,7 +179,7 @@ void metricData_HistogramPoints() { counts.add(c); }); assertThat(boundaries).isEqualTo(Arrays.asList(1.0, Double.POSITIVE_INFINITY)); - assertThat(counts).isEqualTo(HISTOGRAM_POINT.getCounts()); + assertThat(counts).isEqualTo(ImmutableList.of(1L, 1L)); MetricData metricData = MetricData.createDoubleHistogram( From e12b6c835b644e8ecd2579e64fed5a32d8402678 Mon Sep 17 00:00:00 2001 From: beanliu Date: Fri, 29 Jan 2021 10:26:07 +1100 Subject: [PATCH 11/17] fixup! use Immutable(Long|Double)Array for histogram aggregation --- .../sdk/metrics/aggregator/DoubleHistogramAggregator.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index 012a9bd157f..d470fbd7951 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -127,12 +127,12 @@ protected HistogramAccumulation doAccumulateThenReset() { lock.writeLock().unlock(); } - long total_count = 0; + long totalCount = 0; for (int i = 0; i < counts.length(); ++i) { - total_count += counts.get(i); + totalCount += counts.get(i); } - return HistogramAccumulation.create(total_count, sum, current.boundaries, counts); + return HistogramAccumulation.create(totalCount, sum, current.boundaries, counts); } @Override From d4c2a0d83bc53d107833d127e45d1cbde286c873 Mon Sep 17 00:00:00 2001 From: beanliu Date: Fri, 29 Jan 2021 10:47:09 +1100 Subject: [PATCH 12/17] fixup! use Immutable(Long|Double)Array for histogram aggregation --- .../sdk/metrics/common/ImmutableDoubleArray.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java index 7098a530660..7505eb14ee1 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java @@ -68,13 +68,18 @@ public boolean equals(@Nullable Object object) { return false; } for (int i = 0; i < length(); i++) { - if (this.get(i) != that.get(i)) { + if (!areEqual(this.get(i), that.get(i))) { return false; } } return true; } + // Match the behavior of Double.equals() + private static boolean areEqual(double a, double b) { + return Double.doubleToLongBits(a) == Double.doubleToLongBits(b); + } + /** Returns an unspecified hash code for the contents of this immutable array. */ @Override public int hashCode() { From ce83dbc648b5a2ee0e33e2e4f81894ce05c90a15 Mon Sep 17 00:00:00 2001 From: beanliu Date: Sat, 30 Jan 2021 10:17:58 +1100 Subject: [PATCH 13/17] fixup! use Immutable(Long|Double)Array for histogram aggregation --- .../aggregator/DoubleHistogramAggregator.java | 49 ++++++++++--------- .../metrics/common/ImmutableDoubleArray.java | 15 +----- .../metrics/common/ImmutableLongArray.java | 5 -- 3 files changed, 26 insertions(+), 43 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index d470fbd7951..3ac4fcf31f1 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -16,7 +16,7 @@ import io.opentelemetry.sdk.resources.Resource; import java.util.Arrays; import java.util.Map; -import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.concurrent.GuardedBy; @@ -105,26 +105,40 @@ public HistogramAccumulation accumulateLong(long value) { } static final class Handle extends AggregatorHandle { - private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + private final ImmutableDoubleArray boundaries; + + private final ReentrantLock lock = new ReentrantLock(); @GuardedBy("lock") private final State current; Handle(ImmutableDoubleArray boundaries) { - current = new State(boundaries); + this.boundaries = boundaries; + this.current = new State(this.boundaries.length() + 1); + } + + // Benchmark shows that linear search performs better than binary search with ordinary + // buckets. + private int findBucketIndex(double value) { + for (int i = 0; i < boundaries.length(); ++i) { + if (value < boundaries.get(i)) { + return i; + } + } + return boundaries.length(); } @Override protected HistogramAccumulation doAccumulateThenReset() { double sum; ImmutableLongArray counts; - lock.writeLock().lock(); + lock.lock(); try { sum = current.sum; counts = ImmutableLongArray.copyOf(current.counts); current.reset(); } finally { - lock.writeLock().unlock(); + lock.unlock(); } long totalCount = 0; @@ -132,18 +146,18 @@ protected HistogramAccumulation doAccumulateThenReset() { totalCount += counts.get(i); } - return HistogramAccumulation.create(totalCount, sum, current.boundaries, counts); + return HistogramAccumulation.create(totalCount, sum, boundaries, counts); } @Override protected void doRecordDouble(double value) { - int bucketIndex = current.findBucketIndex(value); + int bucketIndex = findBucketIndex(value); - lock.writeLock().lock(); + lock.lock(); try { current.record(bucketIndex, value); } finally { - lock.writeLock().unlock(); + lock.unlock(); } } @@ -154,26 +168,13 @@ protected void doRecordLong(long value) { private static final class State { private double sum; - private final ImmutableDoubleArray boundaries; private final long[] counts; - public State(ImmutableDoubleArray boundaries) { - this.boundaries = boundaries; - this.counts = new long[this.boundaries.length() + 1]; + public State(int bucketSize) { + this.counts = new long[bucketSize]; reset(); } - // Benchmark shows that linear search performs better than binary search with ordinary - // buckets. - private int findBucketIndex(double value) { - for (int i = 0; i < this.boundaries.length(); ++i) { - if (value < this.boundaries.get(i)) { - return i; - } - } - return this.boundaries.length(); - } - private void reset() { this.sum = 0; Arrays.fill(this.counts, 0); diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java index 7505eb14ee1..5a4bfde66ca 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java @@ -64,20 +64,7 @@ public boolean equals(@Nullable Object object) { return false; } ImmutableDoubleArray that = (ImmutableDoubleArray) object; - if (this.length() != that.length()) { - return false; - } - for (int i = 0; i < length(); i++) { - if (!areEqual(this.get(i), that.get(i))) { - return false; - } - } - return true; - } - - // Match the behavior of Double.equals() - private static boolean areEqual(double a, double b) { - return Double.doubleToLongBits(a) == Double.doubleToLongBits(b); + return Arrays.equals(this.array, that.array); } /** Returns an unspecified hash code for the contents of this immutable array. */ diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java index c1b9625006a..fe09e1f1af6 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java @@ -13,11 +13,6 @@ public class ImmutableLongArray { private static final ImmutableLongArray EMPTY = new ImmutableLongArray(new long[0]); - /** Returns the empty array. */ - public static ImmutableLongArray of() { - return EMPTY; - } - /** Returns an immutable array containing a single value. */ public static ImmutableLongArray of(long e0) { return new ImmutableLongArray(new long[] {e0}); From d46ba13bed9b859dd0133182e1fb638aa4602427 Mon Sep 17 00:00:00 2001 From: beanliu Date: Mon, 1 Feb 2021 09:39:05 +1100 Subject: [PATCH 14/17] fixup! use Immutable(Long|Double)Array for histogram aggregation --- .../sdk/metrics/common/ImmutableDoubleArray.java | 5 ----- .../metrics/aggregator/DoubleHistogramAggregatorTest.java | 8 ++++++-- .../sdk/metrics/aggregator/HistogramAccumulationTest.java | 7 +++++-- .../io/opentelemetry/sdk/metrics/data/MetricDataTest.java | 5 +++-- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java index 5a4bfde66ca..435a81e6217 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java @@ -18,11 +18,6 @@ public static ImmutableDoubleArray of() { return EMPTY; } - /** Returns an immutable array containing a single value. */ - public static ImmutableDoubleArray of(double e0) { - return new ImmutableDoubleArray(new double[] {e0}); - } - /** Returns an immutable array containing the given values, in order. */ public static ImmutableDoubleArray copyOf(double[] values) { return values.length == 0 diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java index 82456299dd0..09db9e86ff6 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java @@ -86,9 +86,13 @@ void toAccumulationAndReset() { void invalidMergeReturnsEmptyAccumulation() { HistogramAccumulation x = HistogramAccumulation.create( - 1, 1, ImmutableDoubleArray.of(1), ImmutableLongArray.copyOf(new long[] {0, 1})); + 1, + 1, + ImmutableDoubleArray.copyOf(new double[] {1}), + ImmutableLongArray.copyOf(new long[] {0, 1})); HistogramAccumulation y = - HistogramAccumulation.create(1, 2, ImmutableDoubleArray.of(2), ImmutableLongArray.of(1)); + HistogramAccumulation.create( + 1, 2, ImmutableDoubleArray.copyOf(new double[] {2}), ImmutableLongArray.of(1)); assertThat(aggregator.merge(x, y)) .isEqualTo( HistogramAccumulation.create( diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java index 3c3e28a06be..b185ecec01d 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java @@ -21,11 +21,14 @@ public class HistogramAccumulationTest { void toPoint() { HistogramAccumulation accumulation = HistogramAccumulation.create( - 12, 25, ImmutableDoubleArray.of(1), ImmutableLongArray.copyOf(new long[] {1, 2})); + 12, + 25, + ImmutableDoubleArray.copyOf(new double[] {1}), + ImmutableLongArray.copyOf(new long[] {1, 2})); DoubleHistogramPointData point = getPoint(accumulation); assertThat(point.getCount()).isEqualTo(12); assertThat(point.getSum()).isEqualTo(25); - assertThat(point.getBoundaries()).isEqualTo(ImmutableDoubleArray.of(1)); + assertThat(point.getBoundaries()).isEqualTo(ImmutableDoubleArray.copyOf(new double[] {1})); assertThat(point.getCounts()).isEqualTo(ImmutableLongArray.copyOf(new long[] {1, 2})); List boundaries = new ArrayList<>(); diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java index dcf2275a2b0..24ddf883e3b 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java @@ -52,7 +52,7 @@ class MetricDataTest { Labels.of("key", "value"), DOUBLE_VALUE, LONG_VALUE, - ImmutableDoubleArray.of(1), + ImmutableDoubleArray.copyOf(new double[] {1}), ImmutableLongArray.copyOf(new long[] {1, 1})); @Test @@ -168,7 +168,8 @@ void metricData_HistogramPoints() { assertThat(HISTOGRAM_POINT.getLabels().get("key")).isEqualTo("value"); assertThat(HISTOGRAM_POINT.getCount()).isEqualTo(LONG_VALUE); assertThat(HISTOGRAM_POINT.getSum()).isEqualTo(DOUBLE_VALUE); - assertThat(HISTOGRAM_POINT.getBoundaries()).isEqualTo(ImmutableDoubleArray.of(1)); + assertThat(HISTOGRAM_POINT.getBoundaries()) + .isEqualTo(ImmutableDoubleArray.copyOf(new double[] {1})); assertThat(HISTOGRAM_POINT.getCounts()).isEqualTo(ImmutableLongArray.copyOf(new long[] {1, 1})); List boundaries = new ArrayList<>(); From 09dd32b42a3c841c40333261cff4487c96a2d52d Mon Sep 17 00:00:00 2001 From: beanliu Date: Tue, 23 Feb 2021 09:56:15 +1100 Subject: [PATCH 15/17] remove ImmutableDoubleArray.of() --- .../sdk/metrics/aggregator/DoubleHistogramAggregator.java | 6 +++--- .../sdk/metrics/common/ImmutableDoubleArray.java | 5 ----- .../metrics/aggregator/DoubleHistogramAggregatorTest.java | 6 +++--- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index 3ac4fcf31f1..bab7562cb16 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -58,7 +58,7 @@ public final HistogramAccumulation merge(HistogramAccumulation x, HistogramAccum loggedMergingInvalidBoundaries = true; } return HistogramAccumulation.create( - 0, 0, ImmutableDoubleArray.of(), ImmutableLongArray.of(0)); + 0, 0, ImmutableDoubleArray.copyOf(new double[0]), ImmutableLongArray.of(0)); } long[] mergedCounts = new long[x.getCounts().length()]; @@ -95,13 +95,13 @@ public final MetricData toMetricData( @Override public HistogramAccumulation accumulateDouble(double value) { return HistogramAccumulation.create( - 1, value, ImmutableDoubleArray.of(), ImmutableLongArray.of(1)); + 1, value, ImmutableDoubleArray.copyOf(new double[0]), ImmutableLongArray.of(1)); } @Override public HistogramAccumulation accumulateLong(long value) { return HistogramAccumulation.create( - 1, value, ImmutableDoubleArray.of(), ImmutableLongArray.of(1)); + 1, value, ImmutableDoubleArray.copyOf(new double[0]), ImmutableLongArray.of(1)); } static final class Handle extends AggregatorHandle { diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java index 435a81e6217..ed9f6507eab 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java @@ -13,11 +13,6 @@ public class ImmutableDoubleArray { private static final ImmutableDoubleArray EMPTY = new ImmutableDoubleArray(new double[0]); - /** Returns the empty array. */ - public static ImmutableDoubleArray of() { - return EMPTY; - } - /** Returns an immutable array containing the given values, in order. */ public static ImmutableDoubleArray copyOf(double[] values) { return values.length == 0 diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java index 09db9e86ff6..92560a55302 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java @@ -96,7 +96,7 @@ void invalidMergeReturnsEmptyAccumulation() { assertThat(aggregator.merge(x, y)) .isEqualTo( HistogramAccumulation.create( - 0, 0, ImmutableDoubleArray.of(), ImmutableLongArray.of(0))); + 0, 0, ImmutableDoubleArray.copyOf(new double[0]), ImmutableLongArray.of(0))); } @Test @@ -121,11 +121,11 @@ void accumulateData() { assertThat(aggregator.accumulateDouble(2.0)) .isEqualTo( HistogramAccumulation.create( - 1, 2.0, ImmutableDoubleArray.of(), ImmutableLongArray.of(1))); + 1, 2.0, ImmutableDoubleArray.copyOf(new double[0]), ImmutableLongArray.of(1))); assertThat(aggregator.accumulateLong(10)) .isEqualTo( HistogramAccumulation.create( - 1, 10.0, ImmutableDoubleArray.of(), ImmutableLongArray.of(1))); + 1, 10.0, ImmutableDoubleArray.copyOf(new double[0]), ImmutableLongArray.of(1))); } @Test From 7696284a9d15e729d7e72fb9f592c9f4fa8cc351 Mon Sep 17 00:00:00 2001 From: beanliu Date: Tue, 23 Feb 2021 10:48:46 +1100 Subject: [PATCH 16/17] remove boundaries from HistogramAccumulation --- .../aggregator/DoubleHistogramAggregator.java | 37 ++++--------- .../aggregator/HistogramAccumulation.java | 22 +------- .../metrics/aggregator/MetricDataUtils.java | 15 ++++- .../aggregator/HistogramAccumulationTest.java | 55 ------------------- 4 files changed, 26 insertions(+), 103 deletions(-) delete mode 100644 sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index bab7562cb16..6011a569c90 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -17,15 +17,9 @@ import java.util.Arrays; import java.util.Map; import java.util.concurrent.locks.ReentrantLock; -import java.util.logging.Level; -import java.util.logging.Logger; import javax.annotation.concurrent.GuardedBy; final class DoubleHistogramAggregator extends AbstractAggregator { - private static final Logger logger = Logger.getLogger(DoubleHistogramAggregator.class.getName()); - - private static volatile boolean loggedMergingInvalidBoundaries = false; - private final ImmutableDoubleArray boundaries; DoubleHistogramAggregator( @@ -43,24 +37,13 @@ public AggregatorHandle createHandle() { return new Handle(this.boundaries); } + /** + * Return the result of the merge of two histogram accumulations. + * As long as one Aggregator instance produces all Accumulations with constant boundaries we + * don't need to worry about merging accumulations with different boundaries. + */ @Override public final HistogramAccumulation merge(HistogramAccumulation x, HistogramAccumulation y) { - if (!x.getBoundaries().equals(y.getBoundaries())) { - // If this happens, it's a pretty severe bug in the SDK. - if (!loggedMergingInvalidBoundaries) { - logger.log( - Level.SEVERE, - "can't merge histograms with different boundaries, something's very wrong: " - + "x.boundaries=" - + x.getBoundaries() - + " y.boundaries=" - + y.getBoundaries()); - loggedMergingInvalidBoundaries = true; - } - return HistogramAccumulation.create( - 0, 0, ImmutableDoubleArray.copyOf(new double[0]), ImmutableLongArray.of(0)); - } - long[] mergedCounts = new long[x.getCounts().length()]; for (int i = 0; i < x.getCounts().length(); ++i) { mergedCounts[i] = x.getCounts().get(i) + y.getCounts().get(i); @@ -68,7 +51,6 @@ public final HistogramAccumulation merge(HistogramAccumulation x, HistogramAccum return HistogramAccumulation.create( x.getCount() + y.getCount(), x.getSum() + y.getSum(), - x.getBoundaries(), ImmutableLongArray.copyOf(mergedCounts)); } @@ -89,19 +71,20 @@ public final MetricData toMetricData( MetricDataUtils.toDoubleHistogramPointList( accumulationByLabels, isStateful() ? startEpochNanos : lastCollectionEpoch, - epochNanos))); + epochNanos, + boundaries))); } @Override public HistogramAccumulation accumulateDouble(double value) { return HistogramAccumulation.create( - 1, value, ImmutableDoubleArray.copyOf(new double[0]), ImmutableLongArray.of(1)); + 1, value, ImmutableLongArray.of(1)); } @Override public HistogramAccumulation accumulateLong(long value) { return HistogramAccumulation.create( - 1, value, ImmutableDoubleArray.copyOf(new double[0]), ImmutableLongArray.of(1)); + 1, value, ImmutableLongArray.of(1)); } static final class Handle extends AggregatorHandle { @@ -146,7 +129,7 @@ protected HistogramAccumulation doAccumulateThenReset() { totalCount += counts.get(i); } - return HistogramAccumulation.create(totalCount, sum, boundaries, counts); + return HistogramAccumulation.create(totalCount, sum, counts); } @Override diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java index 41ffe3de139..2dab512ce73 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java @@ -6,23 +6,20 @@ package io.opentelemetry.sdk.metrics.aggregator; import com.google.auto.value.AutoValue; -import io.opentelemetry.api.common.Labels; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; -import io.opentelemetry.sdk.metrics.data.DoubleHistogramPointData; import javax.annotation.concurrent.Immutable; @Immutable @AutoValue -public abstract class HistogramAccumulation { +abstract class HistogramAccumulation { /** * Creates a new {@link HistogramAccumulation} with the given values. * * @return a new {@link HistogramAccumulation} with the given values. */ static HistogramAccumulation create( - long count, double sum, ImmutableDoubleArray boundaries, ImmutableLongArray counts) { - return new AutoValue_HistogramAccumulation(count, sum, boundaries, counts); + long count, double sum, ImmutableLongArray counts) { + return new AutoValue_HistogramAccumulation(count, sum, counts); } HistogramAccumulation() {} @@ -41,23 +38,10 @@ static HistogramAccumulation create( */ abstract double getSum(); - /** - * The bucket boundaries. For a Histogram with N defined boundaries, e.g, [x, y, z]. There are N+1 - * counts: [-inf, x), [x, y), [y, z), [z, +inf]. - * - * @return the bucket boundaries in increasing order. - */ - abstract ImmutableDoubleArray getBoundaries(); - /** * The counts in each bucket. * * @return the counts in each bucket. */ abstract ImmutableLongArray getCounts(); - - final DoubleHistogramPointData toPoint(long startEpochNanos, long epochNanos, Labels labels) { - return DoubleHistogramPointData.create( - startEpochNanos, epochNanos, labels, getSum(), getCount(), getBoundaries(), getCounts()); - } } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java index 8efa36e203f..5308469c0d6 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java @@ -6,6 +6,7 @@ package io.opentelemetry.sdk.metrics.aggregator; import io.opentelemetry.api.common.Labels; +import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.data.DoubleHistogramPointData; import io.opentelemetry.sdk.metrics.data.DoublePointData; import io.opentelemetry.sdk.metrics.data.DoubleSummaryPointData; @@ -47,11 +48,21 @@ static List toDoubleSummaryPointList( } static List toDoubleHistogramPointList( - Map accumulationMap, long startEpochNanos, long epochNanos) { + Map accumulationMap, + long startEpochNanos, + long epochNanos, + ImmutableDoubleArray boundaries) { List points = new ArrayList<>(accumulationMap.size()); accumulationMap.forEach( (labels, aggregator) -> - points.add(aggregator.toPoint(startEpochNanos, epochNanos, labels))); + points.add(DoubleHistogramPointData.create( + startEpochNanos, + epochNanos, + labels, + aggregator.getSum(), + aggregator.getCount(), + boundaries, + aggregator.getCounts()))); return points; } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java deleted file mode 100644 index b185ecec01d..00000000000 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulationTest.java +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.sdk.metrics.aggregator; - -import static org.assertj.core.api.Assertions.assertThat; - -import com.google.common.collect.ImmutableList; -import io.opentelemetry.api.common.Labels; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; -import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; -import io.opentelemetry.sdk.metrics.data.DoubleHistogramPointData; -import java.util.ArrayList; -import java.util.List; -import org.junit.jupiter.api.Test; - -public class HistogramAccumulationTest { - @Test - void toPoint() { - HistogramAccumulation accumulation = - HistogramAccumulation.create( - 12, - 25, - ImmutableDoubleArray.copyOf(new double[] {1}), - ImmutableLongArray.copyOf(new long[] {1, 2})); - DoubleHistogramPointData point = getPoint(accumulation); - assertThat(point.getCount()).isEqualTo(12); - assertThat(point.getSum()).isEqualTo(25); - assertThat(point.getBoundaries()).isEqualTo(ImmutableDoubleArray.copyOf(new double[] {1})); - assertThat(point.getCounts()).isEqualTo(ImmutableLongArray.copyOf(new long[] {1, 2})); - - List boundaries = new ArrayList<>(); - List counts = new ArrayList<>(); - point.forEach( - (b, c) -> { - boundaries.add(b); - counts.add(c); - }); - assertThat(boundaries).isEqualTo(ImmutableList.of(1.0, Double.POSITIVE_INFINITY)); - assertThat(counts).isEqualTo(ImmutableList.of(1L, 2L)); - } - - private static DoubleHistogramPointData getPoint(HistogramAccumulation accumulation) { - DoubleHistogramPointData point = accumulation.toPoint(12345, 12358, Labels.of("key", "value")); - assertThat(point).isNotNull(); - assertThat(point.getStartEpochNanos()).isEqualTo(12345); - assertThat(point.getEpochNanos()).isEqualTo(12358); - assertThat(point.getLabels().size()).isEqualTo(1); - assertThat(point.getLabels().get("key")).isEqualTo("value"); - assertThat(point).isInstanceOf(DoubleHistogramPointData.class); - return point; - } -} From 34ef27b6a6f72820d97773c423f26aa8a619c1fe Mon Sep 17 00:00:00 2001 From: beanliu Date: Tue, 23 Feb 2021 14:28:53 +1100 Subject: [PATCH 17/17] move ImmutableDoubleArray and ImmutableLongArray to sdk.metrics.aggregator and mark them non-public --- .../aggregator/DoubleHistogramBenchmark.java | 4 +-- .../metrics/aggregator/AggregatorFactory.java | 3 +- .../aggregator/DoubleHistogramAggregator.java | 4 +-- .../aggregator/HistogramAccumulation.java | 1 - .../HistogramAggregatorFactory.java | 23 +++++++------ .../ImmutableDoubleArray.java | 15 ++++++-- .../ImmutableLongArray.java | 15 ++++++-- .../metrics/aggregator/MetricDataUtils.java | 5 ++- .../data/DoubleHistogramPointData.java | 34 ++++++++++++------- .../aggregator/AggregatorFactoryTest.java | 17 +++------- .../DoubleHistogramAggregatorTest.java | 2 -- .../sdk/metrics/data/MetricDataTest.java | 11 +++--- 12 files changed, 74 insertions(+), 60 deletions(-) rename sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/{common => aggregator}/ImmutableDoubleArray.java (86%) rename sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/{common => aggregator}/ImmutableLongArray.java (88%) diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java index ce93d563e14..f70ce64f656 100644 --- a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java +++ b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramBenchmark.java @@ -6,7 +6,6 @@ package io.opentelemetry.sdk.metrics.aggregator; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.common.InstrumentType; import io.opentelemetry.sdk.metrics.common.InstrumentValueType; @@ -26,8 +25,7 @@ @State(Scope.Benchmark) public class DoubleHistogramBenchmark { private static final Aggregator aggregator = - AggregatorFactory.histogram( - ImmutableDoubleArray.copyOf(new double[] {10, 100, 1_000}), /* stateful= */ false) + AggregatorFactory.histogram(new double[] {10, 100, 1_000}, /* stateful= */ false) .create( Resource.getDefault(), InstrumentationLibraryInfo.empty(), diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java index 84649669948..5ae686e19c8 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactory.java @@ -6,7 +6,6 @@ package io.opentelemetry.sdk.metrics.aggregator; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.data.AggregationTemporality; import io.opentelemetry.sdk.resources.Resource; @@ -54,7 +53,7 @@ static AggregatorFactory count(AggregationTemporality temporality) { * @param boundaries configures the fixed bucket boundaries. * @return an {@code AggregationFactory} that calculates histogram of recorded measurements. */ - static AggregatorFactory histogram(ImmutableDoubleArray boundaries, boolean stateful) { + static AggregatorFactory histogram(double[] boundaries, boolean stateful) { return new HistogramAggregatorFactory(boundaries, stateful); } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java index 84515368ee7..e0430a7c9f6 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregator.java @@ -7,8 +7,6 @@ import io.opentelemetry.api.metrics.common.Labels; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; -import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.data.AggregationTemporality; import io.opentelemetry.sdk.metrics.data.DoubleHistogramData; @@ -72,7 +70,7 @@ public final MetricData toMetricData( accumulationByLabels, isStateful() ? startEpochNanos : lastCollectionEpoch, epochNanos, - boundaries))); + boundaries.toList()))); } @Override diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java index 23f5591dc1f..e7bbc7de529 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAccumulation.java @@ -6,7 +6,6 @@ package io.opentelemetry.sdk.metrics.aggregator; import com.google.auto.value.AutoValue; -import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import javax.annotation.concurrent.Immutable; @Immutable diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java index 8ac37000676..49c3e40bf88 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/HistogramAggregatorFactory.java @@ -6,7 +6,6 @@ package io.opentelemetry.sdk.metrics.aggregator; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.resources.Resource; @@ -14,23 +13,27 @@ final class HistogramAggregatorFactory implements AggregatorFactory { private final ImmutableDoubleArray boundaries; private final boolean stateful; - HistogramAggregatorFactory(ImmutableDoubleArray boundaries, boolean stateful) { - for (int i = 1; i < boundaries.length(); ++i) { - if (Double.compare(boundaries.get(i - 1), boundaries.get(i)) >= 0) { + HistogramAggregatorFactory(double[] boundaries, boolean stateful) { + this.boundaries = ImmutableDoubleArray.copyOf(boundaries); + this.stateful = stateful; + + for (int i = 1; i < this.boundaries.length(); ++i) { + if (Double.compare(this.boundaries.get(i - 1), this.boundaries.get(i)) >= 0) { throw new IllegalArgumentException( - "invalid bucket boundary: " + boundaries.get(i - 1) + " >= " + boundaries.get(i)); + "invalid bucket boundary: " + + this.boundaries.get(i - 1) + + " >= " + + this.boundaries.get(i)); } } - if (boundaries.length() > 0) { - if (boundaries.get(0) == Double.NEGATIVE_INFINITY) { + if (this.boundaries.length() > 0) { + if (this.boundaries.get(0) == Double.NEGATIVE_INFINITY) { throw new IllegalArgumentException("invalid bucket boundary: -Inf"); } - if (boundaries.get(boundaries.length() - 1) == Double.POSITIVE_INFINITY) { + if (this.boundaries.get(this.boundaries.length() - 1) == Double.POSITIVE_INFINITY) { throw new IllegalArgumentException("invalid bucket boundary: +Inf"); } } - this.boundaries = boundaries; - this.stateful = stateful; } @Override diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/ImmutableDoubleArray.java similarity index 86% rename from sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java rename to sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/ImmutableDoubleArray.java index ed9f6507eab..22c9f146bbc 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableDoubleArray.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/ImmutableDoubleArray.java @@ -3,14 +3,16 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.sdk.metrics.common; +package io.opentelemetry.sdk.metrics.aggregator; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; @Immutable -public class ImmutableDoubleArray { +class ImmutableDoubleArray { private static final ImmutableDoubleArray EMPTY = new ImmutableDoubleArray(new double[0]); /** Returns an immutable array containing the given values, in order. */ @@ -26,6 +28,15 @@ private ImmutableDoubleArray(double[] array) { this.array = array; } + /** Returns a copy of the underlying data as list. */ + public List toList() { + List result = new ArrayList<>(array.length); + for (double v : array) { + result.add(v); + } + return result; + } + /** Returns the number of values in this array. */ public int length() { return array.length; diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/ImmutableLongArray.java similarity index 88% rename from sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java rename to sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/ImmutableLongArray.java index fe09e1f1af6..e1ec8a1db60 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/common/ImmutableLongArray.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/ImmutableLongArray.java @@ -3,14 +3,16 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.sdk.metrics.common; +package io.opentelemetry.sdk.metrics.aggregator; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; @Immutable -public class ImmutableLongArray { +class ImmutableLongArray { private static final ImmutableLongArray EMPTY = new ImmutableLongArray(new long[0]); /** Returns an immutable array containing a single value. */ @@ -31,6 +33,15 @@ private ImmutableLongArray(long[] array) { this.array = array; } + /** Returns a copy of the underlying data as list. */ + public List toList() { + List result = new ArrayList<>(array.length); + for (long v : array) { + result.add(v); + } + return result; + } + /** Returns the number of values in this array. */ public int length() { return array.length; diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java index f0af32a28c4..973b5470fbd 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java @@ -6,7 +6,6 @@ package io.opentelemetry.sdk.metrics.aggregator; import io.opentelemetry.api.metrics.common.Labels; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.data.DoubleHistogramPointData; import io.opentelemetry.sdk.metrics.data.DoublePointData; import io.opentelemetry.sdk.metrics.data.DoubleSummaryPointData; @@ -51,7 +50,7 @@ static List toDoubleHistogramPointList( Map accumulationMap, long startEpochNanos, long epochNanos, - ImmutableDoubleArray boundaries) { + List boundaries) { List points = new ArrayList<>(accumulationMap.size()); accumulationMap.forEach( (labels, aggregator) -> @@ -63,7 +62,7 @@ static List toDoubleHistogramPointList( aggregator.getSum(), aggregator.getCount(), boundaries, - aggregator.getCounts()))); + aggregator.getCounts().toList()))); return points; } } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java index 3f614b0a99e..4b6d16f8c3e 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoubleHistogramPointData.java @@ -7,8 +7,9 @@ import com.google.auto.value.AutoValue; import io.opentelemetry.api.metrics.common.Labels; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; -import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import javax.annotation.concurrent.Immutable; /** @@ -36,10 +37,16 @@ public static DoubleHistogramPointData create( Labels labels, double sum, long count, - ImmutableDoubleArray boundaries, - ImmutableLongArray counts) { + List boundaries, + List counts) { return new AutoValue_DoubleHistogramPointData( - startEpochNanos, epochNanos, labels, sum, count, boundaries, counts); + startEpochNanos, + epochNanos, + labels, + sum, + count, + Collections.unmodifiableList(new ArrayList<>(boundaries)), + Collections.unmodifiableList(new ArrayList<>(counts))); } DoubleHistogramPointData() {} @@ -62,24 +69,25 @@ public static DoubleHistogramPointData create( * The bucket boundaries. For a Histogram with N defined boundaries, e.g, [x, y, z]. There are N+1 * counts: [-inf, x), [x, y), [y, z), [z, +inf]. * - * @return the bucket boundaries in increasing order. + * @return the read-only bucket boundaries in increasing order. do not mutate the returned + * object. */ - public abstract ImmutableDoubleArray getBoundaries(); + public abstract List getBoundaries(); /** * The counts in each bucket. * - * @return the counts in each bucket. + * @return the read-only counts in each bucket. do not mutate the returned object. */ - public abstract ImmutableLongArray getCounts(); + public abstract List getCounts(); /** Iterates over all the bucket boundaries and counts in this histogram. */ public void forEach(BucketConsumer action) { - ImmutableDoubleArray boundaries = getBoundaries(); - ImmutableLongArray counts = getCounts(); - for (int i = 0; i < boundaries.length(); ++i) { + List boundaries = getBoundaries(); + List counts = getCounts(); + for (int i = 0; i < boundaries.size(); ++i) { action.accept(boundaries.get(i), counts.get(i)); } - action.accept(Double.POSITIVE_INFINITY, counts.get(boundaries.length())); + action.accept(Double.POSITIVE_INFINITY, counts.get(boundaries.size())); } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java index 38ca6c00247..b0a3cc12c1c 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/AggregatorFactoryTest.java @@ -8,7 +8,6 @@ import static org.assertj.core.api.Assertions.assertThat; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.common.InstrumentType; import io.opentelemetry.sdk.metrics.common.InstrumentValueType; @@ -129,8 +128,7 @@ void getSumAggregatorFactory() { @Test void getHistogramAggregatorFactory() { AggregatorFactory histogram = - AggregatorFactory.histogram( - ImmutableDoubleArray.copyOf(new double[] {1.0}), /* stateful= */ false); + AggregatorFactory.histogram(new double[] {1.0}, /* stateful= */ false); assertThat( histogram.create( Resource.getDefault(), @@ -168,8 +166,7 @@ void getHistogramAggregatorFactory() { .isStateful()) .isFalse(); assertThat( - AggregatorFactory.histogram( - ImmutableDoubleArray.copyOf(new double[] {1.0}), /* stateful= */ true) + AggregatorFactory.histogram(new double[] {1.0}, /* stateful= */ true) .create( Resource.getDefault(), InstrumentationLibraryInfo.empty(), @@ -186,18 +183,14 @@ void getHistogramAggregatorFactory() { IllegalArgumentException.class, () -> AggregatorFactory.histogram( - ImmutableDoubleArray.copyOf(new double[] {Double.NEGATIVE_INFINITY}), - /* stateful= */ false)); + new double[] {Double.NEGATIVE_INFINITY}, /* stateful= */ false)); Assertions.assertThrows( IllegalArgumentException.class, () -> AggregatorFactory.histogram( - ImmutableDoubleArray.copyOf(new double[] {1, Double.POSITIVE_INFINITY}), - /* stateful= */ false)); + new double[] {1, Double.POSITIVE_INFINITY}, /* stateful= */ false)); Assertions.assertThrows( IllegalArgumentException.class, - () -> - AggregatorFactory.histogram( - ImmutableDoubleArray.copyOf(new double[] {2, 1, 3}), /* stateful= */ false)); + () -> AggregatorFactory.histogram(new double[] {2, 1, 3}, /* stateful= */ false)); } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java index e77c50e1ba5..9a2bda5e57c 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/aggregator/DoubleHistogramAggregatorTest.java @@ -10,8 +10,6 @@ import com.google.errorprone.annotations.concurrent.GuardedBy; import io.opentelemetry.api.metrics.common.Labels; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; -import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import io.opentelemetry.sdk.metrics.common.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.common.InstrumentType; import io.opentelemetry.sdk.metrics.common.InstrumentValueType; diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java index 3f655ad8485..e89013a2f30 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/MetricDataTest.java @@ -10,8 +10,6 @@ import com.google.common.collect.ImmutableList; import io.opentelemetry.api.metrics.common.Labels; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; -import io.opentelemetry.sdk.metrics.common.ImmutableDoubleArray; -import io.opentelemetry.sdk.metrics.common.ImmutableLongArray; import io.opentelemetry.sdk.resources.Resource; import java.util.ArrayList; import java.util.Arrays; @@ -52,8 +50,8 @@ class MetricDataTest { Labels.of("key", "value"), DOUBLE_VALUE, LONG_VALUE, - ImmutableDoubleArray.copyOf(new double[] {1}), - ImmutableLongArray.copyOf(new long[] {1, 1})); + ImmutableList.of(1.0), + ImmutableList.of(1L, 1L)); @Test void metricData_Getters() { @@ -168,9 +166,8 @@ void metricData_HistogramPoints() { assertThat(HISTOGRAM_POINT.getLabels().get("key")).isEqualTo("value"); assertThat(HISTOGRAM_POINT.getCount()).isEqualTo(LONG_VALUE); assertThat(HISTOGRAM_POINT.getSum()).isEqualTo(DOUBLE_VALUE); - assertThat(HISTOGRAM_POINT.getBoundaries()) - .isEqualTo(ImmutableDoubleArray.copyOf(new double[] {1})); - assertThat(HISTOGRAM_POINT.getCounts()).isEqualTo(ImmutableLongArray.copyOf(new long[] {1, 1})); + assertThat(HISTOGRAM_POINT.getBoundaries()).isEqualTo(ImmutableList.of(1.0)); + assertThat(HISTOGRAM_POINT.getCounts()).isEqualTo(ImmutableList.of(1L, 1L)); List boundaries = new ArrayList<>(); List counts = new ArrayList<>();