Fix network.protocol.version

open-telemetry · Mar 11, 2024 · a794951 · a794951
1 parent 445a684
commit a794951
Show file tree

Hide file tree

Showing 6 changed files with 51 additions and 28 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs
@@ -14,7 +14,7 @@ internal sealed class HttpInListener : IDisposable
 {
     private readonly HttpRequestRouteHelper routeHelper = new();
     private readonly AspNetTraceInstrumentationOptions options;
-    private readonly RequestMethodHelper requestMethodHelper = new();
+    private readonly RequestDataHelper requestDataHelper = new();
 
     public HttpInListener(AspNetTraceInstrumentationOptions options)
     {
@@ -87,14 +87,20 @@ private void OnStartActivity(Activity activity, HttpContext context)
             activity.SetTag(SemanticConventions.AttributeUrlScheme, url.Scheme);
 
             var originalHttpMethod = request.HttpMethod;
-            var normalizedHttpMethod = this.requestMethodHelper.GetNormalizedHttpMethod(originalHttpMethod);
+            var normalizedHttpMethod = this.requestDataHelper.GetNormalizedHttpMethod(originalHttpMethod);
             activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, normalizedHttpMethod);
 
             if (originalHttpMethod != normalizedHttpMethod)
             {
                 activity.SetTag(SemanticConventions.AttributeHttpRequestMethodOriginal, originalHttpMethod);
             }
 
+            var protocolVersion = RequestDataHelper.GetHttpProtocolVersion(request);
+            if (!string.IsNullOrEmpty(protocolVersion))
+            {
+                activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, protocolVersion);
+            }
+
             activity.SetTag(SemanticConventions.AttributeUrlPath, path);
             activity.SetTag(SemanticConventions.AttributeUserAgentOriginal, request.UserAgent);
             activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUriTagValueFromRequestUri(request.Url));

diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInMetricsListener.cs
@@ -12,7 +12,7 @@ namespace OpenTelemetry.Instrumentation.AspNet.Implementation;
 internal sealed class HttpInMetricsListener : IDisposable
 {
     private readonly HttpRequestRouteHelper routeHelper = new();
-    private readonly RequestMethodHelper requestMethodHelper = new();
+    private readonly RequestDataHelper requestDataHelper = new();
     private readonly Histogram<double> httpServerDuration;
     private readonly AspNetMetricsInstrumentationOptions options;
 
@@ -31,18 +31,6 @@ public void Dispose()
         TelemetryHttpModule.Options.OnRequestStoppedCallback -= this.OnStopActivity;
     }
 
-    private static string GetHttpProtocolVersion(HttpRequest request)
-    {
-        var protocol = request.ServerVariables["SERVER_PROTOCOL"];
-        return protocol switch
-        {
-            "HTTP/1.1" => "1.1",
-            "HTTP/2" => "2",
-            "HTTP/3" => "3",
-            _ => protocol,
-        };
-    }
-
     private void OnStopActivity(Activity activity, HttpContext context)
     {
         var request = context.Request;
@@ -55,10 +43,10 @@ private void OnStopActivity(Activity activity, HttpContext context)
             { SemanticConventions.AttributeHttpResponseStatusCode, context.Response.StatusCode },
         };
 
-        var normalizedMethod = this.requestMethodHelper.GetNormalizedHttpMethod(request.HttpMethod);
+        var normalizedMethod = this.requestDataHelper.GetNormalizedHttpMethod(request.HttpMethod);
         tags.Add(SemanticConventions.AttributeHttpRequestMethod, normalizedMethod);
 
-        var protocolVersion = GetHttpProtocolVersion(request);
+        var protocolVersion = RequestDataHelper.GetHttpProtocolVersion(request);
         if (!string.IsNullOrEmpty(protocolVersion))
         {
             tags.Add(SemanticConventions.AttributeNetworkProtocolVersion, protocolVersion);

diff --git a/...Net/Implementation/RequestMethodHelper.cs → ...spNet/Implementation/RequestDataHelper.cs b/...Net/Implementation/RequestMethodHelper.cs → ...spNet/Implementation/RequestDataHelper.cs
@@ -4,10 +4,11 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Web;
 
 namespace OpenTelemetry.Instrumentation.AspNet.Implementation;
 
-internal sealed class RequestMethodHelper
+internal sealed class RequestDataHelper
 {
     private const string KnownHttpMethodsEnvironmentVariable = "OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS";
 
@@ -20,7 +21,7 @@ internal sealed class RequestMethodHelper
     // List of known HTTP methods as per spec.
     private readonly Dictionary<string, string> knownHttpMethods;
 
-    public RequestMethodHelper()
+    public RequestDataHelper()
     {
         var suppliedKnownMethods = Environment.GetEnvironmentVariable(KnownHttpMethodsEnvironmentVariable)
             ?.Split(SplitChars, StringSplitOptions.RemoveEmptyEntries);
@@ -40,10 +41,26 @@ public RequestMethodHelper()
             };
     }
 
+    public static string GetHttpProtocolVersion(HttpRequest request)
+    {
+        return GetHttpProtocolVersion(request.ServerVariables["SERVER_PROTOCOL"]);
+    }
+
     public string GetNormalizedHttpMethod(string method)
     {
         return this.knownHttpMethods.TryGetValue(method, out var normalizedMethod)
             ? normalizedMethod
             : OtherHttpMethod;
     }
+
+    internal static string GetHttpProtocolVersion(string protocol)
+    {
+        return protocol switch
+        {
+            "HTTP/1.1" => "1.1",
+            "HTTP/2" => "2",
+            "HTTP/3" => "3",
+            _ => protocol,
+        };
+    }
 }
