From 401a6a5db3cb53a883a30c198908fd4f1ccda92e Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Tue, 20 Dec 2022 14:30:19 -0800 Subject: [PATCH] [main-1.3.1] Patch Histogram synchronization fix (#4031) * Patch Histogram synchronization fix --- .github/workflows/publish-packages-1.0.yml | 2 +- src/OpenTelemetry/CHANGELOG.md | 9 +++ src/OpenTelemetry/Metrics/HistogramBuckets.cs | 2 - src/OpenTelemetry/Metrics/MetricPoint.cs | 70 +++++++++++++------ 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/.github/workflows/publish-packages-1.0.yml b/.github/workflows/publish-packages-1.0.yml index 7368694b89e..af8ea73284c 100644 --- 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 index 90e3d0b1958..5dfb6f001e6 100644 --- 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 index 626779d4901..d87ed7a46d3 100644 --- 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 index f848c319594..e7c8fce0951 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -339,6 +339,7 @@ internal void Update(double number) { if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0) { + // Lock acquired unchecked { this.runningValue.AsLong++; @@ -346,7 +347,8 @@ internal void Update(double 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,26 +499,37 @@ 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; @@ -522,17 +537,28 @@ internal void TakeSnapshot(bool outputDelta) 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;