Merge branch 'main' into vibankwa/add-error-type-aspnetcore

open-telemetry · Nov 3, 2023 · b59075c · b59075c
2 parents a36104c + 37481f1
commit b59075c
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 43 deletions.
diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -17,6 +17,10 @@
   ([#5004](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5004))
   ([#5016](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5016))
 
+* Update `AggregatorStore` to provide known connection metrics with larger
+  histogram buckets.
+  ([#5008](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5008))
+
 ## 1.7.0-alpha.1
 
 Released 2023-Oct-16

diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs
@@ -372,10 +372,19 @@ internal MetricPointsAccessor GetMetricPoints()
 
     private static double[] FindDefaultHistogramBounds(in MetricStreamIdentity metricStreamIdentity)
     {
-        if (metricStreamIdentity.Unit == "s" && Metric.DefaultHistogramBoundMappings
-            .Contains((metricStreamIdentity.MeterName, metricStreamIdentity.InstrumentName)))
+        if (metricStreamIdentity.Unit == "s")
         {
-            return Metric.DefaultHistogramBoundsSeconds;
+            if (Metric.DefaultHistogramBoundShortMappings
+                .Contains((metricStreamIdentity.MeterName, metricStreamIdentity.InstrumentName)))
+            {
+                return Metric.DefaultHistogramBoundsShortSeconds;
+            }
+
+            if (Metric.DefaultHistogramBoundLongMappings
+                .Contains((metricStreamIdentity.MeterName, metricStreamIdentity.InstrumentName)))
+            {
+                return Metric.DefaultHistogramBoundsLongSeconds;
+            }
         }
 
         return Metric.DefaultHistogramBounds;

diff --git a/src/OpenTelemetry/Metrics/Metric.cs b/src/OpenTelemetry/Metrics/Metric.cs
@@ -28,23 +28,31 @@ public sealed class Metric
     internal const int DefaultExponentialHistogramMaxScale = 20;
 
     internal static readonly double[] DefaultHistogramBounds = new double[] { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000 };
-    internal static readonly double[] DefaultHistogramBoundsSeconds = new double[] { 0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10 };
-    internal static readonly HashSet<(string, string)> DefaultHistogramBoundMappings = new()
+
+    // Short default histogram bounds. Based on the recommended semantic convention values for http.server.request.duration.
+    internal static readonly double[] DefaultHistogramBoundsShortSeconds = new double[] { 0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10 };
+    internal static readonly HashSet<(string, string)> DefaultHistogramBoundShortMappings = new()
     {
         ("Microsoft.AspNetCore.Hosting", "http.server.request.duration"),
-        ("Microsoft.AspNetCore.Http.Connections", "signalr.server.connection.duration"),
         ("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request.time_in_queue"),
         ("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request_lease.duration"),
-        ("Microsoft.AspNetCore.Server.Kestrel", "kestrel.connection.duration"),
         ("Microsoft.AspNetCore.Server.Kestrel", "kestrel.tls_handshake.duration"),
         ("OpenTelemetry.Instrumentation.AspNetCore", "http.server.request.duration"),
         ("OpenTelemetry.Instrumentation.Http", "http.client.request.duration"),
-        ("System.Net.Http", "http.client.connection.duration"),
         ("System.Net.Http", "http.client.request.duration"),
         ("System.Net.Http", "http.client.request.time_in_queue"),
         ("System.Net.NameResolution", "dns.lookups.duration"),
     };
 
+    // Long default histogram bounds. Not based on a standard. May change in the future.
+    internal static readonly double[] DefaultHistogramBoundsLongSeconds = new double[] { 0, 0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 30, 60, 120, 300 };
+    internal static readonly HashSet<(string, string)> DefaultHistogramBoundLongMappings = new()
+    {
+        ("Microsoft.AspNetCore.Http.Connections", "signalr.server.connection.duration"),
+        ("Microsoft.AspNetCore.Server.Kestrel", "kestrel.connection.duration"),
+        ("System.Net.Http", "http.client.connection.duration"),
+    };
+
     private readonly AggregatorStore aggStore;
 
     internal Metric(

diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs
@@ -239,47 +239,49 @@ public void MultiThreadedHistogramUpdateAndSnapShotTest()
     }
 
     [Theory]
-    [InlineData("Microsoft.AspNetCore.Hosting", "http.server.request.duration")]
-    [InlineData("Microsoft.AspNetCore.Http.Connections", "signalr.server.connection.duration")]
-    [InlineData("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request_lease.duration")]
-    [InlineData("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request.time_in_queue")]
-    [InlineData("Microsoft.AspNetCore.Server.Kestrel", "kestrel.connection.duration")]
-    [InlineData("Microsoft.AspNetCore.Server.Kestrel", "kestrel.tls_handshake.duration")]
-    [InlineData("OpenTelemetry.Instrumentation.AspNetCore", "http.server.duration")]
-    [InlineData("OpenTelemetry.Instrumentation.Http", "http.client.duration")]
-    [InlineData("System.Net.Http", "http.client.connection.duration")]
-    [InlineData("System.Net.Http", "http.client.request.duration")]
-    [InlineData("System.Net.Http", "http.client.request.time_in_queue")]
-    [InlineData("System.Net.NameResolution", "dns.lookups.duration")]
-    [InlineData("General.App", "simple.alternative.counter")]
-    public void HistogramBucketsDefaultUpdatesForSecondsTest(string meterName, string instrumentName)
+    [InlineData("Microsoft.AspNetCore.Hosting", "http.server.request.duration", "s", KnownHistogramBuckets.DefaultShortSeconds)]
+    [InlineData("Microsoft.AspNetCore.Hosting", "http.server.request.duration", "ms", KnownHistogramBuckets.Default)]
+    [InlineData("Microsoft.AspNetCore.Hosting", "http.server.request.duration", "By", KnownHistogramBuckets.Default)]
+    [InlineData("Microsoft.AspNetCore.Hosting", "http.server.request.duration", null, KnownHistogramBuckets.Default)]
+    [InlineData("Microsoft.AspNetCore.Http.Connections", "signalr.server.connection.duration", "s", KnownHistogramBuckets.DefaultLongSeconds)]
+    [InlineData("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request_lease.duration", "s", KnownHistogramBuckets.DefaultShortSeconds)]
+    [InlineData("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request.time_in_queue", "s", KnownHistogramBuckets.DefaultShortSeconds)]
+    [InlineData("Microsoft.AspNetCore.Server.Kestrel", "kestrel.connection.duration", "s", KnownHistogramBuckets.DefaultLongSeconds)]
+    [InlineData("Microsoft.AspNetCore.Server.Kestrel", "kestrel.tls_handshake.duration", "s", KnownHistogramBuckets.DefaultShortSeconds)]
+    [InlineData("OpenTelemetry.Instrumentation.AspNetCore", "http.server.duration", "ms", KnownHistogramBuckets.Default)]
+    [InlineData("OpenTelemetry.Instrumentation.Http", "http.client.duration", "ms", KnownHistogramBuckets.Default)]
+    [InlineData("System.Net.Http", "http.client.connection.duration", "s", KnownHistogramBuckets.DefaultLongSeconds)]
+    [InlineData("System.Net.Http", "http.client.request.duration", "s", KnownHistogramBuckets.DefaultShortSeconds)]
+    [InlineData("System.Net.Http", "http.client.request.time_in_queue", "s", KnownHistogramBuckets.DefaultShortSeconds)]
+    [InlineData("System.Net.NameResolution", "dns.lookups.duration", "s", KnownHistogramBuckets.DefaultShortSeconds)]
+    [InlineData("General.App", "simple.alternative.counter", "s", KnownHistogramBuckets.Default)]
+    public void HistogramBucketsDefaultUpdatesForSecondsTest(string meterName, string instrumentName, string unit, KnownHistogramBuckets expectedHistogramBuckets)
     {
-        RunTest(meterName, instrumentName, unit: "s");
-        RunTest(meterName, instrumentName, unit: "ms");
-        RunTest(meterName, instrumentName, unit: "By");
-        RunTest(meterName, instrumentName, unit: null);
+        using var meter = new Meter(meterName);
 
-        void RunTest(string meterName, string instrumentName, string unit)
-        {
-            using var meter = new Meter(meterName);
-
-            var instrument = meter.CreateHistogram<double>(instrumentName, unit);
+        var instrument = meter.CreateHistogram<double>(instrumentName, unit);
 
-            var metricStreamIdentity = new MetricStreamIdentity(instrument, metricStreamConfiguration: null);
+        var metricStreamIdentity = new MetricStreamIdentity(instrument, metricStreamConfiguration: null);
 
-            AggregatorStore aggregatorStore = new(
-                metricStreamIdentity,
-                AggregationType.Histogram,
-                AggregationTemporality.Cumulative,
-                maxMetricPoints: 1024,
-                this.emitOverflowAttribute);
+        AggregatorStore aggregatorStore = new(
+            metricStreamIdentity,
+            AggregationType.Histogram,
+            AggregationTemporality.Cumulative,
+            maxMetricPoints: 1024,
+            this.emitOverflowAttribute);
 
-            Assert.NotNull(aggregatorStore.HistogramBounds);
-            Assert.Equal(
-                unit == "s" && Metric.DefaultHistogramBoundMappings.Contains((meterName, instrumentName)) ?
-                    Metric.DefaultHistogramBoundsSeconds : Metric.DefaultHistogramBounds,
-                aggregatorStore.HistogramBounds);
+        KnownHistogramBuckets actualHistogramBounds = KnownHistogramBuckets.Default;
+        if (aggregatorStore.HistogramBounds == Metric.DefaultHistogramBoundsShortSeconds)
+        {
+            actualHistogramBounds = KnownHistogramBuckets.DefaultShortSeconds;
         }
+        else if (aggregatorStore.HistogramBounds == Metric.DefaultHistogramBoundsLongSeconds)
+        {
+            actualHistogramBounds = KnownHistogramBuckets.DefaultLongSeconds;
+        }
+
+        Assert.NotNull(aggregatorStore.HistogramBounds);
+        Assert.Equal(expectedHistogramBuckets, actualHistogramBounds);
     }
 
     internal static void AssertExponentialBucketsAreCorrect(Base2ExponentialBucketHistogram expectedHistogram, ExponentialHistogramData data)

diff --git a/test/OpenTelemetry.Tests/Metrics/KnownHistogramBuckets.cs b/test/OpenTelemetry.Tests/Metrics/KnownHistogramBuckets.cs
@@ -0,0 +1,35 @@
+// <copyright file="KnownHistogramBuckets.cs" company="OpenTelemetry Authors">
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+// </copyright>
+
+namespace OpenTelemetry.Metrics.Tests;
+
+public enum KnownHistogramBuckets
+{
+    /// <summary>
+    /// Default OpenTelemetry semantic convention buckets.
+    /// </summary>
+    Default,
+
+    /// <summary>
+    /// Buckets for up to 10 seconds.
+    /// </summary>
+    DefaultShortSeconds,
+
+    /// <summary>
+    /// Buckets for up to 300 seconds.
+    /// </summary>
+    DefaultLongSeconds,
+}