From 50499e4a29f6fe331fdb07affa6814d85aa7481e Mon Sep 17 00:00:00 2001 From: Asaf Mesika Date: Tue, 30 Jan 2024 11:08:28 +0200 Subject: [PATCH 1/2] All done --- ...trumentGarbageCollectionBenchmarkTest.java | 6 +- .../internal/state/TestInstrumentType.java | 31 ++++- .../state/tester/DoubleSumTester.java | 58 +++++++++ .../internal/state/tester/LongSumTester.java | 58 +++++++++ .../aggregator/DoubleSumAggregator.java | 25 +++- .../aggregator/LongSumAggregator.java | 25 +++- .../internal/data/MutableDoublePointData.java | 14 +-- .../internal/data/MutableLongPointData.java | 14 +-- .../metrics/internal/view/SumAggregation.java | 6 +- .../aggregator/DoubleSumAggregatorTest.java | 111 ++++++++++++------ .../aggregator/LongSumAggregatorTest.java | 110 +++++++++++------ 11 files changed, 363 insertions(+), 95 deletions(-) create mode 100644 sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/tester/DoubleSumTester.java create mode 100644 sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/tester/LongSumTester.java diff --git a/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/InstrumentGarbageCollectionBenchmarkTest.java b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/InstrumentGarbageCollectionBenchmarkTest.java index de1733b0ad8..63de98ef5f3 100644 --- a/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/InstrumentGarbageCollectionBenchmarkTest.java +++ b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/InstrumentGarbageCollectionBenchmarkTest.java @@ -108,6 +108,10 @@ public void normalizedAllocationRateTest() throws RunnerException { assertThat(immutableDataAllocRate).isNotNull().isNotZero(); assertThat(reusableDataAllocRate).isNotNull().isNotZero(); + float dataAllocRateReductionPercentage = + TestInstrumentType.valueOf(testInstrumentType) + .getDataAllocRateReductionPercentage(); + // If this test suddenly fails for you this means you have changed the code in a way // that allocates more memory than before. You can find out where, by running // ProfileBenchmark class and looking at the flame graph. Make sure to @@ -116,7 +120,7 @@ public void normalizedAllocationRateTest() throws RunnerException { .describedAs( "Aggregation temporality = %s, testInstrumentType = %s", aggregationTemporality, testInstrumentType) - .isCloseTo(99.8, Offset.offset(2.0)); + .isCloseTo(dataAllocRateReductionPercentage, Offset.offset(2.0)); }); }); } diff --git a/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/TestInstrumentType.java b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/TestInstrumentType.java index 9f799d72894..f2c89bb8fba 100644 --- a/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/TestInstrumentType.java +++ b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/TestInstrumentType.java @@ -9,8 +9,10 @@ import io.opentelemetry.sdk.metrics.Aggregation; import io.opentelemetry.sdk.metrics.SdkMeterProvider; import io.opentelemetry.sdk.metrics.internal.state.tester.AsyncCounterTester; +import io.opentelemetry.sdk.metrics.internal.state.tester.DoubleSumTester; import io.opentelemetry.sdk.metrics.internal.state.tester.ExplicitBucketHistogramTester; import io.opentelemetry.sdk.metrics.internal.state.tester.ExponentialHistogramTester; +import io.opentelemetry.sdk.metrics.internal.state.tester.LongSumTester; import java.util.List; import java.util.Random; @@ -32,11 +34,36 @@ InstrumentTester createInstrumentTester() { InstrumentTester createInstrumentTester() { return new ExplicitBucketHistogramTester(); } + }, + LONG_SUM(/* dataAllocRateReductionPercentage= */ 97.3f) { + @Override + InstrumentTester createInstrumentTester() { + return new LongSumTester(); + } + }, + DOUBLE_SUM(/* dataAllocRateReductionPercentage= */ 97.3f) { + @Override + InstrumentTester createInstrumentTester() { + return new DoubleSumTester(); + } }; - abstract InstrumentTester createInstrumentTester(); + private final float dataAllocRateReductionPercentage; + + TestInstrumentType() { + this.dataAllocRateReductionPercentage = 99.8f; // default + } + + // Some instruments have different reduction percentage. + TestInstrumentType(float dataAllocRateReductionPercentage) { + this.dataAllocRateReductionPercentage = dataAllocRateReductionPercentage; + } + + float getDataAllocRateReductionPercentage() { + return dataAllocRateReductionPercentage; + } - TestInstrumentType() {} + abstract InstrumentTester createInstrumentTester(); public interface InstrumentTester { Aggregation testedAggregation(); diff --git a/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/tester/DoubleSumTester.java b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/tester/DoubleSumTester.java new file mode 100644 index 00000000000..47cf4e4023e --- /dev/null +++ b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/tester/DoubleSumTester.java @@ -0,0 +1,58 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.internal.state.tester; + +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.metrics.DoubleCounter; +import io.opentelemetry.api.metrics.Meter; +import io.opentelemetry.sdk.metrics.Aggregation; +import io.opentelemetry.sdk.metrics.SdkMeterProvider; +import io.opentelemetry.sdk.metrics.internal.state.TestInstrumentType; +import java.util.List; +import java.util.Random; + +public class DoubleSumTester implements TestInstrumentType.InstrumentTester { + private static final int measurementsPerAttributeSet = 1_000; + + static class DoubleSumState implements TestInstrumentType.TestInstrumentsState { + DoubleCounter doubleCounter; + } + + @Override + public Aggregation testedAggregation() { + return Aggregation.sum(); + } + + @Override + public TestInstrumentType.TestInstrumentsState buildInstruments( + double instrumentCount, + SdkMeterProvider sdkMeterProvider, + List attributesList, + Random random) { + DoubleSumState doubleSumState = new DoubleSumState(); + + Meter meter = sdkMeterProvider.meterBuilder("meter").build(); + doubleSumState.doubleCounter = meter.counterBuilder("test.double.sum").ofDoubles().build(); + + return doubleSumState; + } + + @SuppressWarnings("ForLoopReplaceableByForEach") // This is for GC sensitivity testing: no streams + @Override + public void recordValuesInInstruments( + TestInstrumentType.TestInstrumentsState testInstrumentsState, + List attributesList, + Random random) { + DoubleSumState state = (DoubleSumState) testInstrumentsState; + + for (int j = 0; j < attributesList.size(); j++) { + Attributes attributes = attributesList.get(j); + for (int i = 0; i < measurementsPerAttributeSet; i++) { + state.doubleCounter.add(1.2f, attributes); + } + } + } +} diff --git a/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/tester/LongSumTester.java b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/tester/LongSumTester.java new file mode 100644 index 00000000000..2477020633e --- /dev/null +++ b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/tester/LongSumTester.java @@ -0,0 +1,58 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.internal.state.tester; + +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.metrics.LongCounter; +import io.opentelemetry.api.metrics.Meter; +import io.opentelemetry.sdk.metrics.Aggregation; +import io.opentelemetry.sdk.metrics.SdkMeterProvider; +import io.opentelemetry.sdk.metrics.internal.state.TestInstrumentType; +import java.util.List; +import java.util.Random; + +public class LongSumTester implements TestInstrumentType.InstrumentTester { + private static final int measurementsPerAttributeSet = 1_000; + + static class LongSumState implements TestInstrumentType.TestInstrumentsState { + LongCounter longCounter; + } + + @Override + public Aggregation testedAggregation() { + return Aggregation.sum(); + } + + @Override + public TestInstrumentType.TestInstrumentsState buildInstruments( + double instrumentCount, + SdkMeterProvider sdkMeterProvider, + List attributesList, + Random random) { + LongSumState longSumState = new LongSumState(); + + Meter meter = sdkMeterProvider.meterBuilder("meter").build(); + longSumState.longCounter = meter.counterBuilder("test.long.sum").build(); + + return longSumState; + } + + @SuppressWarnings("ForLoopReplaceableByForEach") // This is for GC sensitivity testing: no streams + @Override + public void recordValuesInInstruments( + TestInstrumentType.TestInstrumentsState testInstrumentsState, + List attributesList, + Random random) { + LongSumState state = (LongSumState) testInstrumentsState; + + for (int j = 0; j < attributesList.size(); j++) { + Attributes attributes = attributesList.get(j); + for (int i = 0; i < measurementsPerAttributeSet; i++) { + state.longCounter.add(1, attributes); + } + } + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleSumAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleSumAggregator.java index c88217114c7..17f02f85208 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleSumAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleSumAggregator.java @@ -7,6 +7,7 @@ import io.opentelemetry.api.common.Attributes; import io.opentelemetry.sdk.common.InstrumentationScopeInfo; +import io.opentelemetry.sdk.common.export.MemoryMode; import io.opentelemetry.sdk.metrics.data.AggregationTemporality; import io.opentelemetry.sdk.metrics.data.DoubleExemplarData; import io.opentelemetry.sdk.metrics.data.DoublePointData; @@ -25,6 +26,7 @@ import java.util.Collection; import java.util.List; import java.util.function.Supplier; +import javax.annotation.Nullable; /** * Sum aggregator that keeps values as {@code double}s. @@ -35,24 +37,28 @@ public final class DoubleSumAggregator extends AbstractSumAggregator { private final Supplier> reservoirSupplier; + private final MemoryMode memoryMode; /** * Constructs a sum aggregator. * * @param instrumentDescriptor The instrument being recorded, used to compute monotonicity. * @param reservoirSupplier Supplier of exemplar reservoirs per-stream. + * @param memoryMode The memory mode to use. */ public DoubleSumAggregator( InstrumentDescriptor instrumentDescriptor, - Supplier> reservoirSupplier) { + Supplier> reservoirSupplier, + MemoryMode memoryMode) { super(instrumentDescriptor); this.reservoirSupplier = reservoirSupplier; + this.memoryMode = memoryMode; } @Override public AggregatorHandle createHandle() { - return new Handle(reservoirSupplier.get()); + return new Handle(reservoirSupplier.get(), memoryMode); } @Override @@ -124,8 +130,12 @@ public MetricData toMetricData( static final class Handle extends AggregatorHandle { private final DoubleAdder current = AdderUtil.createDoubleAdder(); - Handle(ExemplarReservoir exemplarReservoir) { + // Only used if memoryMode == MemoryMode.REUSABLE_DATA + @Nullable private final MutableDoublePointData reusablePoint; + + Handle(ExemplarReservoir exemplarReservoir, MemoryMode memoryMode) { super(exemplarReservoir); + reusablePoint = memoryMode == MemoryMode.REUSABLE_DATA ? new MutableDoublePointData() : null; } @Override @@ -136,8 +146,13 @@ protected DoublePointData doAggregateThenMaybeReset( List exemplars, boolean reset) { double value = reset ? this.current.sumThenReset() : this.current.sum(); - return ImmutableDoublePointData.create( - startEpochNanos, epochNanos, attributes, value, exemplars); + if (reusablePoint != null) { + reusablePoint.set(startEpochNanos, epochNanos, attributes, value, exemplars); + return reusablePoint; + } else { + return ImmutableDoublePointData.create( + startEpochNanos, epochNanos, attributes, value, exemplars); + } } @Override diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/LongSumAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/LongSumAggregator.java index 131c82ce3e9..cfd23c0cf97 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/LongSumAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/LongSumAggregator.java @@ -7,6 +7,7 @@ import io.opentelemetry.api.common.Attributes; import io.opentelemetry.sdk.common.InstrumentationScopeInfo; +import io.opentelemetry.sdk.common.export.MemoryMode; import io.opentelemetry.sdk.metrics.data.AggregationTemporality; import io.opentelemetry.sdk.metrics.data.LongExemplarData; import io.opentelemetry.sdk.metrics.data.LongPointData; @@ -25,6 +26,7 @@ import java.util.Collection; import java.util.List; import java.util.function.Supplier; +import javax.annotation.Nullable; /** * Sum aggregator that keeps values as {@code long}s. @@ -36,17 +38,20 @@ public final class LongSumAggregator extends AbstractSumAggregator { private final Supplier> reservoirSupplier; + private final MemoryMode memoryMode; public LongSumAggregator( InstrumentDescriptor instrumentDescriptor, - Supplier> reservoirSupplier) { + Supplier> reservoirSupplier, + MemoryMode memoryMode) { super(instrumentDescriptor); this.reservoirSupplier = reservoirSupplier; + this.memoryMode = memoryMode; } @Override public AggregatorHandle createHandle() { - return new Handle(reservoirSupplier.get()); + return new Handle(reservoirSupplier.get(), memoryMode); } @Override @@ -118,8 +123,13 @@ public MetricData toMetricData( static final class Handle extends AggregatorHandle { private final LongAdder current = AdderUtil.createLongAdder(); - Handle(ExemplarReservoir exemplarReservoir) { + // Only used if memoryMode == MemoryMode.REUSABLE_DATA + @Nullable private final MutableLongPointData reusablePointData; + + Handle(ExemplarReservoir exemplarReservoir, MemoryMode memoryMode) { super(exemplarReservoir); + reusablePointData = + memoryMode == MemoryMode.REUSABLE_DATA ? new MutableLongPointData() : null; } @Override @@ -130,8 +140,13 @@ protected LongPointData doAggregateThenMaybeReset( List exemplars, boolean reset) { long value = reset ? this.current.sumThenReset() : this.current.sum(); - return ImmutableLongPointData.create( - startEpochNanos, epochNanos, attributes, value, exemplars); + if (reusablePointData != null) { + reusablePointData.set(startEpochNanos, epochNanos, attributes, value, exemplars); + return reusablePointData; + } else { + return ImmutableLongPointData.create( + startEpochNanos, epochNanos, attributes, value, exemplars); + } } @Override diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableDoublePointData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableDoublePointData.java index fb412e6f2a8..1f19c3911ae 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableDoublePointData.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableDoublePointData.java @@ -93,15 +93,15 @@ public boolean equals(Object o) { if (this == o) { return true; } - if (o == null || !(o instanceof MutableDoublePointData)) { + if (!(o instanceof DoublePointData)) { return false; } - MutableDoublePointData pointData = (MutableDoublePointData) o; - return startEpochNanos == pointData.startEpochNanos - && epochNanos == pointData.epochNanos - && Double.doubleToLongBits(value) == Double.doubleToLongBits(pointData.value) - && Objects.equals(attributes, pointData.attributes) - && Objects.equals(exemplars, pointData.exemplars); + DoublePointData pointData = (DoublePointData) o; + return startEpochNanos == pointData.getStartEpochNanos() + && epochNanos == pointData.getEpochNanos() + && Double.doubleToLongBits(value) == Double.doubleToLongBits(pointData.getValue()) + && Objects.equals(attributes, pointData.getAttributes()) + && Objects.equals(exemplars, pointData.getExemplars()); } @Override diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableLongPointData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableLongPointData.java index 179accaf486..679a9e4902f 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableLongPointData.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableLongPointData.java @@ -91,15 +91,15 @@ public boolean equals(Object o) { if (this == o) { return true; } - if (o == null || !(o instanceof MutableLongPointData)) { + if (!(o instanceof LongPointData)) { return false; } - MutableLongPointData that = (MutableLongPointData) o; - return value == that.value - && startEpochNanos == that.startEpochNanos - && epochNanos == that.epochNanos - && Objects.equals(attributes, that.attributes) - && Objects.equals(exemplars, that.exemplars); + LongPointData that = (LongPointData) o; + return value == that.getValue() + && startEpochNanos == that.getStartEpochNanos() + && epochNanos == that.getEpochNanos() + && Objects.equals(attributes, that.getAttributes()) + && Objects.equals(exemplars, that.getExemplars()); } @Override diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/SumAggregation.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/SumAggregation.java index a23b0bc4d7e..7ca1294666b 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/SumAggregation.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/SumAggregation.java @@ -54,7 +54,8 @@ public Aggregator createAggr Clock.getDefault(), Runtime.getRuntime().availableProcessors(), RandomSupplier.platformDefault())); - return (Aggregator) new LongSumAggregator(instrumentDescriptor, reservoirFactory); + return (Aggregator) + new LongSumAggregator(instrumentDescriptor, reservoirFactory, memoryMode); } case DOUBLE: { @@ -66,7 +67,8 @@ public Aggregator createAggr Clock.getDefault(), Runtime.getRuntime().availableProcessors(), RandomSupplier.platformDefault())); - return (Aggregator) new DoubleSumAggregator(instrumentDescriptor, reservoirFactory); + return (Aggregator) + new DoubleSumAggregator(instrumentDescriptor, reservoirFactory, memoryMode); } } throw new IllegalArgumentException("Invalid instrument value type"); diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleSumAggregatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleSumAggregatorTest.java index edd1f0fc077..6b4715dd729 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleSumAggregatorTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleSumAggregatorTest.java @@ -6,7 +6,6 @@ package io.opentelemetry.sdk.metrics.internal.aggregator; import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat; -import static org.assertj.core.api.Assertions.assertThat; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; @@ -15,6 +14,7 @@ import io.opentelemetry.api.trace.TraceState; import io.opentelemetry.context.Context; import io.opentelemetry.sdk.common.InstrumentationScopeInfo; +import io.opentelemetry.sdk.common.export.MemoryMode; import io.opentelemetry.sdk.metrics.InstrumentType; import io.opentelemetry.sdk.metrics.InstrumentValueType; import io.opentelemetry.sdk.metrics.data.AggregationTemporality; @@ -33,6 +33,8 @@ import java.util.List; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; @@ -48,24 +50,33 @@ class DoubleSumAggregatorTest { private static final MetricDescriptor metricDescriptor = MetricDescriptor.create("name", "description", "unit"); - private static final DoubleSumAggregator aggregator = - new DoubleSumAggregator( - InstrumentDescriptor.create( - "instrument_name", - "instrument_description", - "instrument_unit", - InstrumentType.COUNTER, - InstrumentValueType.DOUBLE, - Advice.empty()), - ExemplarReservoir::doubleNoSamples); + private DoubleSumAggregator aggregator; - @Test - void createHandle() { + private void init(MemoryMode memoryMode) { + aggregator = + new DoubleSumAggregator( + InstrumentDescriptor.create( + "instrument_name", + "instrument_description", + "instrument_unit", + InstrumentType.COUNTER, + InstrumentValueType.DOUBLE, + Advice.empty()), + ExemplarReservoir::doubleNoSamples, + memoryMode); + } + + @ParameterizedTest + @EnumSource(MemoryMode.class) + void createHandle(MemoryMode memoryMode) { + init(memoryMode); assertThat(aggregator.createHandle()).isInstanceOf(DoubleSumAggregator.Handle.class); } - @Test - void multipleRecords() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void multipleRecords(MemoryMode memoryMode) { + init(memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); aggregatorHandle.recordDouble(12.1); @@ -80,8 +91,10 @@ void multipleRecords() { .isEqualTo(12.1 * 5); } - @Test - void multipleRecords_WithNegatives() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void multipleRecords_WithNegatives(MemoryMode memoryMode) { + init(memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); aggregatorHandle.recordDouble(12); @@ -97,8 +110,10 @@ void multipleRecords_WithNegatives() { .isEqualTo(14); } - @Test - void aggregateThenMaybeReset() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void aggregateThenMaybeReset(MemoryMode memoryMode) { + init(memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); @@ -119,8 +134,9 @@ void aggregateThenMaybeReset() { .isEqualTo(-13); } - @Test - void aggregateThenMaybeReset_WithExemplars() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void aggregateThenMaybeReset_WithExemplars(MemoryMode memoryMode) { Attributes attributes = Attributes.builder().put("test", "value").build(); DoubleExemplarData exemplar = ImmutableDoubleExemplarData.create( @@ -143,7 +159,8 @@ void aggregateThenMaybeReset_WithExemplars() { InstrumentType.COUNTER, InstrumentValueType.DOUBLE, Advice.empty()), - () -> reservoir); + () -> reservoir, + memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); aggregatorHandle.recordDouble(0, attributes, Context.root()); @@ -152,8 +169,9 @@ void aggregateThenMaybeReset_WithExemplars() { .isEqualTo(ImmutableDoublePointData.create(0, 1, Attributes.empty(), 0, exemplars)); } - @Test - void mergeAndDiff() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void mergeAndDiff(MemoryMode memoryMode) { Attributes attributes = Attributes.builder().put("test", "value").build(); DoubleExemplarData exemplar = ImmutableDoubleExemplarData.create( @@ -177,7 +195,8 @@ void mergeAndDiff() { instrumentType, InstrumentValueType.LONG, Advice.empty()), - ExemplarReservoir::doubleNoSamples); + ExemplarReservoir::doubleNoSamples, + memoryMode); DoublePointData diffed = aggregator.diff( @@ -193,8 +212,10 @@ void mergeAndDiff() { } } - @Test - void diffInPlace() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void diffInPlace(MemoryMode memoryMode) { + init(memoryMode); Attributes attributes = Attributes.builder().put("test", "value").build(); DoubleExemplarData exemplar = ImmutableDoubleExemplarData.create( @@ -235,8 +256,10 @@ void diffInPlace() { assertThat(previous.getExemplars()).isEqualTo(exemplars); } - @Test - void copyPoint() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void copyPoint(MemoryMode memoryMode) { + init(memoryMode); MutableDoublePointData pointData = (MutableDoublePointData) aggregator.createReusablePoint(); Attributes attributes = Attributes.of(AttributeKey.longKey("test"), 100L); @@ -278,8 +301,10 @@ void copyPoint() { assertThat(toPointData.getExemplars()).isEqualTo(pointData.getExemplars()); } - @Test - void toMetricData() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void toMetricData(MemoryMode memoryMode) { + init(memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); aggregatorHandle.recordDouble(10); @@ -310,8 +335,10 @@ void toMetricData() { .hasValue(10))); } - @Test - void toMetricDataWithExemplars() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void toMetricDataWithExemplars(MemoryMode memoryMode) { + init(memoryMode); Attributes attributes = Attributes.builder().put("test", "value").build(); DoubleExemplarData exemplar = ImmutableDoubleExemplarData.create( @@ -335,4 +362,22 @@ void toMetricDataWithExemplars() { .hasDoubleSumSatisfying( sum -> sum.hasPointsSatisfying(point -> point.hasValue(1).hasExemplars(exemplar))); } + + @Test + void sameObjectReturnedOnReusableDataMemoryMode() { + init(MemoryMode.REUSABLE_DATA); + AggregatorHandle aggregatorHandle = + aggregator.createHandle(); + aggregatorHandle.recordDouble(1.0); + + DoublePointData firstCollection = + aggregatorHandle.aggregateThenMaybeReset(0, 1, Attributes.empty(), /* reset= */ false); + + aggregatorHandle.recordDouble(1.0); + DoublePointData secondCollection = + aggregatorHandle.aggregateThenMaybeReset(0, 1, Attributes.empty(), /* reset= */ false); + + // Should be same object since we are in REUSABLE_DATA mode. + assertThat(firstCollection).isSameAs(secondCollection); + } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/LongSumAggregatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/LongSumAggregatorTest.java index 11b98233b07..5ec6f4be28d 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/LongSumAggregatorTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/LongSumAggregatorTest.java @@ -6,7 +6,6 @@ package io.opentelemetry.sdk.metrics.internal.aggregator; import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat; -import static org.assertj.core.api.Assertions.assertThat; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; @@ -15,6 +14,7 @@ import io.opentelemetry.api.trace.TraceState; import io.opentelemetry.context.Context; import io.opentelemetry.sdk.common.InstrumentationScopeInfo; +import io.opentelemetry.sdk.common.export.MemoryMode; import io.opentelemetry.sdk.metrics.InstrumentType; import io.opentelemetry.sdk.metrics.InstrumentValueType; import io.opentelemetry.sdk.metrics.data.AggregationTemporality; @@ -33,6 +33,8 @@ import java.util.List; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; @@ -47,24 +49,33 @@ class LongSumAggregatorTest { private static final InstrumentationScopeInfo library = InstrumentationScopeInfo.empty(); private static final MetricDescriptor metricDescriptor = MetricDescriptor.create("name", "description", "unit"); - private static final LongSumAggregator aggregator = - new LongSumAggregator( - InstrumentDescriptor.create( - "instrument_name", - "instrument_description", - "instrument_unit", - InstrumentType.COUNTER, - InstrumentValueType.LONG, - Advice.empty()), - ExemplarReservoir::longNoSamples); + private LongSumAggregator aggregator; - @Test - void createHandle() { + private void init(MemoryMode memoryMode) { + aggregator = + new LongSumAggregator( + InstrumentDescriptor.create( + "instrument_name", + "instrument_description", + "instrument_unit", + InstrumentType.COUNTER, + InstrumentValueType.LONG, + Advice.empty()), + ExemplarReservoir::longNoSamples, + memoryMode); + } + + @ParameterizedTest + @EnumSource(MemoryMode.class) + void createHandle(MemoryMode memoryMode) { + init(memoryMode); assertThat(aggregator.createHandle()).isInstanceOf(LongSumAggregator.Handle.class); } - @Test - void multipleRecords() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void multipleRecords(MemoryMode memoryMode) { + init(memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); aggregatorHandle.recordLong(12); aggregatorHandle.recordLong(12); @@ -78,8 +89,10 @@ void multipleRecords() { .isEqualTo(12 * 5); } - @Test - void multipleRecords_WithNegatives() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void multipleRecords_WithNegatives(MemoryMode memoryMode) { + init(memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); aggregatorHandle.recordLong(12); aggregatorHandle.recordLong(12); @@ -94,8 +107,10 @@ void multipleRecords_WithNegatives() { .isEqualTo(14); } - @Test - void aggregateThenMaybeReset() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void aggregateThenMaybeReset(MemoryMode memoryMode) { + init(memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); aggregatorHandle.recordLong(13); @@ -115,8 +130,9 @@ void aggregateThenMaybeReset() { .isEqualTo(-13); } - @Test - void aggregateThenMaybeReset_WithExemplars() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void aggregateThenMaybeReset_WithExemplars(MemoryMode memoryMode) { Attributes attributes = Attributes.builder().put("test", "value").build(); LongExemplarData exemplar = ImmutableLongExemplarData.create( @@ -139,7 +155,8 @@ void aggregateThenMaybeReset_WithExemplars() { InstrumentType.COUNTER, InstrumentValueType.LONG, Advice.empty()), - () -> reservoir); + () -> reservoir, + memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); aggregatorHandle.recordLong(0, attributes, Context.root()); assertThat( @@ -147,8 +164,9 @@ void aggregateThenMaybeReset_WithExemplars() { .isEqualTo(ImmutableLongPointData.create(0, 1, Attributes.empty(), 0, exemplars)); } - @Test - void mergeAndDiff() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void mergeAndDiff(MemoryMode memoryMode) { LongExemplarData exemplar = ImmutableLongExemplarData.create( Attributes.empty(), @@ -171,7 +189,8 @@ void mergeAndDiff() { instrumentType, InstrumentValueType.LONG, Advice.empty()), - ExemplarReservoir::longNoSamples); + ExemplarReservoir::longNoSamples, + memoryMode); LongPointData diffed = aggregator.diff( @@ -187,8 +206,10 @@ void mergeAndDiff() { } } - @Test - void diffInPlace() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void diffInPlace(MemoryMode memoryMode) { + init(memoryMode); Attributes attributes = Attributes.builder().put("test", "value").build(); LongExemplarData exemplar = ImmutableLongExemplarData.create( @@ -229,8 +250,10 @@ void diffInPlace() { assertThat(previous.getExemplars()).isEqualTo(current.getExemplars()); } - @Test - void copyPoint() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void copyPoint(MemoryMode memoryMode) { + init(memoryMode); MutableLongPointData pointData = (MutableLongPointData) aggregator.createReusablePoint(); Attributes attributes = Attributes.of(AttributeKey.longKey("test"), 100L); @@ -272,8 +295,10 @@ void copyPoint() { assertThat(toPointData.getExemplars()).isEqualTo(pointData.getExemplars()); } - @Test - void toMetricData() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void toMetricData(MemoryMode memoryMode) { + init(memoryMode); AggregatorHandle aggregatorHandle = aggregator.createHandle(); aggregatorHandle.recordLong(10); @@ -303,8 +328,10 @@ void toMetricData() { .hasValue(10))); } - @Test - void toMetricDataWithExemplars() { + @ParameterizedTest + @EnumSource(MemoryMode.class) + void toMetricDataWithExemplars(MemoryMode memoryMode) { + init(memoryMode); Attributes attributes = Attributes.builder().put("test", "value").build(); LongExemplarData exemplar = ImmutableLongExemplarData.create( @@ -329,4 +356,21 @@ void toMetricDataWithExemplars() { .hasLongSumSatisfying( sum -> sum.hasPointsSatisfying(point -> point.hasValue(1).hasExemplars(exemplar))); } + + @Test + void sameObjectReturnedOnReusableDataMemoryMode() { + init(MemoryMode.REUSABLE_DATA); + AggregatorHandle aggregatorHandle = aggregator.createHandle(); + + aggregatorHandle.recordLong(1L); + LongPointData firstCollection = + aggregatorHandle.aggregateThenMaybeReset(0, 1, Attributes.empty(), /* reset= */ false); + + aggregatorHandle.recordLong(1L); + LongPointData secondCollection = + aggregatorHandle.aggregateThenMaybeReset(0, 1, Attributes.empty(), /* reset= */ false); + + // Should be same object since we are in REUSABLE_DATA mode. + assertThat(firstCollection).isSameAs(secondCollection); + } } From e4b856b22e709110a963d2844a2e4bda3e947fd2 Mon Sep 17 00:00:00 2001 From: Asaf Mesika Date: Wed, 31 Jan 2024 10:32:48 +0200 Subject: [PATCH 2/2] PR Fixes --- .../internal/state/TestInstrumentType.java | 50 +++++++------------ .../MutableExponentialHistogramBuckets.java | 6 +-- .../MutableExponentialHistogramPointData.java | 32 ++++++------ 3 files changed, 36 insertions(+), 52 deletions(-) diff --git a/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/TestInstrumentType.java b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/TestInstrumentType.java index f2c89bb8fba..1cf082699be 100644 --- a/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/TestInstrumentType.java +++ b/sdk/metrics/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/TestInstrumentType.java @@ -15,47 +15,29 @@ import io.opentelemetry.sdk.metrics.internal.state.tester.LongSumTester; import java.util.List; import java.util.Random; +import java.util.function.Supplier; +@SuppressWarnings("ImmutableEnumChecker") public enum TestInstrumentType { - ASYNC_COUNTER() { - @Override - InstrumentTester createInstrumentTester() { - return new AsyncCounterTester(); - } - }, - EXPONENTIAL_HISTOGRAM() { - @Override - InstrumentTester createInstrumentTester() { - return new ExponentialHistogramTester(); - } - }, - EXPLICIT_BUCKET() { - @Override - InstrumentTester createInstrumentTester() { - return new ExplicitBucketHistogramTester(); - } - }, - LONG_SUM(/* dataAllocRateReductionPercentage= */ 97.3f) { - @Override - InstrumentTester createInstrumentTester() { - return new LongSumTester(); - } - }, - DOUBLE_SUM(/* dataAllocRateReductionPercentage= */ 97.3f) { - @Override - InstrumentTester createInstrumentTester() { - return new DoubleSumTester(); - } - }; + ASYNC_COUNTER(AsyncCounterTester::new), + EXPONENTIAL_HISTOGRAM(ExponentialHistogramTester::new), + EXPLICIT_BUCKET(ExplicitBucketHistogramTester::new), + LONG_SUM(LongSumTester::new, /* dataAllocRateReductionPercentage= */ 97.3f), + DOUBLE_SUM(DoubleSumTester::new, /* dataAllocRateReductionPercentage= */ 97.3f); + private final Supplier instrumentTesterInitializer; private final float dataAllocRateReductionPercentage; - TestInstrumentType() { + TestInstrumentType(Supplier instrumentTesterInitializer) { this.dataAllocRateReductionPercentage = 99.8f; // default + this.instrumentTesterInitializer = instrumentTesterInitializer; } // Some instruments have different reduction percentage. - TestInstrumentType(float dataAllocRateReductionPercentage) { + TestInstrumentType( + Supplier instrumentTesterInitializer, + float dataAllocRateReductionPercentage) { + this.instrumentTesterInitializer = instrumentTesterInitializer; this.dataAllocRateReductionPercentage = dataAllocRateReductionPercentage; } @@ -63,7 +45,9 @@ float getDataAllocRateReductionPercentage() { return dataAllocRateReductionPercentage; } - abstract InstrumentTester createInstrumentTester(); + InstrumentTester createInstrumentTester() { + return instrumentTesterInitializer.get(); + } public interface InstrumentTester { Aggregation testedAggregation(); diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableExponentialHistogramBuckets.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableExponentialHistogramBuckets.java index 9c9f815426d..9554d4722ea 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableExponentialHistogramBuckets.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableExponentialHistogramBuckets.java @@ -81,12 +81,12 @@ public boolean equals(Object o) { if (o == this) { return true; } - if (o instanceof MutableExponentialHistogramBuckets) { - MutableExponentialHistogramBuckets that = (MutableExponentialHistogramBuckets) o; + if (o instanceof ExponentialHistogramBuckets) { + ExponentialHistogramBuckets that = (ExponentialHistogramBuckets) o; return this.scale == that.getScale() && this.offset == that.getOffset() && this.totalCount == that.getTotalCount() - && Objects.equals(this.bucketCounts, that.bucketCounts); + && Objects.equals(this.bucketCounts, that.getBucketCounts()); } return false; } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableExponentialHistogramPointData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableExponentialHistogramPointData.java index c32565af087..bccc7a9abe1 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableExponentialHistogramPointData.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableExponentialHistogramPointData.java @@ -192,22 +192,22 @@ public boolean equals(Object o) { if (o == this) { return true; } - if (o instanceof MutableExponentialHistogramPointData) { - MutableExponentialHistogramPointData that = (MutableExponentialHistogramPointData) o; - return this.startEpochNanos == that.startEpochNanos - && this.epochNanos == that.epochNanos - && this.attributes.equals(that.attributes) - && this.scale == that.scale - && Double.doubleToLongBits(this.sum) == Double.doubleToLongBits(that.sum) - && this.count == that.count - && this.zeroCount == that.zeroCount - && this.hasMin == that.hasMin - && Double.doubleToLongBits(this.min) == Double.doubleToLongBits(that.min) - && this.hasMax == that.hasMax - && Double.doubleToLongBits(this.max) == Double.doubleToLongBits(that.max) - && this.positiveBuckets.equals(that.positiveBuckets) - && this.negativeBuckets.equals(that.negativeBuckets) - && this.exemplars.equals(that.exemplars); + if (o instanceof ExponentialHistogramPointData) { + ExponentialHistogramPointData that = (ExponentialHistogramPointData) o; + return this.startEpochNanos == that.getStartEpochNanos() + && this.epochNanos == that.getEpochNanos() + && this.attributes.equals(that.getAttributes()) + && this.scale == that.getScale() + && Double.doubleToLongBits(this.sum) == Double.doubleToLongBits(that.getSum()) + && this.count == that.getCount() + && this.zeroCount == that.getZeroCount() + && this.hasMin == that.hasMin() + && Double.doubleToLongBits(this.min) == Double.doubleToLongBits(that.getMin()) + && this.hasMax == that.hasMax() + && Double.doubleToLongBits(this.max) == Double.doubleToLongBits(that.getMax()) + && this.positiveBuckets.equals(that.getPositiveBuckets()) + && this.negativeBuckets.equals(that.getNegativeBuckets()) + && this.exemplars.equals(that.getExemplars()); } return false; }