Skip to content

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

Merged
ManickaP merged 1 commit into
mainfrom
revert-128940-otel-http
Jun 16, 2026
Merged

Revert "[HTTP] Remove peer.address from connection OTel counters"#129317
ManickaP merged 1 commit into
mainfrom
revert-128940-otel-http

Conversation

@ManickaP

@ManickaP ManickaP commented Jun 12, 2026

Copy link
Copy Markdown
Member

Reverts #128940

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

@ManickaP
ManickaP requested review from a team and Copilot June 12, 2026 07:19
@dotnet-policy-service

dotnet-policy-service Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

🤖 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 AI reviewed Jun 12, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs
@ManickaP
ManickaP merged commit b802891 into main Jun 16, 2026
97 of 98 checks passed
@ManickaP
ManickaP deleted the revert-128940-otel-http branch June 16, 2026 07:17
@dotnet-maestro dotnet-maestro Bot mentioned this pull request Jun 17, 2026
Merged
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 11.0-preview6 milestone Jun 17, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
@ManickaP @eiriktsarpalis
Revert "[HTTP] Remove peer.address from connection OTel counters" (#1…
d8eca85 
…29317)

Reverts #128940

Apparently, there are 2 different specs, and the one for .NET is stable.
See discussion in
#128940 (comment)
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 18, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@MihaZupan MihaZupan MihaZupan approved these changes

Assignees

@ManickaP ManickaP

Labels

area-System.Net.Http

Projects

None yet

Milestone

11.0-preview6

Development

Successfully merging this pull request may close these issues.

3 participants

@ManickaP @MihaZupan