Fix Histogram synchronization issue (#3534)

open-telemetry · Aug 23, 2022 · 968dc52 · 968dc52
1 parent ad55478
commit 968dc52
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 24 deletions.
diff --git a/src/OpenTelemetry/Metrics/HistogramBuckets.cs b/src/OpenTelemetry/Metrics/HistogramBuckets.cs
@@ -43,8 +43,6 @@ internal HistogramBuckets(double[] explicitBounds)
             this.SnapshotBucketCounts = explicitBounds != null ? new long[explicitBounds.Length + 1] : new long[0];
         }
 
-        internal object LockObject => this.SnapshotBucketCounts;
-
         public Enumerator GetEnumerator() => new(this);
 
         internal HistogramBuckets Copy()

diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs
@@ -341,14 +341,16 @@ internal void Update(double number)
                         {
                             if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
                             {
+                                // Lock acquired
                                 unchecked
                                 {
                                     this.runningValue.AsLong++;
                                     this.histogramBuckets.RunningSum += number;
                                     this.histogramBuckets.RunningBucketCounts[i]++;
                                 }
 
-                                this.histogramBuckets.IsCriticalSectionOccupied = 0;
+                                // Release lock
+                                Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
                                 break;
                             }
 
@@ -365,13 +367,15 @@ internal void Update(double number)
                         {
                             if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
                             {
+                                // Lock acquired
                                 unchecked
                                 {
                                     this.runningValue.AsLong++;
                                     this.histogramBuckets.RunningSum += number;
                                 }
 
-                                this.histogramBuckets.IsCriticalSectionOccupied = 0;
+                                // Release lock
+                                Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
                                 break;
                             }
 
@@ -497,44 +501,66 @@ internal void TakeSnapshot(bool outputDelta)
 
                 case AggregationType.Histogram:
                     {
-                        lock (this.histogramBuckets.LockObject)
+                        var sw = default(SpinWait);
+                        while (true)
                         {
-                            this.snapshotValue.AsLong = this.runningValue.AsLong;
-                            this.histogramBuckets.SnapshotSum = this.histogramBuckets.RunningSum;
-                            if (outputDelta)
-                            {
-                                this.runningValue.AsLong = 0;
-                                this.histogramBuckets.RunningSum = 0;
-                            }
-
-                            for (int i = 0; i < this.histogramBuckets.RunningBucketCounts.Length; i++)
+                            if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
                             {
-                                this.histogramBuckets.SnapshotBucketCounts[i] = this.histogramBuckets.RunningBucketCounts[i];
+                                // Lock acquired
+                                this.snapshotValue.AsLong = this.runningValue.AsLong;
+                                this.histogramBuckets.SnapshotSum = this.histogramBuckets.RunningSum;
                                 if (outputDelta)
                                 {
-                                    this.histogramBuckets.RunningBucketCounts[i] = 0;
+                                    this.runningValue.AsLong = 0;
+                                    this.histogramBuckets.RunningSum = 0;
                                 }
+
+                                for (int i = 0; i < this.histogramBuckets.RunningBucketCounts.Length; i++)
+                                {
+                                    this.histogramBuckets.SnapshotBucketCounts[i] = this.histogramBuckets.RunningBucketCounts[i];
+                                    if (outputDelta)
+                                    {
+                                        this.histogramBuckets.RunningBucketCounts[i] = 0;
+                                    }
+                                }
+
+                                this.MetricPointStatus = MetricPointStatus.NoCollectPending;
+
+                                // Release lock
+                                Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
+                                break;
                             }
 
-                            this.MetricPointStatus = MetricPointStatus.NoCollectPending;
+                            sw.SpinOnce();
                         }
 
                         break;
                     }
 
                 case AggregationType.HistogramSumCount:
                     {
-                        lock (this.histogramBuckets.LockObject)
+                        var sw = default(SpinWait);
+                        while (true)
                         {
-                            this.snapshotValue.AsLong = this.runningValue.AsLong;
-                            this.histogramBuckets.SnapshotSum = this.histogramBuckets.RunningSum;
-                            if (outputDelta)
+                            if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
                             {
-                                this.runningValue.AsLong = 0;
-                                this.histogramBuckets.RunningSum = 0;
+                                // Lock acquired
+                                this.snapshotValue.AsLong = this.runningValue.AsLong;
+                                this.histogramBuckets.SnapshotSum = this.histogramBuckets.RunningSum;
+                                if (outputDelta)
+                                {
+                                    this.runningValue.AsLong = 0;
+                                    this.histogramBuckets.RunningSum = 0;
+                                }
+
+                                this.MetricPointStatus = MetricPointStatus.NoCollectPending;
+
+                                // Release lock
+                                Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
+                                break;
                             }
 
-                            this.MetricPointStatus = MetricPointStatus.NoCollectPending;
+                            sw.SpinOnce();
                         }
 
                         break;