[HttpClient] Remove http.host and add net.peer.name & net.peer.port to match spec (#3832)

vishweshbankwar · web-flow · commit 2fce583b9c04 · 2022-11-11T12:49:30.000-05:00
diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* **Breaking change** `http.host` will no longer be populated. `net.peer.name`
+  and `net.peer.port` attributes will be populated instead.
+  ([#3832](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3832))
+
 ## 1.0.0-rc9.9
 
 Released 2022-Nov-07
diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
@@ -159,7 +159,12 @@ public void OnStartActivity(Activity activity, object payload)
 
                 activity.SetTag(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme);
                 activity.SetTag(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method));
-                activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri));
+                activity.SetTag(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host);
+                if (!request.RequestUri.IsDefaultPort)
+                {
+                    activity.SetTag(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port);
+                }
+
                 activity.SetTag(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));
                 activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version));
 
diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpTagHelper.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpTagHelper.cs
@@ -27,7 +27,6 @@ internal static class HttpTagHelper
         private static readonly ConcurrentDictionary<string, string> MethodOperationNameCache = new();
         private static readonly ConcurrentDictionary<HttpMethod, string> HttpMethodOperationNameCache = new();
         private static readonly ConcurrentDictionary<HttpMethod, string> HttpMethodNameCache = new();
-        private static readonly ConcurrentDictionary<string, ConcurrentDictionary<int, string>> HostAndPortToStringCache = new();
         private static readonly ConcurrentDictionary<Version, string> ProtocolVersionToStringCache = new();
 
         private static readonly Func<string, string> ConvertMethodToOperationNameRef = ConvertMethodToOperationName;
@@ -63,37 +62,6 @@ internal static class HttpTagHelper
         /// <returns>Span flavor value.</returns>
         public static string GetFlavorTagValueFromProtocolVersion(Version protocolVersion) => ProtocolVersionToStringCache.GetOrAdd(protocolVersion, ConvertProtocolVersionToStringRef);
 
-        /// <summary>
-        /// Gets the OpenTelemetry standard host tag value for a span based on its request <see cref="Uri"/>.
-        /// </summary>
-        /// <param name="requestUri"><see cref="Uri"/>.</param>
-        /// <returns>Span host value.</returns>
-        public static string GetHostTagValueFromRequestUri(Uri requestUri)
-        {
-            string host = requestUri.Host;
-
-            if (requestUri.IsDefaultPort)
-            {
-                return host;
-            }
-
-            int port = requestUri.Port;
-
-            if (!HostAndPortToStringCache.TryGetValue(host, out ConcurrentDictionary<int, string> portCache))
-            {
-                portCache = new ConcurrentDictionary<int, string>();
-                HostAndPortToStringCache.TryAdd(host, portCache);
-            }
-
-            if (!portCache.TryGetValue(port, out string hostTagValue))
-            {
-                hostTagValue = $"{requestUri.Host}:{requestUri.Port}";
-                portCache.TryAdd(port, hostTagValue);
-            }
-
-            return hostTagValue;
-        }
-
         /// <summary>
         /// Gets the OpenTelemetry standard uri tag value for a span based on its request <see cref="Uri"/>.
         /// </summary>
diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs
@@ -99,8 +99,13 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A
             if (activity.IsAllDataRequested)
             {
                 activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method);
+                activity.SetTag(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host);
+                if (!request.RequestUri.IsDefaultPort)
+                {
+                    activity.SetTag(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port);
+                }
+
                 activity.SetTag(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme);
-                activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri));
                 activity.SetTag(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));
                 activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion));
 
diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.cs
@@ -231,7 +231,8 @@ public async Task DebugIndividualTestAsync()
                     ""spanAttributes"": {
                       ""http.scheme"": ""http"",
                       ""http.method"": ""GET"",
-                      ""http.host"": ""{host}:{port}"",
+                      ""net.peer.name"": ""{host}"",
+                      ""net.peer.port"": ""{port}"",
                       ""http.status_code"": ""399"",
                       ""http.flavor"": ""{flavor}"",
                       ""http.url"": ""http://{host}:{port}/""
diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs
@@ -39,6 +39,8 @@ public class HttpWebRequestActivitySourceTests : IDisposable
         private readonly string testServerHost;
         private readonly int testServerPort;
         private readonly string hostNameAndPort;
+        private readonly string netPeerName;
+        private readonly int netPeerPort;
 
         static HttpWebRequestActivitySourceTests()
         {
@@ -74,6 +76,8 @@ public HttpWebRequestActivitySourceTests()
                 out this.testServerPort);
 
             this.hostNameAndPort = $"{this.testServerHost}:{this.testServerPort}";
+            this.netPeerName = this.testServerHost;
+            this.netPeerPort = this.testServerPort;
 
             void ProcessServerRequest(HttpListenerContext context)
             {
@@ -198,7 +202,7 @@ public async Task TestBasicReceiveAndResponseEvents(string method, string queryS
             // Check to make sure: The first record must be a request, the next record must be a response.
             Activity activity = AssertFirstEventWasStart(eventRecords);
 
-            VerifyActivityStartTags(this.hostNameAndPort, method, url, activity);
+            VerifyActivityStartTags(this.netPeerName, this.netPeerPort, method, url, activity);
 
             Assert.True(eventRecords.Records.TryDequeue(out var stopEvent));
             Assert.Equal("Stop", stopEvent.Key);
@@ -378,7 +382,7 @@ public async Task TestBasicReceiveAndResponseWebRequestEvents(string method, int
             // Check to make sure: The first record must be a request, the next record must be a response.
             Activity activity = AssertFirstEventWasStart(eventRecords);
 
-            VerifyActivityStartTags(this.hostNameAndPort, method, url, activity);
+            VerifyActivityStartTags(this.netPeerName, this.netPeerPort, method, url, activity);
 
             Assert.True(eventRecords.Records.TryDequeue(out var stopEvent));
             Assert.Equal("Stop", stopEvent.Key);
@@ -484,7 +488,7 @@ public async Task TestResponseWithoutContentEvents(string method)
             // Check to make sure: The first record must be a request, the next record must be a response.
             Activity activity = AssertFirstEventWasStart(eventRecords);
 
-            VerifyActivityStartTags(this.hostNameAndPort, method, url, activity);
+            VerifyActivityStartTags(this.netPeerName, this.netPeerPort, method, url, activity);
 
             Assert.True(eventRecords.Records.TryDequeue(out var stopEvent));
             Assert.Equal("Stop", stopEvent.Key);
@@ -523,9 +527,10 @@ public async Task TestRedirectedRequest(string method)
         [InlineData("POST")]
         public async Task TestRequestWithException(string method)
         {
+            string host = Guid.NewGuid().ToString() + ".com";
             string url = method == "GET"
-                ? $"http://{Guid.NewGuid()}.com"
-                : $"http://{Guid.NewGuid()}.com";
+                ? $"http://{host}"
+                : $"http://{host}";
 
             using var eventRecords = new ActivitySourceRecorder();
 
@@ -548,7 +553,7 @@ public async Task TestRequestWithException(string method)
 
             // Check to make sure: The first record must be a request, the next record must be an exception.
             Activity activity = AssertFirstEventWasStart(eventRecords);
-            VerifyActivityStartTags(null, method, url, activity);
+            VerifyActivityStartTags(host, null, method, url, activity);
 
             Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair<string, Activity> exceptionEvent));
             Assert.Equal("Stop", exceptionEvent.Key);
@@ -587,7 +592,7 @@ public async Task TestCanceledRequest(string method)
             Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop"));
 
             Activity activity = AssertFirstEventWasStart(eventRecords);
-            VerifyActivityStartTags(this.hostNameAndPort, method, url, activity);
+            VerifyActivityStartTags(this.netPeerName, this.netPeerPort, method, url, activity);
 
             Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair<string, Activity> exceptionEvent));
             Assert.Equal("Stop", exceptionEvent.Key);
@@ -626,7 +631,7 @@ public async Task TestSecureTransportFailureRequest(string method)
             Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop"));
 
             Activity activity = AssertFirstEventWasStart(eventRecords);
-            VerifyActivityStartTags(null, method, url, activity);
+            VerifyActivityStartTags("expired.badssl.com", null, method, url, activity);
 
             Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair<string, Activity> exceptionEvent));
             Assert.Equal("Stop", exceptionEvent.Key);
@@ -668,7 +673,7 @@ public async Task TestSecureTransportRetryFailureRequest(string method)
             Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop"));
 
             Activity activity = AssertFirstEventWasStart(eventRecords);
-            VerifyActivityStartTags(this.hostNameAndPort, method, url, activity);
+            VerifyActivityStartTags(this.netPeerName, this.netPeerPort, method, url, activity);
 
             Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair<string, Activity> exceptionEvent));
             Assert.Equal("Stop", exceptionEvent.Key);
@@ -778,15 +783,17 @@ private static void VerifyHeaders(HttpWebRequest startRequest)
             Assert.Matches("^[0-9a-f]{2}-[0-9a-f]{32}-[0-9a-f]{16}-[0-9a-f]{2}$", traceparent);
         }
 
-        private static void VerifyActivityStartTags(string hostNameAndPort, string method, string url, Activity activity)
+        private static void VerifyActivityStartTags(string netPeerName, int? netPeerPort, string method, string url, Activity activity)
         {
             Assert.NotNull(activity.TagObjects);
             Assert.Equal(method, activity.GetTagValue(SemanticConventions.AttributeHttpMethod));
-            if (hostNameAndPort != null)
+            if (netPeerPort != null)
             {
-                Assert.Equal(hostNameAndPort, activity.GetTagValue(SemanticConventions.AttributeHttpHost));
+                Assert.Equal(netPeerPort, activity.GetTagValue(SemanticConventions.AttributeNetPeerPort));
             }
 
+            Assert.Equal(netPeerName, activity.GetTagValue(SemanticConventions.AttributeNetPeerName));
+
             Assert.Equal(url, activity.GetTagValue(SemanticConventions.AttributeHttpUrl));
         }
 
diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.cs
@@ -182,7 +182,8 @@ public void DebugIndividualTest()
                 ""spanAttributes"": {
                   ""http.scheme"": ""http"",
                   ""http.method"": ""GET"",
-                  ""http.host"": ""{host}:{port}"",
+                  ""net.peer.name"": ""{host}"",
+                  ""net.peer.port"": ""{port}"",
                   ""http.flavor"": ""1.1"",
                   ""http.status_code"": ""200"",
                   ""http.url"": ""http://{host}:{port}/""
diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json b/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json
