HTTP Metrics: Uncoditionally report server.port (#104741)

Since `server.port` is required, we should not omit it for default ports in `http.client.request.duration`.

For consistency, this PR also makes  `http.client.connection.duration` and `http.client.open_connections` REQUIRED, meaning port will be uncoditionally present in those metrics too.

Resolves #94829.

Author: antonfirsov

src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs

@@ -200,7 +200,7 @@ await _innerHandler.SendAsync(request, cancellationToken).ConfigureAwait(false)
                         // Add standard tags known at request completion.
                         if (response is not null)
                         {
-                            activity.SetTag("http.response.status_code", DiagnosticsHelper.GetBoxedStatusCode((int)response.StatusCode));
+                            activity.SetTag("http.response.status_code", DiagnosticsHelper.GetBoxedInt32((int)response.StatusCode));
                             activity.SetTag("network.protocol.version", DiagnosticsHelper.GetProtocolVersionString(response.Version));
                         }
 
@@ -219,7 +219,7 @@ await _innerHandler.SendAsync(request, cancellationToken).ConfigureAwait(false)
                         // Add standard tags known at request completion.
                         if (response is not null)
                         {
-                            activity.SetTag("http.response.status_code", DiagnosticsHelper.GetBoxedStatusCode((int)response.StatusCode));
+                            activity.SetTag("http.response.status_code", DiagnosticsHelper.GetBoxedInt32((int)response.StatusCode));
                             activity.SetTag("network.protocol.version", DiagnosticsHelper.GetProtocolVersionString(response.Version));
                         }
 

src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHelper.cs

@@ -106,13 +106,15 @@ public static bool TryGetErrorType(HttpResponseMessage? response, Exception? exc
         private static string[]? s_statusCodeStrings;
 
 #pragma warning disable CA1859 // we explictly box here
-        public static object GetBoxedStatusCode(int statusCode)
+        // Returns a pooled object if 'value' is between 0-512,
+        // saving allocations for standard HTTP status codes and small port tag values.
+        public static object GetBoxedInt32(int value)
         {
             object[] boxes = LazyInitializer.EnsureInitialized(ref s_boxedStatusCodes, static () => new object[512]);
 
-            return (uint)statusCode < (uint)boxes.Length
-                ? boxes[statusCode] ??= statusCode
-                : statusCode;
+            return (uint)value < (uint)boxes.Length
+                ? boxes[value] ??= value
+                : value;
         }
 #pragma warning restore
 

src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs

@@ -110,7 +110,7 @@ private void RequestStop(HttpRequestMessage request, HttpResponseMessage? respon
 
             if (response is not null)
             {
-                tags.Add("http.response.status_code", DiagnosticsHelper.GetBoxedStatusCode((int)response.StatusCode));
+                tags.Add("http.response.status_code", DiagnosticsHelper.GetBoxedInt32((int)response.StatusCode));
                 tags.Add("network.protocol.version", DiagnosticsHelper.GetProtocolVersionString(response.Version));
             }
 
@@ -140,11 +140,7 @@ private static TagList InitializeCommonTags(HttpRequestMessage request)
             {
                 tags.Add("url.scheme", requestUri.Scheme);
                 tags.Add("server.address", requestUri.Host);
-                // Add port tag when not the default value for the current scheme
-                if (!requestUri.IsDefaultPort)
-                {
-                    tags.Add("server.port", requestUri.Port);
-                }
+                tags.Add("server.port", DiagnosticsHelper.GetBoxedInt32(requestUri.Port));
             }
             tags.Add(DiagnosticsHelper.GetMethodTag(request.Method, out _));
 

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionBase.cs

@@ -72,7 +72,7 @@ protected void MarkConnectionAsEstablished(Activity? connectionSetupActivity, IP
                     protocol,
                     _pool.IsSecure ? "https" : "http",
                     _pool.OriginAuthority.HostValue,
-                    _pool.IsDefaultPort ? null : _pool.OriginAuthority.Port,
+                    _pool.OriginAuthority.Port,
                     remoteEndPoint?.Address?.ToString());
 
                 _connectionMetrics.ConnectionEstablished();

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/ConnectionMetrics.cs

@@ -13,18 +13,18 @@ internal sealed class ConnectionMetrics
         private readonly object _protocolVersionTag;
         private readonly object _schemeTag;
         private readonly object _hostTag;
-        private readonly object? _portTag;
+        private readonly object _portTag;
         private readonly object? _peerAddressTag;
         private bool _currentlyIdle;
 
-        public ConnectionMetrics(SocketsHttpHandlerMetrics metrics, string protocolVersion, string scheme, string host, int? port, string? peerAddress)
+        public ConnectionMetrics(SocketsHttpHandlerMetrics metrics, string protocolVersion, string scheme, string host, int port, string? peerAddress)
         {
             _metrics = metrics;
             _openConnectionsEnabled = _metrics.OpenConnections.Enabled;
             _protocolVersionTag = protocolVersion;
             _schemeTag = scheme;
             _hostTag = host;
-            _portTag = port;
+            _portTag = DiagnosticsHelper.GetBoxedInt32(port);
             _peerAddressTag = peerAddress;
         }
 
@@ -36,11 +36,7 @@ private TagList GetTags()
             tags.Add("network.protocol.version", _protocolVersionTag);
             tags.Add("url.scheme", _schemeTag);
             tags.Add("server.address", _hostTag);
-
-            if (_portTag is not null)
-            {
-                tags.Add("server.port", _portTag);
-            }
+            tags.Add("server.port", _portTag);
 
             if (_peerAddressTag is not null)
             {

src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs

@@ -14,6 +14,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.DotNet.RemoteExecutor;
+using Microsoft.DotNet.XUnitExtensions;
 using Xunit;
 using Xunit.Abstractions;
 
@@ -71,13 +72,12 @@ protected HttpMetricsTestBase(ITestOutputHelper output) : base(output)
         }
 
 
-        private static void VerifyPeerAddress(KeyValuePair<string, object?>[] tags)
+        private static void VerifyPeerAddress(KeyValuePair<string, object?>[] tags, IPAddress[] validPeerAddresses = null)
         {
             string ipString = (string)tags.Single(t => t.Key == "network.peer.address").Value;
+            validPeerAddresses ??= [IPAddress.Loopback.MapToIPv6(), IPAddress.Loopback, IPAddress.IPv6Loopback];
             IPAddress ip = IPAddress.Parse(ipString);
-            Assert.True(ip.Equals(IPAddress.Loopback.MapToIPv6()) ||
-                    ip.Equals(IPAddress.Loopback) ||
-                    ip.Equals(IPAddress.IPv6Loopback));
+            Assert.Contains(ip, validPeerAddresses);
         }
 
 
@@ -126,24 +126,29 @@ protected static void VerifyActiveRequests(string instrumentName, long measureme
             Assert.Equal(method, tags.Single(t => t.Key == "http.request.method").Value);
         }
 
-        protected static void VerifyOpenConnections(string actualName, object measurement, KeyValuePair<string, object?>[] tags, long expectedValue, Uri uri, Version? protocolVersion, string state)
+        protected static void VerifyOpenConnections(string actualName, object measurement, KeyValuePair<string, object?>[] tags, long expectedValue, Uri uri, Version? protocolVersion, string state, IPAddress[] validPeerAddresses = null)
         {
             Assert.Equal(InstrumentNames.OpenConnections, actualName);
             Assert.Equal(expectedValue, Assert.IsType<long>(measurement));
             VerifySchemeHostPortTags(tags, uri);
             VerifyTag(tags, "network.protocol.version", GetVersionString(protocolVersion));
             VerifyTag(tags, "http.connection.state", state);
-            VerifyPeerAddress(tags);
+            VerifyPeerAddress(tags, validPeerAddresses);
         }
 
-        protected static void VerifyConnectionDuration(string instrumentName, object measurement, KeyValuePair<string, object?>[] tags, Uri uri, Version? protocolVersion)
+        protected static void VerifyConnectionDuration(string instrumentName, object measurement, KeyValuePair<string, object?>[] tags, Uri uri, Version? protocolVersion, IPAddress[] validPeerAddresses = null)
         {
             Assert.Equal(InstrumentNames.ConnectionDuration, instrumentName);
             double value = Assert.IsType<double>(measurement);
-            Assert.InRange(value, double.Epsilon, 60);
+
+            // This flakes for remote requests on CI.
+            if (validPeerAddresses is null)
+            {
+                Assert.InRange(value, double.Epsilon, 60);
+            }
             VerifySchemeHostPortTags(tags, uri);
             VerifyTag(tags, "network.protocol.version", GetVersionString(protocolVersion));
-            VerifyPeerAddress(tags);
+            VerifyPeerAddress(tags, validPeerAddresses);
         }
 
         protected static void VerifyTimeInQueue(string instrumentName, object measurement, KeyValuePair<string, object?>[] tags, Uri uri, Version? protocolVersion, string method = "GET")
@@ -362,6 +367,41 @@ public Task RequestDuration_Success_Recorded(string method, HttpStatusCode statu
             });
         }
 
+        [OuterLoop("Uses external server.")]
+        [ConditionalFact]
+        public async Task ExternalServer_DurationMetrics_Recorded()
+        {
+            if (UseVersion == HttpVersion.Version30)
+            {
+                throw new SkipTestException("No remote HTTP/3 server available for testing.");
+            }
+
+            using InstrumentRecorder<double> requestDurationRecorder = SetupInstrumentRecorder<double>(InstrumentNames.RequestDuration);
+            using InstrumentRecorder<double> connectionDurationRecorder = SetupInstrumentRecorder<double>(InstrumentNames.ConnectionDuration);
+            using InstrumentRecorder<long> openConnectionsRecorder = SetupInstrumentRecorder<long>(InstrumentNames.OpenConnections);
+
+            Uri uri = UseVersion == HttpVersion.Version11
+                ? Test.Common.Configuration.Http.RemoteHttp11Server.EchoUri
+                : Test.Common.Configuration.Http.RemoteHttp2Server.EchoUri;
+            IPAddress[] addresses = await Dns.GetHostAddressesAsync(uri.Host);
+            addresses = addresses.Union(addresses.Select(a => a.MapToIPv6())).ToArray();
+
+            using (HttpMessageInvoker client = CreateHttpMessageInvoker())
+            {
+                using HttpRequestMessage request = new(HttpMethod.Get, uri) { Version = UseVersion };
+                request.Headers.ConnectionClose = true;
+                using HttpResponseMessage response = await SendAsync(client, request);
+                await response.Content.LoadIntoBufferAsync();
+                await WaitForEnvironmentTicksToAdvance();
+            }
+
+            VerifyRequestDuration(Assert.Single(requestDurationRecorder.GetMeasurements()), uri, UseVersion, 200, "GET");
+            Measurement<double> cd = Assert.Single(connectionDurationRecorder.GetMeasurements());
+            VerifyConnectionDuration(InstrumentNames.ConnectionDuration, cd.Value, cd.Tags.ToArray(), uri, UseVersion, addresses);
+            Measurement<long> oc = openConnectionsRecorder.GetMeasurements().First();
+            VerifyOpenConnections(InstrumentNames.OpenConnections, oc.Value, oc.Tags.ToArray(), 1, uri, UseVersion, "idle", addresses);
+        }
+
         [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
         public async Task RequestDuration_HttpTracingEnabled_RecordedWhileRequestActivityRunning()
         {