Make exponential histogram negative buckets internal (#4494)

open-telemetry · May 16, 2023 · 30ad91a · 30ad91a
1 parent 8f513bb
commit 30ad91a
Show file tree

Hide file tree

Showing 11 changed files with 46 additions and 36 deletions.
diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs
@@ -347,13 +347,6 @@ internal static OtlpMetrics.Metric ToOtlpMetric(this Metric metric)
                                 dataPoint.Positive.BucketCounts.Add((ulong)bucketCount);
                             }
 
-                            dataPoint.Negative = new OtlpMetrics.ExponentialHistogramDataPoint.Types.Buckets();
-                            dataPoint.Negative.Offset = exponentialHistogramData.NegativeBuckets.Offset;
-                            foreach (var bucketCount in exponentialHistogramData.NegativeBuckets)
-                            {
-                                dataPoint.Negative.BucketCounts.Add((ulong)bucketCount);
-                            }
-
                             // TODO: exemplars.
 
                             histogram.DataPoints.Add(dataPoint);

diff --git a/src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt
@@ -28,7 +28,6 @@ OpenTelemetry.Metrics.ExponentialHistogramBuckets.Enumerator.MoveNext() -> bool
 OpenTelemetry.Metrics.ExponentialHistogramBuckets.GetEnumerator() -> OpenTelemetry.Metrics.ExponentialHistogramBuckets.Enumerator
 OpenTelemetry.Metrics.ExponentialHistogramBuckets.Offset.get -> int
 OpenTelemetry.Metrics.ExponentialHistogramData
-OpenTelemetry.Metrics.ExponentialHistogramData.NegativeBuckets.get -> OpenTelemetry.Metrics.ExponentialHistogramBuckets!
 OpenTelemetry.Metrics.ExponentialHistogramData.PositiveBuckets.get -> OpenTelemetry.Metrics.ExponentialHistogramBuckets!
 OpenTelemetry.Metrics.ExponentialHistogramData.Scale.get -> int
 OpenTelemetry.Metrics.ExponentialHistogramData.ZeroCount.get -> long

diff --git a/src/OpenTelemetry/.publicApi/net6.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net6.0/PublicAPI.Unshipped.txt
@@ -28,7 +28,6 @@ OpenTelemetry.Metrics.ExponentialHistogramBuckets.Enumerator.MoveNext() -> bool
 OpenTelemetry.Metrics.ExponentialHistogramBuckets.GetEnumerator() -> OpenTelemetry.Metrics.ExponentialHistogramBuckets.Enumerator
 OpenTelemetry.Metrics.ExponentialHistogramBuckets.Offset.get -> int
 OpenTelemetry.Metrics.ExponentialHistogramData
-OpenTelemetry.Metrics.ExponentialHistogramData.NegativeBuckets.get -> OpenTelemetry.Metrics.ExponentialHistogramBuckets!
 OpenTelemetry.Metrics.ExponentialHistogramData.PositiveBuckets.get -> OpenTelemetry.Metrics.ExponentialHistogramBuckets!
 OpenTelemetry.Metrics.ExponentialHistogramData.Scale.get -> int
 OpenTelemetry.Metrics.ExponentialHistogramData.ZeroCount.get -> long

diff --git a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
@@ -28,7 +28,6 @@ OpenTelemetry.Metrics.ExponentialHistogramBuckets.Enumerator.MoveNext() -> bool
 OpenTelemetry.Metrics.ExponentialHistogramBuckets.GetEnumerator() -> OpenTelemetry.Metrics.ExponentialHistogramBuckets.Enumerator
 OpenTelemetry.Metrics.ExponentialHistogramBuckets.Offset.get -> int
 OpenTelemetry.Metrics.ExponentialHistogramData
-OpenTelemetry.Metrics.ExponentialHistogramData.NegativeBuckets.get -> OpenTelemetry.Metrics.ExponentialHistogramBuckets!
 OpenTelemetry.Metrics.ExponentialHistogramData.PositiveBuckets.get -> OpenTelemetry.Metrics.ExponentialHistogramBuckets!
 OpenTelemetry.Metrics.ExponentialHistogramData.Scale.get -> int
 OpenTelemetry.Metrics.ExponentialHistogramData.ZeroCount.get -> long

diff --git a/src/OpenTelemetry/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt
@@ -28,7 +28,6 @@ OpenTelemetry.Metrics.ExponentialHistogramBuckets.Enumerator.MoveNext() -> bool
 OpenTelemetry.Metrics.ExponentialHistogramBuckets.GetEnumerator() -> OpenTelemetry.Metrics.ExponentialHistogramBuckets.Enumerator
 OpenTelemetry.Metrics.ExponentialHistogramBuckets.Offset.get -> int
 OpenTelemetry.Metrics.ExponentialHistogramData
