From bc1360843a4a384178b65a7e243bbd3d609f0395 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Thu, 2 Nov 2023 18:26:04 -0700 Subject: [PATCH 1/6] Update histogram buckets --- src/OpenTelemetry/CHANGELOG.md | 9 +++++++-- src/OpenTelemetry/Metrics/Metric.cs | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 2e6b2a17876..f7db15a77c3 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -17,8 +17,13 @@ ([#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. +* Update Metrics SDK to override the default histogram buckets for the following + metrics from ASP.NET Core and HttpClient runtime: + * `signalr.server.connection.duration` + * `kestrel.connection.duration` + * `http.client.connection.duration` These histogram metrics which have their + `Unit` as `s` (second) will have their default histogram buckets as `[ 0.01, + 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 30, 60, 120, 300 ]`. ([#5008](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5008)) ## 1.7.0-alpha.1 diff --git a/src/OpenTelemetry/Metrics/Metric.cs b/src/OpenTelemetry/Metrics/Metric.cs index 636755ed3b4..d515e98fe74 100644 --- a/src/OpenTelemetry/Metrics/Metric.cs +++ b/src/OpenTelemetry/Metrics/Metric.cs @@ -30,7 +30,7 @@ public sealed class Metric internal static readonly double[] DefaultHistogramBounds = new double[] { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000 }; // 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 double[] DefaultHistogramBoundsShortSeconds = new double[] { 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"), @@ -45,7 +45,7 @@ public sealed class Metric }; // 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 double[] DefaultHistogramBoundsLongSeconds = new double[] { 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"), From 9a2e791c0a1714f5dc9ef6dd4ef41704dd421d54 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Thu, 2 Nov 2023 18:29:32 -0700 Subject: [PATCH 2/6] Update CHANGELOG --- src/OpenTelemetry/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index f7db15a77c3..4fa69e99f35 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -26,6 +26,10 @@ 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 30, 60, 120, 300 ]`. ([#5008](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5008)) +* Remove the bucket with value `0` for histogram buckets for metrics from + ASP.NET Core and HttpClient. + ([#5021](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5021)) + ## 1.7.0-alpha.1 Released 2023-Oct-16 From 931401eed3dc0f51d5c1d3f93ae6b74e62aee157 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Thu, 2 Nov 2023 18:31:19 -0700 Subject: [PATCH 3/6] Update CHANGELOG --- src/OpenTelemetry/CHANGELOG.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 4fa69e99f35..937933c259d 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -21,9 +21,11 @@ metrics from ASP.NET Core and HttpClient runtime: * `signalr.server.connection.duration` * `kestrel.connection.duration` - * `http.client.connection.duration` These histogram metrics which have their - `Unit` as `s` (second) will have their default histogram buckets as `[ 0.01, - 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 30, 60, 120, 300 ]`. + * `http.client.connection.duration` + + These histogram metrics which have their `Unit` as `s` (second) will have + their default histogram buckets as `[ 0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, + 5, 10, 30, 60, 120, 300 ]`. ([#5008](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5008)) * Remove the bucket with value `0` for histogram buckets for metrics from From 33c65a622aabf0212462455a37ef4b0740cefcca Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Thu, 2 Nov 2023 18:33:06 -0700 Subject: [PATCH 4/6] Fix CI --- src/OpenTelemetry/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 937933c259d..c5205f289e5 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -22,7 +22,7 @@ * `signalr.server.connection.duration` * `kestrel.connection.duration` * `http.client.connection.duration` - + These histogram metrics which have their `Unit` as `s` (second) will have their default histogram buckets as `[ 0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 30, 60, 120, 300 ]`. From aeba42fac51f30c69c5ad265b2959b27a761803e Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Thu, 2 Nov 2023 19:09:08 -0700 Subject: [PATCH 5/6] Update unit tests --- .../MetricTests.cs | 2 +- .../OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index d6f72df7c62..39fb8bbfd24 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -558,7 +558,7 @@ private static KeyValuePair[] AssertMetricPoint_New( } Assert.Equal( - expected: new List { 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, double.PositiveInfinity }, + expected: new List { 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, double.PositiveInfinity }, actual: histogramBounds); return attributes; diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 63fb2e65b8a..25eb0632922 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -546,7 +546,7 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( } Assert.Equal( - expected: new List { 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, double.PositiveInfinity }, + expected: new List { 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, double.PositiveInfinity }, actual: histogramBounds); } } From 9b70052fcd32e2b743e18fdfa2446b500aafdd84 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> Date: Thu, 2 Nov 2023 20:04:28 -0700 Subject: [PATCH 6/6] Fix CI --- src/OpenTelemetry/CHANGELOG.md | 3 ++- .../MetricTests.cs | 12 +++++++++--- .../HttpClientTests.cs | 12 +++++++++--- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index c5205f289e5..740395d5bed 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -27,8 +27,9 @@ their default histogram buckets as `[ 0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 30, 60, 120, 300 ]`. ([#5008](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5008)) + ([#5021](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5021)) -* Remove the bucket with value `0` for histogram buckets for metrics from +* Remove the bucket with value `0` for histogram buckets for all metrics from ASP.NET Core and HttpClient. ([#5021](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5021)) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs index 39fb8bbfd24..cbe1e012b9e 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs @@ -557,9 +557,15 @@ private static KeyValuePair[] AssertMetricPoint_New( histogramBounds.Add(t.ExplicitBound); } - Assert.Equal( - expected: new List { 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, double.PositiveInfinity }, - actual: histogramBounds); + // TODO: Remove the check for the older bounds once 1.7.0 is released. This is a temporary fix for instrumentation libraries CI workflow. + + var expectedHistogramBoundsOld = new List { 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, double.PositiveInfinity }; + var expectedHistogramBoundsNew = new List { 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, double.PositiveInfinity }; + + var histogramBoundsMatchCorrectly = Enumerable.SequenceEqual(expectedHistogramBoundsOld, histogramBounds) || + Enumerable.SequenceEqual(expectedHistogramBoundsNew, histogramBounds); + + Assert.True(histogramBoundsMatchCorrectly); return attributes; } diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs index 25eb0632922..314ff67ea41 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs @@ -545,9 +545,15 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync( histogramBounds.Add(t.ExplicitBound); } - Assert.Equal( - expected: new List { 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, double.PositiveInfinity }, - actual: histogramBounds); + // TODO: Remove the check for the older bounds once 1.7.0 is released. This is a temporary fix for instrumentation libraries CI workflow. + + var expectedHistogramBoundsOld = new List { 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, double.PositiveInfinity }; + var expectedHistogramBoundsNew = new List { 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, double.PositiveInfinity }; + + var histogramBoundsMatchCorrectly = Enumerable.SequenceEqual(expectedHistogramBoundsOld, histogramBounds) || + Enumerable.SequenceEqual(expectedHistogramBoundsNew, histogramBounds); + + Assert.True(histogramBoundsMatchCorrectly); } } }