[main-1.3.1] Patch Histogram synchronization fix (#4031)

* Patch Histogram synchronization fix
open-telemetry · Dec 20, 2022 · 401a6a5 · 401a6a5
1 parent c9052ef
commit 401a6a5
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 25 deletions.
diff --git a/.github/workflows/publish-packages-1.0.yml b/.github/workflows/publish-packages-1.0.yml
@@ -24,7 +24,7 @@ jobs:
     strategy:
       matrix:
         os: [windows-latest]
-        branches: [main-1.3.0]
+        branches: [main-1.3.1]
 
     steps:
     - uses: actions/checkout@v3

diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -2,6 +2,15 @@
 
 ## Unreleased
 
+* Support for metrics was originally released in version 1.3.0. A
+  synchronization issue with regards to the histogram aggregation was
+  discovered and fixed in
+  [#3534](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3534).
+  This fix has been back-ported to OpenTelemetry .NET 1.3.2. If you
+  are currently running OpenTelemetry .NET version 1.3.x, it is recommended
+  that you upgrade to 1.3.2.
+  ([#4031](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4031))
+
 ## 1.3.1
 
 Released 2022-Sep-06

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
@@ -339,14 +339,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;
                             }
 
@@ -363,13 +365,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;
                             }
 
@@ -495,44 +499,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;