Use synchronized instead of reentrant lock in explicit bucket histogram (open-telemetry#6309)

jack-berg · web-flow · commit 1dde2550b01f · 2024-04-03T10:23:17.000-05:00
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -138,6 +138,14 @@ uses [google-java-format](https://github.com/google/google-java-format) library:
 * Adding `toString()` overrides on classes is encouraged, but we only use `toString()` to provide
   debugging assistance. The implementations of all `toString()` methods should be considered to be
   unstable unless explicitly documented otherwise.
+* Avoid synchronizing using a class's intrinsic lock. Instead, synchronize on a dedicated lock object. E.g:
+  ```java
+  private final Object lock = new Object();
+
+  public void doSomething() {
+    synchronized (lock) { ... }
+  }
+  ```
 
 If you notice any practice being applied in the project consistently that isn't listed here, please
 consider a pull request to add it.
diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExplicitBucketHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExplicitBucketHistogramAggregator.java
@@ -26,7 +26,6 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
-import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
@@ -95,6 +94,8 @@ static final class Handle extends AggregatorHandle<HistogramPointData, DoubleExe
     // read-only
     private final double[] boundaries;
 
+    private final Object lock = new Object();
+
     @GuardedBy("lock")
     private double sum;
 
@@ -110,8 +111,6 @@ static final class Handle extends AggregatorHandle<HistogramPointData, DoubleExe
     @GuardedBy("lock")
     private final long[] counts;
 
-    private final ReentrantLock lock = new ReentrantLock();
-
     // Used only when MemoryMode = REUSABLE_DATA
     @Nullable private MutableHistogramPointData reusablePoint;
 
@@ -140,8 +139,7 @@ protected HistogramPointData doAggregateThenMaybeReset(
         Attributes attributes,
         List<DoubleExemplarData> exemplars,
         boolean reset) {
-      lock.lock();
-      try {
+      synchronized (lock) {
         HistogramPointData pointData;
         if (reusablePoint == null) {
           pointData =
@@ -180,24 +178,19 @@ protected HistogramPointData doAggregateThenMaybeReset(
           Arrays.fill(this.counts, 0);
         }
         return pointData;
-      } finally {
-        lock.unlock();
       }
     }
 
     @Override
     protected void doRecordDouble(double value) {
       int bucketIndex = ExplicitBucketHistogramUtils.findBucketIndex(this.boundaries, value);
 
-      lock.lock();
-      try {
+      synchronized (lock) {
         this.sum += value;
         this.min = Math.min(this.min, value);
         this.max = Math.max(this.max, value);
         this.count++;
         this.counts[bucketIndex]++;
-      } finally {
-        lock.unlock();
       }
     }
 
