[sdk-metrics] Improve the perf of histogram snapshots (#5366)

open-telemetry · Feb 16, 2024 · dd877fe · dd877fe
1 parent 5849d3a
commit dd877fe
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 56 deletions.
diff --git a/src/OpenTelemetry/Metrics/HistogramBuckets.cs b/src/OpenTelemetry/Metrics/HistogramBuckets.cs
@@ -16,8 +16,7 @@ public class HistogramBuckets
 
     internal readonly double[]? ExplicitBounds;
 
-    internal readonly long[]? RunningBucketCounts;
-    internal readonly long[] SnapshotBucketCounts;
+    internal readonly HistogramBucketValues[] BucketCounts;
 
     internal double RunningSum;
     internal double SnapshotSum;
@@ -62,8 +61,7 @@ internal HistogramBuckets(double[]? explicitBounds)
             }
         }
 
-        this.RunningBucketCounts = explicitBounds != null ? new long[explicitBounds.Length + 1] : null;
-        this.SnapshotBucketCounts = explicitBounds != null ? new long[explicitBounds.Length + 1] : new long[0];
+        this.BucketCounts = explicitBounds != null ? new HistogramBucketValues[explicitBounds.Length + 1] : Array.Empty<HistogramBucketValues>();
     }
 
     /// <summary>
@@ -76,7 +74,7 @@ internal HistogramBuckets Copy()
     {
         HistogramBuckets copy = new HistogramBuckets(this.ExplicitBounds);
 
-        Array.Copy(this.SnapshotBucketCounts, copy.SnapshotBucketCounts, this.SnapshotBucketCounts.Length);
+        Array.Copy(this.BucketCounts, copy.BucketCounts, this.BucketCounts.Length);
         copy.SnapshotSum = this.SnapshotSum;
         copy.SnapshotMin = this.SnapshotMin;
         copy.SnapshotMax = this.SnapshotMax;
@@ -137,6 +135,31 @@ internal int FindBucketIndexLinear(double value)
         return i;
     }
 
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    internal void Snapshot(bool outputDelta)
+    {
+        var bucketCounts = this.BucketCounts;
+
+        if (outputDelta)
+        {
+            for (int i = 0; i < bucketCounts.Length; i++)
+            {
+                ref var values = ref bucketCounts[i];
+                ref var running = ref values.RunningValue;
+                values.SnapshotValue = running;
+                running = 0L;
+            }
+        }
+        else
+        {
+            for (int i = 0; i < bucketCounts.Length; i++)
+            {
+                ref var values = ref bucketCounts[i];
+                values.SnapshotValue = values.RunningValue;
+            }
+        }
+    }
+
     /// <summary>
     /// Enumerates the elements of a <see cref="HistogramBuckets"/>.
     /// </summary>
@@ -152,7 +175,7 @@ internal Enumerator(HistogramBuckets histogramMeasurements)
             this.histogramMeasurements = histogramMeasurements;
             this.index = 0;
             this.Current = default;
-            this.numberOfBuckets = histogramMeasurements.SnapshotBucketCounts.Length;
+            this.numberOfBuckets = histogramMeasurements.BucketCounts.Length;
         }
 
         /// <summary>
@@ -175,7 +198,7 @@ public bool MoveNext()
                 double explicitBound = this.index < this.numberOfBuckets - 1
                     ? this.histogramMeasurements.ExplicitBounds![this.index]
                     : double.PositiveInfinity;
-                long bucketCount = this.histogramMeasurements.SnapshotBucketCounts[this.index];
+                long bucketCount = this.histogramMeasurements.BucketCounts[this.index].SnapshotValue;
                 this.Current = new HistogramBucket(explicitBound, bucketCount);
                 this.index++;
                 return true;
@@ -185,6 +208,12 @@ public bool MoveNext()
         }
     }
 
+    internal struct HistogramBucketValues
+    {
+        public long RunningValue;
+        public long SnapshotValue;
+    }
+
     private sealed class BucketLookupNode
     {
         public double UpperBoundInclusive { get; set; }

diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs
@@ -915,16 +915,7 @@ internal void TakeSnapshot(bool outputDelta)
                         histogramBuckets.RunningSum = 0;
                     }
 
-                    Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null");
-
-                    for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++)
-                    {
-                        histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i];
-                        if (outputDelta)
-                        {
-                            histogramBuckets.RunningBucketCounts[i] = 0;
-                        }
-                    }
+                    histogramBuckets.Snapshot(outputDelta);
 
                     this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta);
 
@@ -979,16 +970,7 @@ internal void TakeSnapshot(bool outputDelta)
                         histogramBuckets.RunningMax = double.NegativeInfinity;
                     }
 
