Minor refactor on histogram metric update (#4111)

open-telemetry · Jan 27, 2023 · 2e9a204 · 2e9a204
1 parent 5680f83
commit 2e9a204
Show file tree

Hide file tree

Showing 3 changed files with 131 additions and 102 deletions.
diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs
@@ -291,15 +291,28 @@ internal void Update(long number)
                         break;
                     }
 
-                case AggregationType.HistogramWithBuckets:
-                case AggregationType.HistogramWithMinMaxBuckets:
                 case AggregationType.Histogram:
+                    {
+                        this.UpdateHistogram((double)number);
+                        break;
+                    }
+
                 case AggregationType.HistogramWithMinMax:
                     {
-                        this.Update((double)number);
+                        this.UpdateHistogramWithMinMax((double)number);
+                        break;
+                    }
 
-                        // At this point MetricPointStatus is already set to CollectPending so we can simply return.
-                        return;
+                case AggregationType.HistogramWithBuckets:
+                    {
+                        this.UpdateHistogramWithBuckets((double)number);
+                        break;
+                    }
+
+                case AggregationType.HistogramWithMinMaxBuckets:
+                    {
+                        this.UpdateHistogramWithBucketsAndMinMax((double)number);
+                        break;
                     }
             }
 
@@ -357,113 +370,27 @@ internal void Update(double number)
                         break;
                     }
 
-                case AggregationType.HistogramWithBuckets:
+                case AggregationType.Histogram:
                     {
-                        int i = this.histogramBuckets.FindBucketIndex(number);
-
-                        var sw = default(SpinWait);
-                        while (true)
-                        {
-                            if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
-                            {
-                                // Lock acquired
-                                unchecked
-                                {
-                                    this.runningValue.AsLong++;
-                                    this.histogramBuckets.RunningSum += number;
-                                    this.histogramBuckets.RunningBucketCounts[i]++;
-                                }
-
-                                // Release lock
-                                Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
-                                break;
-                            }
-
-                            sw.SpinOnce();
-                        }
-
+                        this.UpdateHistogram(number);
                         break;
                     }
 
-                case AggregationType.Histogram:
+                case AggregationType.HistogramWithMinMax:
                     {
-                        var sw = default(SpinWait);
-                        while (true)
-                        {
-                            if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
-                            {
-                                // Lock acquired
-                                unchecked
-                                {
-                                    this.runningValue.AsLong++;
-                                    this.histogramBuckets.RunningSum += number;
-                                }
-
-                                // Release lock
-                                Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
-                                break;
-                            }
-
-                            sw.SpinOnce();
-                        }
-
+                        this.UpdateHistogramWithMinMax(number);
                         break;
                     }
 
-                case AggregationType.HistogramWithMinMaxBuckets:
+                case AggregationType.HistogramWithBuckets:
                     {
-                        int i = this.histogramBuckets.FindBucketIndex(number);
-
-                        var sw = default(SpinWait);
-                        while (true)
-                        {
-                            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.RunningMin = Math.Min(this.histogramBuckets.RunningMin, number);
-                                    this.histogramBuckets.RunningMax = Math.Max(this.histogramBuckets.RunningMax, number);
-                                }
-
-                                // Release lock
-                                Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
-                                break;
-                            }
-
-                            sw.SpinOnce();
-                        }
-
+                        this.UpdateHistogramWithBuckets(number);
                         break;
                     }
 
-                case AggregationType.HistogramWithMinMax:
+                case AggregationType.HistogramWithMinMaxBuckets:
                     {
-                        var sw = default(SpinWait);
-                        while (true)
-                        {
-                            if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
-                            {
-                                // Lock acquired
-                                unchecked
-                                {
-                                    this.runningValue.AsLong++;
-                                    this.histogramBuckets.RunningSum += number;
-                                    this.histogramBuckets.RunningMin = Math.Min(this.histogramBuckets.RunningMin, number);
-                                    this.histogramBuckets.RunningMax = Math.Max(this.histogramBuckets.RunningMax, number);
-                                }
-
-                                // Release lock
-                                Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
-                                break;
-                            }
-
-                            sw.SpinOnce();
-                        }
-
+                        this.UpdateHistogramWithBucketsAndMinMax(number);
                         break;
                     }
             }