-OpenTelemetry.Metrics.ExponentialHistogramData.NegativeBuckets.get -> OpenTelemetry.Metrics.ExponentialHistogramBuckets!
 OpenTelemetry.Metrics.ExponentialHistogramData.PositiveBuckets.get -> OpenTelemetry.Metrics.ExponentialHistogramBuckets!
 OpenTelemetry.Metrics.ExponentialHistogramData.Scale.get -> int
 OpenTelemetry.Metrics.ExponentialHistogramData.ZeroCount.get -> long

diff --git a/src/OpenTelemetry/Metrics/ExponentialHistogramData.cs b/src/OpenTelemetry/Metrics/ExponentialHistogramData.cs
@@ -32,7 +32,7 @@ internal ExponentialHistogramData()
 
     public ExponentialHistogramBuckets PositiveBuckets { get; private set; }
 
-    public ExponentialHistogramBuckets NegativeBuckets { get; private set; }
+    internal ExponentialHistogramBuckets NegativeBuckets { get; private set; }
 
     internal ExponentialHistogramData Copy()
     {

diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs
@@ -1579,6 +1579,11 @@ private void UpdateHistogramWithBucketsAndMinMax(double number, ReadOnlySpan<Key
         private void UpdateBase2ExponentialHistogram(double number, ReadOnlySpan<KeyValuePair<string, object>> tags = default, bool reportExemplar = false)
 #pragma warning restore IDE0060 // Remove unused parameter
         {
+            if (number < 0)
+            {
+                return;
+            }
+
             var histogram = this.mpComponents!.Base2ExponentialBucketHistogram;
 
             var sw = default(SpinWait);
@@ -1607,6 +1612,11 @@ private void UpdateBase2ExponentialHistogram(double number, ReadOnlySpan<KeyValu
         private void UpdateBase2ExponentialHistogramWithMinMax(double number, ReadOnlySpan<KeyValuePair<string, object>> tags = default, bool reportExemplar = false)
 #pragma warning restore IDE0060 // Remove unused parameter
         {
+            if (number < 0)
+            {
+                return;
+            }
+
             var histogram = this.mpComponents!.Base2ExponentialBucketHistogram;
 
             var sw = default(SpinWait);

diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs
@@ -544,43 +544,46 @@ public void TestExponentialHistogramToOtlpMetric(string name, string description
             Assert.True(dataPoint.TimeUnixNano > 0);
 
             Assert.Equal(20, dataPoint.Scale);
-            Assert.Equal(2UL, dataPoint.Count);
             Assert.Equal(1UL, dataPoint.ZeroCount);
+            if (longValue > 0 || doubleValue > 0)
+            {
+                Assert.Equal(2UL, dataPoint.Count);
+            }
+            else
+            {
+                Assert.Equal(1UL, dataPoint.Count);
+            }
 
             if (longValue.HasValue)
             {
-                // Known issue: Negative measurements affect the Sum. Per the spec, they should not.
-                Assert.Equal((double)longValue, dataPoint.Sum);
                 if (longValue > 0)
                 {
+                    Assert.Equal((double)longValue, dataPoint.Sum);
+                    Assert.Null(dataPoint.Negative);
                     Assert.True(dataPoint.Positive.Offset > 0);
                     Assert.Equal(1UL, dataPoint.Positive.BucketCounts[0]);
-                    Assert.True(dataPoint.Negative.Offset == 0);
-                    Assert.Empty(dataPoint.Negative.BucketCounts);
                 }
                 else
                 {
-                    Assert.True(dataPoint.Negative.Offset > 0);
-                    Assert.Equal(1UL, dataPoint.Negative.BucketCounts[0]);
+                    Assert.Equal(0, dataPoint.Sum);
+                    Assert.Null(dataPoint.Negative);
                     Assert.True(dataPoint.Positive.Offset == 0);
                     Assert.Empty(dataPoint.Positive.BucketCounts);
                 }
             }
             else
             {
-                // Known issue: Negative measurements affect the Sum. Per the spec, they should not.
-                Assert.Equal(doubleValue, dataPoint.Sum);
                 if (doubleValue > 0)
                 {
+                    Assert.Equal(doubleValue, dataPoint.Sum);
+                    Assert.Null(dataPoint.Negative);
                     Assert.True(dataPoint.Positive.Offset > 0);
                     Assert.Equal(1UL, dataPoint.Positive.BucketCounts[0]);
-                    Assert.True(dataPoint.Negative.Offset == 0);
-                    Assert.Empty(dataPoint.Negative.BucketCounts);
                 }
                 else
                 {
-                    Assert.True(dataPoint.Negative.Offset > 0);
-                    Assert.Equal(1UL, dataPoint.Negative.BucketCounts[0]);
+                    Assert.Equal(0, dataPoint.Sum);
+                    Assert.Null(dataPoint.Negative);
                     Assert.True(dataPoint.Positive.Offset == 0);
                     Assert.Empty(dataPoint.Positive.BucketCounts);
                 }

diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs
@@ -291,7 +291,11 @@ internal void ExponentialHistogramTests(AggregationType aggregationType, Aggrega
             foreach (var value in valuesToRecord)
             {
                 aggregatorStore.Update(value, Array.Empty<KeyValuePair<string, object>>());
-                expectedHistogram.Record(value);
+
+                if (value >= 0)
+                {
+                    expectedHistogram.Record(value);
+                }
             }
 
             aggregatorStore.Snapshot();
@@ -311,13 +315,13 @@ internal void ExponentialHistogramTests(AggregationType aggregationType, Aggrega
             var hasMinMax = metricPoint.TryGetHistogramMinMaxValues(out var min, out var max);
 
             AssertExponentialBucketsAreCorrect(expectedHistogram, metricPoint.GetExponentialHistogramData());
-            Assert.Equal(40, sum);
-            Assert.Equal(7, count);
+            Assert.Equal(50, sum);
+            Assert.Equal(6, count);
 
             if (aggregationType == AggregationType.Base2ExponentialHistogramWithMinMax)
             {
                 Assert.True(hasMinMax);
-                Assert.Equal(-10, min);
+                Assert.Equal(0, min);
                 Assert.Equal(19, max);
             }
             else
@@ -334,13 +338,13 @@ internal void ExponentialHistogramTests(AggregationType aggregationType, Aggrega
             if (aggregationTemporality == AggregationTemporality.Cumulative)
             {
                 AssertExponentialBucketsAreCorrect(expectedHistogram, metricPoint.GetExponentialHistogramData());
-                Assert.Equal(40, sum);
-                Assert.Equal(7, count);
+                Assert.Equal(50, sum);
+                Assert.Equal(6, count);
 
                 if (aggregationType == AggregationType.Base2ExponentialHistogramWithMinMax)
                 {
                     Assert.True(hasMinMax);
-                    Assert.Equal(-10, min);
+                    Assert.Equal(0, min);
                     Assert.Equal(19, max);
                 }
                 else

diff --git a/test/OpenTelemetry.Tests/Metrics/MetricTestData.cs b/test/OpenTelemetry.Tests/Metrics/MetricTestData.cs
@@ -58,7 +58,7 @@ public static IEnumerable<object[]> ValidHistogramMinMax
                     new object[] { new double[] { double.NegativeInfinity, 0, double.PositiveInfinity }, new HistogramConfiguration(), double.NegativeInfinity, double.PositiveInfinity },
                     new object[] { new double[] { 1 }, new HistogramConfiguration(), 1, 1 },
                     new object[] { new double[] { 5, 100, 4, 101, -2, 97 }, new ExplicitBucketHistogramConfiguration() { Boundaries = new double[] { 10, 20 } }, -2, 101 },
-                    new object[] { new double[] { 5, 100, 4, 101, -2, 97 }, new Base2ExponentialBucketHistogramConfiguration(), -2, 101 },
+                    new object[] { new double[] { 5, 100, 4, 101, -2, 97 }, new Base2ExponentialBucketHistogramConfiguration(), 4, 101 },
            };
 
         public static IEnumerable<object[]> InvalidHistogramMinMax

diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs
@@ -611,7 +611,11 @@ public void ViewToProduceExponentialHistogram()
             foreach (var value in valuesToRecord)
             {
                 histogram.Record(value);
-                expectedHistogram.Record(value);
+
+                if (value >= 0)
+                {
+                    expectedHistogram.Record(value);
+                }
             }
 
             meterProvider.ForceFlush(MaxTimeToAllowForFlush);
@@ -633,8 +637,8 @@ public void ViewToProduceExponentialHistogram()
             var sum = metricPoint.GetHistogramSum();
 
             AggregatorTest.AssertExponentialBucketsAreCorrect(expectedHistogram, metricPoint.GetExponentialHistogramData());
-            Assert.Equal(40, sum);
-            Assert.Equal(7, count);
+            Assert.Equal(50, sum);
+            Assert.Equal(6, count);
         }
 
         [Theory]