Revert "[HTTP] Remove peer.address from connection OTel counters"

[ManickaP]

Reverts #128940

Apparently, there are 2 different specs, and the one for .NET is stable. See discussion in #128940 (comment)

[dotnet-policy-service]

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[github-actions]

🤖 Copilot Code Review — PR #129317

Note

This review was AI-generated by GitHub Copilot.

Holistic Assessment

Motivation: Well-justified revert. PR #128940 removed network.peer.address based on the general OTel HTTP semantic conventions (still in "development" stage), but the .NET-specific OTel conventions — which .NET follows — are marked "Stable" and still list network.peer.address as "Recommended". The revert is the correct action.

Approach: Clean git revert of the original commit. All removed code and tests are restored faithfully.

Summary: ✅ LGTM. This is a straightforward, clean revert that restores network.peer.address to HTTP connection OTel counters. The code is correct, the motivation is clearly documented in the PR description and linked discussion, and tests are properly restored.

---

Detailed Findings

✅ Correctness — Revert is complete and faithful

The revert touches exactly the three files modified in #128940:

• ConnectionMetrics.cs: Restores _peerAddressTag field, constructor parameter, and tag emission in GetTags().
• HttpConnectionBase.cs: Passes remoteEndPoint?.Address?.ToString() to the constructor.
• MetricsTest.cs: Restores VerifyPeerAddress helper and integrates it into VerifyOpenConnections and VerifyConnectionDuration.

The single callsite (HttpConnectionBase.cs:73) is updated. The class is internal sealed, so no public API surface is affected.

✅ Test quality — Restored tests are sound

• VerifyPeerAddress correctly validates the tag against loopback addresses (IPv4, IPv6, mapped).
• The external server test (ExternalServer_DurationMetrics_Recorded) pre-resolves DNS addresses and includes both original and IPv6-mapped forms to handle platform differences.
• The Assert.InRange duration check is conditionally skipped for remote requests with the comment "This flakes for remote requests on CI" — a reasonable robustness measure that was part of the original code.

💡 Minor observation — Nullable annotation style (non-blocking)

VerifyPeerAddress declares IPAddress[] validPeerAddresses = null without the nullable annotation (IPAddress[]?). This is consistent with the existing pattern in the same file (e.g., string[] acceptedErrorTypes = null on line 90), so it's fine as-is. Not a blocker.

Generated by Code Review for issue #129317 · ● 4.1M · ◷

[Copilot]

Pull request overview

This PR reverts a prior change that removed the network.peer.address tag from System.Net.Http connection-level OpenTelemetry metrics by restoring the tag emission in SocketsHttpHandler and re-updating functional tests to expect/validate it.

Changes:

• Restore emission of network.peer.address on connection metrics (http.client.open_connections, http.client.connection.duration) when a remote endpoint address is available.
• Update functional metrics tests to validate network.peer.address, including external-server coverage by allowing any resolved IP for the host.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs	Adds peer-address validation helpers and updates external-server test to accept any resolved peer IP.
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/ConnectionMetrics.cs	Reintroduces the network.peer.address tag (conditionally) in the tag set used for connection metrics.
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionBase.cs	Plumbs the remote endpoint address string into ConnectionMetrics at connection establishment.