From 41120bf48e619548d33577a41b7a80eb2141df36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= Date: Mon, 25 Mar 2024 10:01:50 +0100 Subject: [PATCH 1/3] [Instrumentation.AspNetCore, Instrumentation.Http] Fix http.request.method_original attribute --- .../CHANGELOG.md | 3 ++ .../CHANGELOG.md | 5 ++ src/Shared/RequestMethodHelper.cs | 14 +++--- .../BasicTests.cs | 38 ++++++--------- .../HttpClientTests.Basic.cs | 48 ++++++++++++------- 5 files changed, 59 insertions(+), 49 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 5b608371d46..dc8929d3546 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -6,6 +6,9 @@ `server.address` when it has default values (`80` for `HTTP` and `443` for `HTTPS` protocol). ([#5419](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5419)) +* Fixed an issue for spans when `http.request.method_original` attribute was not + set for non canonical form of HTTP methods, e.g. for `Options`. + ([#5471](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5471)) ## 1.7.1 diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 9b7fa69f2f8..dfae6550b8b 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -6,6 +6,11 @@ `server.address` when it has default values (`80` for `HTTP` and `443` for `HTTPS` protocol). ([#5419](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5419)) +* Fixed an issue for spans when `http.request.method_original` attribute was not + set for non canonical form of HTTP methods, e.g. for `Options`. The attribute + is not set .NET Framework for non canonical form of `Connect`, `GET`, `HEAD`, + `PUT`, and `POST`. HTTP Client is converting these values to canonical form. + ([#5471](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5471)) ## 1.7.1 diff --git a/src/Shared/RequestMethodHelper.cs b/src/Shared/RequestMethodHelper.cs index 05a3f022511..667837061a1 100644 --- a/src/Shared/RequestMethodHelper.cs +++ b/src/Shared/RequestMethodHelper.cs @@ -51,16 +51,14 @@ public static string GetNormalizedHttpMethod(string method) : OtherHttpMethod; } - public static void SetHttpMethodTag(Activity activity, string method) + public static void SetHttpMethodTag(Activity activity, string originalHttpMethod) { - if (KnownMethods.TryGetValue(method, out var normalizedMethod)) - { - activity?.SetTag(SemanticConventions.AttributeHttpRequestMethod, normalizedMethod); - } - else + var normalizedHttpMethod = GetNormalizedHttpMethod(originalHttpMethod); + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, normalizedHttpMethod); + + if (originalHttpMethod != normalizedHttpMethod) { - activity?.SetTag(SemanticConventions.AttributeHttpRequestMethod, OtherHttpMethod); - activity?.SetTag(SemanticConventions.AttributeHttpRequestMethodOriginal, method); + activity.SetTag(SemanticConventions.AttributeHttpRequestMethodOriginal, originalHttpMethod); } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index 80033de3711..44145005b72 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -629,18 +629,18 @@ void ConfigureTestServices(IServiceCollection services) } [Theory] - [InlineData("CONNECT", "CONNECT")] - [InlineData("DELETE", "DELETE")] - [InlineData("GET", "GET")] - [InlineData("PUT", "PUT")] - [InlineData("HEAD", "HEAD")] - [InlineData("OPTIONS", "OPTIONS")] - [InlineData("PATCH", "PATCH")] - [InlineData("Get", "GET")] - [InlineData("POST", "POST")] - [InlineData("TRACE", "TRACE")] - [InlineData("CUSTOM", "_OTHER")] - public async Task HttpRequestMethodIsSetAsPerSpec(string originalMethod, string expectedMethod) + [InlineData("CONNECT", "CONNECT", null)] + [InlineData("DELETE", "DELETE", null)] + [InlineData("GET", "GET", null)] + [InlineData("PUT", "PUT", null)] + [InlineData("HEAD", "HEAD", null)] + [InlineData("OPTIONS", "OPTIONS", null)] + [InlineData("PATCH", "PATCH", null)] + [InlineData("Get", "GET", "Get")] + [InlineData("POST", "POST", null)] + [InlineData("TRACE", "TRACE", null)] + [InlineData("CUSTOM", "_OTHER", "CUSTOM")] + public async Task HttpRequestMethodIsSetAsPerSpec(string originalMethod, string expectedMethod, string expectedOriginalMethod) { var exportedItems = new List(); @@ -681,18 +681,8 @@ void ConfigureTestServices(IServiceCollection services) var activity = exportedItems[0]; - Assert.Contains(activity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRequestMethod); - - if (originalMethod.Equals(expectedMethod, StringComparison.OrdinalIgnoreCase)) - { - Assert.DoesNotContain(activity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRequestMethodOriginal); - } - else - { - Assert.Equal(originalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal) as string); - } - - Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod) as string); + Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod)); + Assert.Equal(expectedOriginalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal)); } [Fact] diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index ba758f25103..47a15f25f33 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -356,18 +356,34 @@ public async Task ExportsSpansCreatedForRetries() } [Theory] - [InlineData("CONNECT", "CONNECT")] - [InlineData("DELETE", "DELETE")] - [InlineData("GET", "GET")] - [InlineData("PUT", "PUT")] - [InlineData("HEAD", "HEAD")] - [InlineData("OPTIONS", "OPTIONS")] - [InlineData("PATCH", "PATCH")] - [InlineData("Get", "GET")] - [InlineData("POST", "POST")] - [InlineData("TRACE", "TRACE")] - [InlineData("CUSTOM", "_OTHER")] - public async Task HttpRequestMethodIsSetOnActivityAsPerSpec(string originalMethod, string expectedMethod) + [InlineData("CONNECT", "CONNECT", null)] + [InlineData("DELETE", "DELETE", null)] + [InlineData("GET", "GET", null)] + [InlineData("PUT", "PUT", null)] + [InlineData("HEAD", "HEAD", null)] + [InlineData("OPTIONS", "OPTIONS", null)] + [InlineData("PATCH", "PATCH", null)] + [InlineData("POST", "POST", null)] + [InlineData("TRACE", "TRACE", null)] + [InlineData("Delete", "DELETE", "Delete")] +#if NETFRAMEWORK + [InlineData("Connect", "CONNECT", null)]// HTTP Client converts Connect to its canonical form (Connect). Expected method is null. + [InlineData("Get", "GET", null)] // HTTP Client converts Get to its canonical form (GET). Expected method is null. + [InlineData("Put", "PUT", null)] // HTTP Client converts Put to its canonical form (PUT). Expected method is null. + [InlineData("Head", "HEAD", null)] // HTTP Client converts Head to its canonical form (HEAD). Expected method is null. + [InlineData("Post", "POST", null)] // HTTP Client converts Post to its canonical form (POST). Expected method is null. +#else + [InlineData("Connect", "CONNECT", "Connect")] + [InlineData("Get", "GET", "Get")] + [InlineData("Put", "PUT", "Put")] + [InlineData("Head", "HEAD", "Head")] + [InlineData("Post", "POST", "Post")] +#endif + [InlineData("Options", "OPTIONS", "Options")] + [InlineData("Patch", "PATCH", "Patch")] + [InlineData("Trace", "TRACE", "Trace")] + [InlineData("CUSTOM", "_OTHER", "CUSTOM")] + public async Task HttpRequestMethodIsSetOnActivityAsPerSpec(string originalMethod, string expectedMethod, string expectedOriginalMethod) { var exportedItems = new List(); using var request = new HttpRequestMessage @@ -396,20 +412,17 @@ public async Task HttpRequestMethodIsSetOnActivityAsPerSpec(string originalMetho var activity = exportedItems[0]; - Assert.Contains(activity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRequestMethod); - if (originalMethod.Equals(expectedMethod, StringComparison.OrdinalIgnoreCase)) { Assert.Equal(expectedMethod, activity.DisplayName); - Assert.DoesNotContain(activity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRequestMethodOriginal); } else { Assert.Equal("HTTP", activity.DisplayName); - Assert.Equal(originalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal) as string); } - Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod) as string); + Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod)); + Assert.Equal(expectedOriginalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal)); } [Theory] @@ -423,6 +436,7 @@ public async Task HttpRequestMethodIsSetOnActivityAsPerSpec(string originalMetho [InlineData("Get", "GET")] [InlineData("POST", "POST")] [InlineData("TRACE", "TRACE")] + [InlineData("Trace", "TRACE")] [InlineData("CUSTOM", "_OTHER")] public async Task HttpRequestMethodIsSetonRequestDurationMetricAsPerSpec(string originalMethod, string expectedMethod) { From abcc617da654a060be84eb5528f4e686fcaf197a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= Date: Wed, 27 Mar 2024 07:23:15 +0100 Subject: [PATCH 2/3] Tests Fix comments for .NETT Framework behavior --- .../HttpClientTests.Basic.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index 47a15f25f33..e6c0191b435 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -367,11 +367,11 @@ public async Task ExportsSpansCreatedForRetries() [InlineData("TRACE", "TRACE", null)] [InlineData("Delete", "DELETE", "Delete")] #if NETFRAMEWORK - [InlineData("Connect", "CONNECT", null)]// HTTP Client converts Connect to its canonical form (Connect). Expected method is null. - [InlineData("Get", "GET", null)] // HTTP Client converts Get to its canonical form (GET). Expected method is null. - [InlineData("Put", "PUT", null)] // HTTP Client converts Put to its canonical form (PUT). Expected method is null. - [InlineData("Head", "HEAD", null)] // HTTP Client converts Head to its canonical form (HEAD). Expected method is null. - [InlineData("Post", "POST", null)] // HTTP Client converts Post to its canonical form (POST). Expected method is null. + [InlineData("Connect", "CONNECT", null)]// HTTP Client converts Connect to its canonical form (Connect). Expected original method is null. + [InlineData("Get", "GET", null)] // HTTP Client converts Get to its canonical form (GET). Expected original method is null. + [InlineData("Put", "PUT", null)] // HTTP Client converts Put to its canonical form (PUT). Expected original method is null. + [InlineData("Head", "HEAD", null)] // HTTP Client converts Head to its canonical form (HEAD). Expected original method is null. + [InlineData("Post", "POST", null)] // HTTP Client converts Post to its canonical form (POST). Expected original method is null. #else [InlineData("Connect", "CONNECT", "Connect")] [InlineData("Get", "GET", "Get")] From bb346b197b88f0fc2d83a09eb126f075b3f74361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= Date: Wed, 27 Mar 2024 07:31:14 +0100 Subject: [PATCH 3/3] PR feedback - update changelog --- .../CHANGELOG.md | 7 +++++-- src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md | 12 ++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index dc8929d3546..85264fe737b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -6,8 +6,11 @@ `server.address` when it has default values (`80` for `HTTP` and `443` for `HTTPS` protocol). ([#5419](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5419)) -* Fixed an issue for spans when `http.request.method_original` attribute was not - set for non canonical form of HTTP methods, e.g. for `Options`. + +* Fixed an issue where the `http.request.method_original` attribute was not set + on activity. Now, when `http.request.method` is set and the original method + is converted to its canonical form (e.g., `Get` is converted to `GET`), + the original value `Get` will be stored in `http.request.method_original`. ([#5471](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5471)) ## 1.7.1 diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index dfae6550b8b..b23ce1e0096 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -6,10 +6,14 @@ `server.address` when it has default values (`80` for `HTTP` and `443` for `HTTPS` protocol). ([#5419](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5419)) -* Fixed an issue for spans when `http.request.method_original` attribute was not - set for non canonical form of HTTP methods, e.g. for `Options`. The attribute - is not set .NET Framework for non canonical form of `Connect`, `GET`, `HEAD`, - `PUT`, and `POST`. HTTP Client is converting these values to canonical form. + +* Fixed an issue where the `http.request.method_original` attribute was not set + on activity. Now, when `http.request.method` is set and the original method + is converted to its canonical form (e.g., `Get` is converted to `GET`), + the original value `Get` will be stored in `http.request.method_original`. + The attribute is not set on .NET Framework for non canonical form of `CONNECT`, + `GET`, `HEAD`, `PUT`, and `POST`. HTTP Client is converting these values + to canonical form. ([#5471](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5471)) ## 1.7.1