@@ -729,6 +656,108 @@ internal void TakeSnapshot(bool outputDelta)
             }
         }
 
+        private void UpdateHistogram(double number)
+        {
+            var sw = default(SpinWait);
+            while (true)
+            {
+                if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
+                {
+                    // Lock acquired
+                    unchecked
+                    {
+                        this.runningValue.AsLong++;
+                        this.histogramBuckets.RunningSum += number;
+                    }
+
+                    // Release lock
+                    Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
+                    break;
+                }
+
+                sw.SpinOnce();
+            }
+        }
+
+        private void UpdateHistogramWithMinMax(double number)
+        {
+            var sw = default(SpinWait);
+            while (true)
+            {
+                if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
+                {
+                    // Lock acquired
+                    unchecked
+                    {
+                        this.runningValue.AsLong++;
+                        this.histogramBuckets.RunningSum += number;
+                        this.histogramBuckets.RunningMin = Math.Min(this.histogramBuckets.RunningMin, number);
+                        this.histogramBuckets.RunningMax = Math.Max(this.histogramBuckets.RunningMax, number);
+                    }
+
+                    // Release lock
+                    Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
+                    break;
+                }
+
+                sw.SpinOnce();
+            }
+        }
+
+        private void UpdateHistogramWithBuckets(double number)
+        {
+            int i = this.histogramBuckets.FindBucketIndex(number);
+
+            var sw = default(SpinWait);
+            while (true)
+            {
+                if (Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 1) == 0)
+                {
+                    // Lock acquired
+                    unchecked
+                    {
+                        this.runningValue.AsLong++;
+                        this.histogramBuckets.RunningSum += number;
+                        this.histogramBuckets.RunningBucketCounts[i]++;
+                    }
+
+                    // Release lock
+                    Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
+                    break;
+                }
+
+                sw.SpinOnce();
+            }
+        }
+
+        private void UpdateHistogramWithBucketsAndMinMax(double number)
+        {
+            int i = this.histogramBuckets.FindBucketIndex(number);
+
+            var sw = default(SpinWait);
+            while (true)
+            {
+                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.RunningMin = Math.Min(this.histogramBuckets.RunningMin, number);
+                        this.histogramBuckets.RunningMax = Math.Max(this.histogramBuckets.RunningMax, number);
+                    }
+
+                    // Release lock
+                    Interlocked.Exchange(ref this.histogramBuckets.IsCriticalSectionOccupied, 0);
+                    break;
+                }
+
+                sw.SpinOnce();
+            }
+        }
+
         [MethodImpl(MethodImplOptions.NoInlining)]
         private readonly void ThrowNotSupportedMetricTypeException(string methodName)
         {

diff --git a/test/Benchmarks/README.md b/test/Benchmarks/README.md
@@ -3,11 +3,11 @@
 Navigate to `./test/Benchmarks` directory and run the following command:
 
 ```sh
-dotnet run -c Release -f net6.0 -- -m
+dotnet run -c Release -f net7.0 -- -m
 ```
 
 [How to use console arguments](https://benchmarkdotnet.org/articles/guides/console-args.html)
 
 - `-m` enables MemoryDiagnoser and prints memory statistics
 - `-f` allows you to filter the benchmarks by their full name using glob patterns
-  - `dotnet run -c Release -f net6.0 -- -f *TraceBenchmarks*`
+  - `dotnet run -c Release -f net7.0 -- -f *TraceBenchmarks*`
diff --git a/test/OpenTelemetry.Tests.Stress.Metrics/README.md b/test/OpenTelemetry.Tests.Stress.Metrics/README.md
@@ -14,5 +14,5 @@ for `Counter` and uncomment everything related to `Histogram` in the
 Open a console, run the following command from the current folder:
 
 ```sh
-dotnet run --framework net6.0 --configuration Release
+dotnet run --framework net7.0 --configuration Release
 ```