-                    Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null");
-
-                    for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++)
-                    {
-                        histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i];
-                        if (outputDelta)
-                        {
-                            histogramBuckets.RunningBucketCounts[i] = 0;
-                        }
-                    }
+                    histogramBuckets.Snapshot(outputDelta);
 
                     this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta);
                     this.MetricPointStatus = MetricPointStatus.NoCollectPending;
@@ -1181,16 +1163,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta)
                         histogramBuckets.RunningSum = 0;
                     }
 
-                    Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null");
-
-                    for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++)
-                    {
-                        histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i];
-                        if (outputDelta)
-                        {
-                            histogramBuckets.RunningBucketCounts[i] = 0;
-                        }
-                    }
+                    histogramBuckets.Snapshot(outputDelta);
 
                     this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta);
 
@@ -1247,16 +1220,7 @@ internal void TakeSnapshotWithExemplar(bool outputDelta)
                         histogramBuckets.RunningMax = double.NegativeInfinity;
                     }
 
-                    Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null");
-
-                    for (int i = 0; i < histogramBuckets.RunningBucketCounts!.Length; i++)
-                    {
-                        histogramBuckets.SnapshotBucketCounts[i] = histogramBuckets.RunningBucketCounts[i];
-                        if (outputDelta)
-                        {
-                            histogramBuckets.RunningBucketCounts[i] = 0;
-                        }
-                    }
+                    histogramBuckets.Snapshot(outputDelta);
 
                     this.mpComponents.Exemplars = this.mpComponents.ExemplarReservoir?.Collect(this.Tags, outputDelta);
                     this.MetricPointStatus = MetricPointStatus.NoCollectPending;
@@ -1429,15 +1393,13 @@ private void UpdateHistogramWithBuckets(double number, ReadOnlySpan<KeyValuePair
 
         int i = histogramBuckets!.FindBucketIndex(number);
 
-        Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null");
-
         AcquireLock(ref histogramBuckets.IsCriticalSectionOccupied);
 
         unchecked
         {
             this.runningValue.AsLong++;
             histogramBuckets.RunningSum += number;
-            histogramBuckets.RunningBucketCounts![i]++;
+            histogramBuckets.BucketCounts[i].RunningValue++;
 
             if (reportExemplar && isSampled)
             {
@@ -1462,13 +1424,11 @@ private void UpdateHistogramWithBucketsAndMinMax(double number, ReadOnlySpan<Key
 
         AcquireLock(ref histogramBuckets.IsCriticalSectionOccupied);
 
-        Debug.Assert(histogramBuckets.RunningBucketCounts != null, "histogramBuckets.RunningBucketCounts was null");
-
         unchecked
         {
             this.runningValue.AsLong++;
             histogramBuckets.RunningSum += number;
-            histogramBuckets.RunningBucketCounts![i]++;
+            histogramBuckets.BucketCounts[i].RunningValue++;
 
             if (reportExemplar && isSampled)
             {

diff --git a/test/OpenTelemetry.Tests/Metrics/MetricApiTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/MetricApiTestsBase.cs
@@ -1771,7 +1771,7 @@ private void MultithreadedHistogramTest<T>(long[] expected, T[] values)
         {
             foreach (var metricPoint in metric.GetMetricPoints())
             {
-                bucketCounts = metricPoint.GetHistogramBuckets().RunningBucketCounts;
+                bucketCounts = metricPoint.GetHistogramBuckets().BucketCounts.Select(v => v.RunningValue).ToArray();
             }
         }
 

diff --git a/test/OpenTelemetry.Tests/Metrics/MetricPointTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricPointTests.cs
@@ -77,8 +77,8 @@ public void VerifyHistogramBucketsCopy()
         Assert.NotSame(copy, histogramBuckets);
 
         // Verify fields are copied
-        Assert.NotSame(copy.SnapshotBucketCounts, histogramBuckets.SnapshotBucketCounts);
-        Assert.Equal(copy.SnapshotBucketCounts, histogramBuckets.SnapshotBucketCounts);
+        Assert.NotSame(copy.BucketCounts, histogramBuckets.BucketCounts);
+        Assert.Equal(copy.BucketCounts, histogramBuckets.BucketCounts);
         Assert.Equal(copy.SnapshotSum, histogramBuckets.SnapshotSum);
     }
 }
-Original file line number
+Diff line change
@@ Expand Up @@
             {
                 foreach (var metricPoint in metric.GetMetricPoints())
                 {
-                    bucketCounts = metricPoint.GetHistogramBuckets().RunningBucketCounts;
+                    bucketCounts = metricPoint.GetHistogramBuckets().BucketCounts.Select(v => v.RunningValue).ToArray();
                 }
             }
@@ Expand Down @@