From dfdbf01e2f384ae0fe84f147c183c1ce95dc7c85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kie=C5=82kowicz?= Date: Wed, 27 Mar 2024 21:02:11 +0100 Subject: [PATCH] [Instrumentation.AspNetCore, Instrumentation.Http] Fix http.request.method_original attribute (#5471) --- .../CHANGELOG.md | 6 +++ .../CHANGELOG.md | 9 ++++ src/Shared/RequestMethodHelper.cs | 14 +++--- .../BasicTests.cs | 38 ++++++--------- .../HttpClientTests.Basic.cs | 48 ++++++++++++------- 5 files changed, 66 insertions(+), 49 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 5b608371d46..85264fe737b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -7,6 +7,12 @@ `443` for `HTTPS` protocol). ([#5419](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5419)) +* 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 Released 2024-Feb-09 diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index 9b7fa69f2f8..b23ce1e0096 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -7,6 +7,15 @@ `443` for `HTTPS` protocol). ([#5419](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5419)) +* 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 Released 2024-Feb-09 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..e6c0191b435 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 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")] + [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) {