[Instrumentation.AspNetCore, Instrumentation.Http] Fix http.request.m…

…ethod_original attribute (#5471)
open-telemetry · Mar 27, 2024 · dfdbf01 · dfdbf01
1 parent af57de2
commit dfdbf01
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 49 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<Activity>();
 
@@ -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
@@ -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<Activity>();
         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)
     {