From 30ad91af37443ad3d346d922dbc32a01614fe590 Mon Sep 17 00:00:00 2001 From: Alan West <3676547+alanwest@users.noreply.github.com> Date: Tue, 16 May 2023 14:45:33 -0700 Subject: [PATCH] Make exponential histogram negative buckets internal (#4494) --- .../Implementation/MetricItemExtensions.cs | 7 ----- .../.publicApi/net462/PublicAPI.Unshipped.txt | 1 - .../.publicApi/net6.0/PublicAPI.Unshipped.txt | 1 - .../netstandard2.0/PublicAPI.Unshipped.txt | 1 - .../netstandard2.1/PublicAPI.Unshipped.txt | 1 - .../Metrics/ExponentialHistogramData.cs | 2 +- src/OpenTelemetry/Metrics/MetricPoint.cs | 10 +++++++ .../OtlpMetricsExporterTests.cs | 29 ++++++++++--------- .../Metrics/AggregatorTest.cs | 18 +++++++----- .../Metrics/MetricTestData.cs | 2 +- .../Metrics/MetricViewTests.cs | 10 +++++-- 11 files changed, 46 insertions(+), 36 deletions(-) diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs index 44f096c22f2..5ac61d2e068 100644 --- 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 index ae90dbc9e28..846034361b3 100644 --- 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 index ae90dbc9e28..846034361b3 100644 --- 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 index ae90dbc9e28..846034361b3 100644 --- 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 index ae90dbc9e28..846034361b3 100644 --- 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 index 6fe8d145775..2cae22cb512 100644 --- 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 index 69e1e9b2b58..f0c3a0b70f0 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -1579,6 +1579,11 @@ private void UpdateHistogramWithBucketsAndMinMax(double number, ReadOnlySpan> 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> 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 index 62e3c10d6b7..465901e6c01 100644 --- 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 index bbf9af4cc9f..94ce11b76a8 100644 --- 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>()); - 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 index ed5361eb71e..4c08b1e5c21 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricTestData.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricTestData.cs @@ -58,7 +58,7 @@ public static IEnumerable 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 InvalidHistogramMinMax diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs index 579922f5965..16f2568abe8 100644 --- 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]