diff --git a/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs b/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs
@@ -142,6 +142,7 @@ public void AspNetRequestsAreCollectedSuccessfully(
 
         Assert.Equal(expectedRequestMethod, span.GetTagValue("http.request.method"));
         Assert.Equal(expectedOriginalRequestMethod, span.GetTagValue("http.request.method_original"));
+        Assert.Equal("FakeHTTP/123", span.GetTagValue("network.protocol.version"));
 
         Assert.Equal(expectedUrlPath, span.GetTagValue("url.path"));
         Assert.Equal(expectedUrlScheme, span.GetTagValue("url.scheme"));

diff --git a/....AspNet.Tests/RequestMethodHelperTests.cs → ...on.AspNet.Tests/RequestDataHelperTests.cs b/....AspNet.Tests/RequestMethodHelperTests.cs → ...on.AspNet.Tests/RequestDataHelperTests.cs
@@ -7,7 +7,7 @@
 
 namespace OpenTelemetry.Instrumentation.AspNet.Tests;
 
-public class RequestMethodHelperTests : IDisposable
+public class RequestDataHelperTests : IDisposable
 {
     [Theory]
     [InlineData("GET", "GET")]
@@ -23,7 +23,7 @@ public class RequestMethodHelperTests : IDisposable
     [InlineData("invalid", "_OTHER")]
     public void MethodMappingWorksForKnownMethods(string method, string expected)
     {
-        var requestHelper = new RequestMethodHelper();
+        var requestHelper = new RequestDataHelper();
         var actual = requestHelper.GetNormalizedHttpMethod(method);
         Assert.Equal(expected, actual);
     }
@@ -41,11 +41,22 @@ public void MethodMappingWorksForKnownMethods(string method, string expected)
     [InlineData("get", "GET")]
     [InlineData("post", "POST")]
     [InlineData("invalid", "_OTHER")]
-    public void MethodMappingWorksForEnvironmentVariables(string method, string expected)
+    public void MethodMappingWorksForEnvironmentVariables(string protocolVersion, string expected)
     {
         Environment.SetEnvironmentVariable("OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS", "GET,POST");
-        var requestHelper = new RequestMethodHelper();
-        var actual = requestHelper.GetNormalizedHttpMethod(method);
+        var requestHelper = new RequestDataHelper();
+        var actual = requestHelper.GetNormalizedHttpMethod(protocolVersion);
+        Assert.Equal(expected, actual);
+    }
+
+    [Theory]
+    [InlineData("HTTP/1.1", "1.1")]
+    [InlineData("HTTP/2", "2")]
+    [InlineData("HTTP/3", "3")]
+    [InlineData("Unknown", "Unknown")]
+    public void MappingProtocolToVersion(string protocolVersion, string expected)
+    {
+        var actual = RequestDataHelper.GetHttpProtocolVersion(protocolVersion);
         Assert.Equal(expected, actual);
     }
 

diff --git a/test/OpenTelemetry.Instrumentation.AspNet.Tests/TestHttpWorkerRequest.cs b/test/OpenTelemetry.Instrumentation.AspNet.Tests/TestHttpWorkerRequest.cs
@@ -35,17 +35,17 @@ public override string GetHttpVerbName()
 
     public override string GetHttpVersion()
     {
-        throw new NotImplementedException();
+        return "FakeHTTP/123";
     }
 
     public override string GetLocalAddress()
     {
-        throw new NotImplementedException();
+        return "fake-local-address"; // avoid throwing exception
     }
 
     public override int GetLocalPort()
     {
-        throw new NotImplementedException();
+        return 1234; // avoid throwing exception
     }
 
     public override string GetQueryString()
@@ -60,7 +60,7 @@ public override string GetRawUrl()
 
     public override string GetRemoteAddress()
     {
-        throw new NotImplementedException();
+        return "fake-remote-address"; // avoid throwing exception
     }
 
     public override int GetRemotePort()