Histogram MinMax API fixes (#3822)

open-telemetry · Oct 26, 2022 · 88c1864 · 88c1864
1 parent 004c3f6
commit 88c1864
Show file tree

Hide file tree

Showing 8 changed files with 35 additions and 60 deletions.
diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs
@@ -98,9 +98,9 @@ public override ExportResult Export(in Batch<Metric> batch)
                         var sum = metricPoint.GetHistogramSum();
                         var count = metricPoint.GetHistogramCount();
                         bucketsBuilder.Append($"Sum: {sum} Count: {count} ");
-                        if (metricPoint.HasMinMax())
+                        if (metricPoint.TryGetHistogramMinMaxValues(out double min, out double max))
                         {
-                            bucketsBuilder.Append($"Min: {metricPoint.GetHistogramMin()} Max: {metricPoint.GetHistogramMax()} ");
+                            bucketsBuilder.Append($"Min: {min} Max: {max} ");
                         }
 
                         bucketsBuilder.AppendLine();

diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs
@@ -257,10 +257,10 @@ internal static OtlpMetrics.Metric ToOtlpMetric(this Metric metric)
                             dataPoint.Count = (ulong)metricPoint.GetHistogramCount();
                             dataPoint.Sum = metricPoint.GetHistogramSum();
 
-                            if (metricPoint.HasMinMax())
+                            if (metricPoint.TryGetHistogramMinMaxValues(out double min, out double max))
                             {
-                                dataPoint.Min = metricPoint.GetHistogramMin();
-                                dataPoint.Max = metricPoint.GetHistogramMax();
+                                dataPoint.Min = min;
+                                dataPoint.Max = max;
                             }
 
                             foreach (var histogramMeasurement in metricPoint.GetHistogramBuckets())

diff --git a/src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt
@@ -14,9 +14,7 @@ OpenTelemetry.Metrics.HistogramConfiguration
 OpenTelemetry.Metrics.HistogramConfiguration.HistogramConfiguration() -> void
 OpenTelemetry.Metrics.HistogramConfiguration.RecordMinMax.get -> bool
 OpenTelemetry.Metrics.HistogramConfiguration.RecordMinMax.set -> void
-OpenTelemetry.Metrics.MetricPoint.GetHistogramMax() -> double
-OpenTelemetry.Metrics.MetricPoint.GetHistogramMin() -> double
-OpenTelemetry.Metrics.MetricPoint.HasMinMax() -> bool
+OpenTelemetry.Metrics.MetricPoint.TryGetHistogramMinMaxValues(out double min, out double max) -> bool
 OpenTelemetry.Resources.ResourceBuilder.AddDetector(System.Func<System.IServiceProvider?, OpenTelemetry.Resources.IResourceDetector!>! resourceDetectorFactory) -> OpenTelemetry.Resources.ResourceBuilder!
 OpenTelemetry.Trace.BatchExportActivityProcessorOptions.BatchExportActivityProcessorOptions(Microsoft.Extensions.Configuration.IConfiguration! configuration) -> void
 static Microsoft.Extensions.DependencyInjection.MeterProviderBuilderServiceCollectionExtensions.ConfigureOpenTelemetryMetrics(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.Extensions.DependencyInjection.IServiceCollection!

diff --git a/src/OpenTelemetry/.publicApi/net6.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net6.0/PublicAPI.Unshipped.txt
@@ -14,9 +14,7 @@ OpenTelemetry.Metrics.HistogramConfiguration
 OpenTelemetry.Metrics.HistogramConfiguration.HistogramConfiguration() -> void
 OpenTelemetry.Metrics.HistogramConfiguration.RecordMinMax.get -> bool
 OpenTelemetry.Metrics.HistogramConfiguration.RecordMinMax.set -> void
-OpenTelemetry.Metrics.MetricPoint.GetHistogramMax() -> double
-OpenTelemetry.Metrics.MetricPoint.GetHistogramMin() -> double
-OpenTelemetry.Metrics.MetricPoint.HasMinMax() -> bool
+OpenTelemetry.Metrics.MetricPoint.TryGetHistogramMinMaxValues(out double min, out double max) -> bool
 OpenTelemetry.Resources.ResourceBuilder.AddDetector(System.Func<System.IServiceProvider?, OpenTelemetry.Resources.IResourceDetector!>! resourceDetectorFactory) -> OpenTelemetry.Resources.ResourceBuilder!
 OpenTelemetry.Trace.BatchExportActivityProcessorOptions.BatchExportActivityProcessorOptions(Microsoft.Extensions.Configuration.IConfiguration! configuration) -> void
 static Microsoft.Extensions.DependencyInjection.MeterProviderBuilderServiceCollectionExtensions.ConfigureOpenTelemetryMetrics(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.Extensions.DependencyInjection.IServiceCollection!

diff --git a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
@@ -14,9 +14,7 @@ OpenTelemetry.Metrics.HistogramConfiguration
 OpenTelemetry.Metrics.HistogramConfiguration.HistogramConfiguration() -> void
 OpenTelemetry.Metrics.HistogramConfiguration.RecordMinMax.get -> bool
 OpenTelemetry.Metrics.HistogramConfiguration.RecordMinMax.set -> void
-OpenTelemetry.Metrics.MetricPoint.GetHistogramMax() -> double
-OpenTelemetry.Metrics.MetricPoint.GetHistogramMin() -> double
-OpenTelemetry.Metrics.MetricPoint.HasMinMax() -> bool
+OpenTelemetry.Metrics.MetricPoint.TryGetHistogramMinMaxValues(out double min, out double max) -> bool
 OpenTelemetry.Resources.ResourceBuilder.AddDetector(System.Func<System.IServiceProvider?, OpenTelemetry.Resources.IResourceDetector!>! resourceDetectorFactory) -> OpenTelemetry.Resources.ResourceBuilder!
 OpenTelemetry.Trace.BatchExportActivityProcessorOptions.BatchExportActivityProcessorOptions(Microsoft.Extensions.Configuration.IConfiguration! configuration) -> void
 static Microsoft.Extensions.DependencyInjection.MeterProviderBuilderServiceCollectionExtensions.ConfigureOpenTelemetryMetrics(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.Extensions.DependencyInjection.IServiceCollection!

diff --git a/src/OpenTelemetry/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt
@@ -14,9 +14,7 @@ OpenTelemetry.Metrics.HistogramConfiguration
 OpenTelemetry.Metrics.HistogramConfiguration.HistogramConfiguration() -> void
 OpenTelemetry.Metrics.HistogramConfiguration.RecordMinMax.get -> bool
 OpenTelemetry.Metrics.HistogramConfiguration.RecordMinMax.set -> void
-OpenTelemetry.Metrics.MetricPoint.GetHistogramMax() -> double
-OpenTelemetry.Metrics.MetricPoint.GetHistogramMin() -> double
-OpenTelemetry.Metrics.MetricPoint.HasMinMax() -> bool
+OpenTelemetry.Metrics.MetricPoint.TryGetHistogramMinMaxValues(out double min, out double max) -> bool
 OpenTelemetry.Resources.ResourceBuilder.AddDetector(System.Func<System.IServiceProvider?, OpenTelemetry.Resources.IResourceDetector!>! resourceDetectorFactory) -> OpenTelemetry.Resources.ResourceBuilder!
 OpenTelemetry.Trace.BatchExportActivityProcessorOptions.BatchExportActivityProcessorOptions(Microsoft.Extensions.Configuration.IConfiguration! configuration) -> void
 static Microsoft.Extensions.DependencyInjection.MeterProviderBuilderServiceCollectionExtensions.ConfigureOpenTelemetryMetrics(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.Extensions.DependencyInjection.IServiceCollection!

diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs
@@ -243,44 +243,27 @@ public readonly HistogramBuckets GetHistogramBuckets()
         }
 
         /// <summary>
-        /// Gets the minimum value of the histogram associated with the metric
-        /// point if present. Check by using <see cref="HasMinMax"/>.
+        /// Gets the Histogram Min and Max values.
         /// </summary>
-        /// <remarks>
-        /// Applies to <see cref="MetricType.Histogram"/> metric type.
-        /// </remarks>
-        /// <returns>A histogram's maximum value.</returns>
+        /// <param name="min"> The histogram minimum value.</param>
+        /// <param name="max"> The histogram maximum value.</param>
+        /// <returns>True if minimum and maximum value exist, false otherwise.</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public double GetHistogramMin()
+        public bool TryGetHistogramMinMaxValues(out double min, out double max)
         {
-            return this.histogramBuckets.SnapshotMin;
-        }
+            if (this.aggType == AggregationType.HistogramWithMinMax ||
+                            this.aggType == AggregationType.HistogramWithMinMaxBuckets)
+            {
+                Debug.Assert(this.histogramBuckets != null, "histogramBuckets was null");
 
-        /// <summary>
-        /// Gets the maximum value of the histogram associated with the metric
-        /// point if present. Check by using <see cref="HasMinMax"/>.
-        /// </summary>
-        /// <remarks>
-        /// Applies to <see cref="MetricType.Histogram"/> metric type.
-        /// </remarks>
-        /// <returns>A histogram's maximum value.</returns>
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public double GetHistogramMax()
-        {
-            return this.histogramBuckets.SnapshotMax;
-        }
+                min = this.histogramBuckets.SnapshotMin;
+                max = this.histogramBuckets.SnapshotMax;
+                return true;
+            }
 
-        /// <summary>
-        /// Gets whether the histogram has a valid Min and Max value.
-        /// Should be used before
-        /// <see cref="GetHistogramMin"/> and <see cref="GetHistogramMax"/> to
-        /// ensure a valid value is returned.
-        /// </summary>
-        /// <returns>A minimum and maximum value exist.</returns>
-        public bool HasMinMax()
-        {
-            return this.aggType == AggregationType.HistogramWithMinMaxBuckets ||
-                   this.aggType == AggregationType.HistogramWithMinMax;
+            min = 0;
+            max = 0;
+            return false;
         }
 
         internal readonly MetricPoint Copy()

diff --git a/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs b/test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs
@@ -599,14 +599,15 @@ public void HistogramMinMax(double[] values, HistogramConfiguration histogramCon
             }
 
             var histogramPoint = metricPoints[0];
-            var hasMinMax = histogramPoint.HasMinMax();
-            Assert.True(hasMinMax);
-
-            var min = histogramPoint.GetHistogramMin();
-            var max = histogramPoint.GetHistogramMax();
-
-            Assert.Equal(expectedMin, min);
-            Assert.Equal(expectedMax, max);
+            if (histogramPoint.TryGetHistogramMinMaxValues(out double min, out double max))
+            {
+                Assert.Equal(expectedMin, min);
+                Assert.Equal(expectedMax, max);
+            }
+            else
+            {
+                Assert.Fail("MinMax expected");
+            }
         }
 
         [Theory]
@@ -636,8 +637,7 @@ public void HistogramMinMaxNotPresent(double[] values, HistogramConfiguration hi
             }
 
             var histogramPoint = metricPoints[0];
-
-            Assert.False(histogramPoint.HasMinMax());
+            Assert.False(histogramPoint.TryGetHistogramMinMaxValues(out double _, out double _));
         }
 
         [Fact]