Switch to primitive types and ensure synchronization (#3741)

Without this change, there were still consistency issues detected despite the synchronization.

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

implementations/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/types/DynatraceDistributionSummary.java

@@ -64,21 +64,46 @@ protected void recordNonNegative(double amount) {
         summary.recordNonNegative(amount);
     }
 
+    /**
+     * Using this method is not synchronized and might give inconsistent results when
+     * multiple getters are called sequentially. It is recommended to
+     * {@link DynatraceDistributionSummary#takeSummarySnapshot() take a snapshot} and use
+     * the getters on the {@link DynatraceSummarySnapshot} instead.
+     */
     @Override
     public long count() {
         return summary.getCount();
     }
 
+    /**
+     * Using this method is not synchronized and might give inconsistent results when
+     * multiple getters are called sequentially. It is recommended to
+     * {@link DynatraceDistributionSummary#takeSummarySnapshot() take a snapshot} and use
+     * the getters on the {@link DynatraceSummarySnapshot} instead.
+     */
     @Override
     public double totalAmount() {
         return summary.getTotal();
     }
 
+    /**
+     * Using this method is not synchronized and might give inconsistent results when
+     * multiple getters are called sequentially. It is recommended to
+     * {@link DynatraceDistributionSummary#takeSummarySnapshot() take a snapshot} and use
+     * the getters on the {@link DynatraceSummarySnapshot} instead.
+     */
     @Override
     public double max() {
         return summary.getMax();
     }
 
+    /**
+     * @deprecated since 1.9.10. Using this method is not synchronized and might give
+     * inconsistent results when multiple getters are called sequentially. It is
+     * recommended to {@link DynatraceDistributionSummary#takeSummarySnapshot() take a
+     * snapshot} and use the getters on the {@link DynatraceSummarySnapshot} instead.
+     */
+    @Deprecated
     public double min() {
         return summary.getMin();
     }

implementations/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/types/DynatraceSummary.java

@@ -15,61 +15,71 @@
  */
 package io.micrometer.dynatrace.types;
 
-import java.util.concurrent.atomic.AtomicLong;
-import java.util.concurrent.atomic.DoubleAdder;
-import java.util.concurrent.atomic.LongAdder;
-
 /**
  * Internal class for resettable summary statistics.
  *
  * @author Georg Pirklbauer
  */
 final class DynatraceSummary {
 
-    private final LongAdder count = new LongAdder();
+    private long count;
 
-    private final DoubleAdder total = new DoubleAdder();
+    private double total;
 
-    private final AtomicLong min = new AtomicLong();
+    private double min;
 
-    private final AtomicLong max = new AtomicLong();
+    private double max;
 
     void recordNonNegative(double amount) {
         if (amount < 0) {
             return;
         }
 
-        long longBits = Double.doubleToLongBits(amount);
         synchronized (this) {
-            max.getAndUpdate(prev -> Math.max(prev, longBits));
-            // have to check if a value was already recorded before, otherwise min will
-            // always stay 0 (because the default is 0).
-            min.getAndUpdate(prev -> count.longValue() > 0 ? Math.min(prev, longBits) : longBits);
-
-            total.add(amount);
-            count.increment();
+            max = Math.max(max, amount);
+            // check if count is equal to 0 and set initial min value if it is, otherwise
+            // min will always stay at 0.
+            min = count > 0 ? Math.min(min, amount) : amount;
+            total += amount;
+            count++;
         }
     }
 
+    /**
+     * Getters are not synchronized and might give inconsistent results. It is recommended
+     * to take a snapshot and use the {@link DynatraceSummarySnapshot} instead.
+     */
     long getCount() {
-        return count.longValue();
+        return count;
     }
 
+    /**
+     * Getters are not synchronized and might give inconsistent results. It is recommended
+     * to take a snapshot and use the {@link DynatraceSummarySnapshot} instead.
+     */
     double getTotal() {
-        return total.doubleValue();
+        return total;
     }
 
+    /**
+     * Getters are not synchronized and might give inconsistent results. It is recommended
+     * to take a snapshot and use the {@link DynatraceSummarySnapshot} instead.
+     */
     double getMin() {
-        return Double.longBitsToDouble(min.longValue());
+        return min;
     }
 
+    /**
+     * Getters are not synchronized and might give inconsistent results. It is recommended
+     * to take a snapshot and use the {@link DynatraceSummarySnapshot} instead.
+     */
     double getMax() {
-        return Double.longBitsToDouble(max.longValue());
+        return max;
     }
 
     DynatraceSummarySnapshot takeSummarySnapshot() {
         synchronized (this) {
-            return new DynatraceSummarySnapshot(getMin(), getMax(), getTotal(), getCount());
+            return new DynatraceSummarySnapshot(min, max, total, count);
         }
     }
 
@@ -83,10 +93,10 @@ DynatraceSummarySnapshot takeSummarySnapshotAndReset() {
 
     void reset() {
         synchronized (this) {
-            min.set(0);
-            max.set(0);
-            total.reset();
-            count.reset();
+            min = 0.;
+            max = 0.;
+            total = 0.;
+            count = 0;
         }
     }
 

implementations/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/types/DynatraceTimer.java

@@ -104,21 +104,47 @@ protected void recordNonNegative(long amount, TimeUnit unit) {
         summary.recordNonNegative(inBaseUnit);
     }
 
+    /**
+     * Using this method is not synchronized and might give inconsistent results when
+     * multiple getters are called sequentially. It is recommended to
+     * {@link DynatraceDistributionSummary#takeSummarySnapshot(TimeUnit) take a snapshot}
+     * and use the getters on the {@link DynatraceSummarySnapshot} instead.
+     */
     @Override
     public long count() {
         return summary.getCount();
     }
 
+    /**
+     * Using this method is not synchronized and might give inconsistent results when
+     * multiple getters are called sequentially. It is recommended to
+     * {@link DynatraceDistributionSummary#takeSummarySnapshot(TimeUnit) take a snapshot}
+     * and use the getters on the {@link DynatraceSummarySnapshot} instead.
+     */
     @Override
     public double totalTime(TimeUnit unit) {
         return unit.convert((long) summary.getTotal(), baseTimeUnit());
     }
 
+    /**
+     * Using this method is not synchronized and might give inconsistent results when
+     * multiple getters are called sequentially. It is recommended to
+     * {@link DynatraceDistributionSummary#takeSummarySnapshot(TimeUnit) take a snapshot}
+     * and use the getters on the {@link DynatraceSummarySnapshot} instead.
+     */
     @Override
     public double max(TimeUnit unit) {
         return unit.convert((long) summary.getMax(), baseTimeUnit());
     }
 
+    /**
+     * @deprecated since 1.9.10. Using this method is not synchronized and might give
+     * inconsistent results when multiple getters are called sequentially. It is
+     * recommended to {@link DynatraceDistributionSummary#takeSummarySnapshot(TimeUnit)
+     * take a snapshot} and use the getters on the {@link DynatraceSummarySnapshot}
+     * instead.
+     */
+    @Deprecated
     public double min(TimeUnit unit) {
         return unit.convert((long) summary.getMin(), baseTimeUnit());
     }

implementations/micrometer-registry-dynatrace/src/test/java/io/micrometer/dynatrace/types/DynatraceSummaryTest.java

@@ -18,6 +18,10 @@
 import org.assertj.core.data.Offset;
 import org.junit.jupiter.api.Test;
 
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
 import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;
 
 /**
@@ -71,6 +75,46 @@ void testMinMaxAreOverwritten() {
         assertMinMaxSumCount(summary, 0.123, 8.93, 16.953, 4);
     }
 
+    @Test
+    void testConcurrentAdds() throws InterruptedException {
+        DynatraceSummary summary = new DynatraceSummary();
+
+        int valueRecordedNTimes = 100;
+        ExecutorService executorService = Executors.newFixedThreadPool(3);
+        double expMin = 1.234;
+        double expMax = 123.456;
+        double expAvg = (expMin + expMax) / 2;
+
+        // first executor adds the min 100 times
+        executorService.submit(() -> {
+            for (int i = 0; i < valueRecordedNTimes; i++) {
+                summary.recordNonNegative(expMin);
+            }
+        });
+
+        // second executor records the avg 100 times
+        executorService.submit(() -> {
+            for (int i = 0; i < valueRecordedNTimes; i++) {
+                summary.recordNonNegative(expAvg);
+            }
+        });
+
+        // third executor records the max 100 times
+        executorService.submit(() -> {
+            for (int i = 0; i < valueRecordedNTimes; i++) {
+                summary.recordNonNegative(expMax);
+            }
+        });
+
+        executorService.shutdown();
+        boolean terminated = executorService.awaitTermination(1000, TimeUnit.MILLISECONDS);
+        assertThat(terminated).isTrue();
+
+        double expTotal = (valueRecordedNTimes * expMin) + (valueRecordedNTimes * expAvg)
+                + (valueRecordedNTimes * expMax);
+        assertMinMaxSumCount(summary, expMin, expMax, expTotal, 300);
+    }
+
     private void assertMinMaxSumCount(DynatraceSummary summary, Double expMin, Double expMax, Double expTotal,
             long expCount) {
         assertThat(summary.getMin()).isCloseTo(expMin, OFFSET);