Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking change]: HttpClient Metrics reporting server.port unconditionally #42810

Closed
1 of 3 tasks
antonfirsov opened this issue Oct 2, 2024 · 0 comments · Fixed by #43351
Closed
1 of 3 tasks

[Breaking change]: HttpClient Metrics reporting server.port unconditionally #42810

antonfirsov opened this issue Oct 2, 2024 · 0 comments · Fixed by #43351
Assignees
Labels
breaking-change Indicates a .NET Core breaking change doc-idea Indicates issues that are suggestions for new topics [org][type][category] in-pr This issue will be closed (fixed) by an active pull request. Pri1 High priority, do before Pri2 and Pri3 📌 seQUESTered Identifies that an issue has been imported into Quest.

Comments

@antonfirsov
Copy link
Member

antonfirsov commented Oct 2, 2024

Description

When implementing HttpClient Metrics in .NET 8, server.port was introduced as a Conditionally Required attribute in accordance with the state of the standard at that time, meaning that the port was only reported if it did not match the default port of the corresponding protocol (80 for HTTP, 443 for HTTPS). However, the standard requirement level of the attribute has been changed later to Required.

To maintain compliance with the Open Telemetry standard while keeping the instrument's behaviors consistent with each other, the instruments http.client.request.duration, http.client.connection.duration and http.client.open_connections have been changed to unconditionally report the server.port attribute.

This may break existing queries in monitoring software like Prometheus.

Version

.NET 9 Preview 7

Previous behavior

http.client.request.duration, http.client.connection.duration and http.client.open_connections reported the server.port attribute only if it did not match the corresponding protocol's default port (80 for HTTP, 443 for HTTPS).

New behavior

dotnet/runtime#104741 has changed the previos behavior so the server.port attribute is unconditionally reported by the instruments http.client.request.duration, http.client.connection.duration and http.client.open_connections.

Type of breaking change

  • Binary incompatible: Existing binaries might encounter a breaking change in behavior, such as failure to load or execute, and if so, require recompilation.
  • Source incompatible: When recompiled using the new SDK or component or to target the new runtime, existing source code might require source changes to compile successfully.
  • Behavioral change: Existing binaries might behave differently at run time.

Reason for change

Maintaining compliance with the corresponding Open Telemetry specification, while keeping HttpClient instruments consistent with each other.

Recommended action

No action is needed for users who do not rely on HttpClient Metrics.

For users of the http.client.request.duration, http.client.connection.duration and http.client.open_connections instruments, the change may break existing queries in monitoring software like Prometheus.

Feature area

Core .NET libraries, Networking

Affected APIs

System.Net.Http.SocketsHttpHandler.Send(...)
System.Net.Http.SocketsHttpHandler.SendAsync(...)
System.Net.Http.HttpClientHandler.Send(...)
System.Net.Http.HttpClientHandler.SendAsync(...)


Associated WorkItem - 340215

@antonfirsov antonfirsov added doc-idea Indicates issues that are suggestions for new topics [org][type][category] breaking-change Indicates a .NET Core breaking change Pri1 High priority, do before Pri2 and Pri3 labels Oct 2, 2024
@dotnetrepoman dotnetrepoman bot added ⌚ Not Triaged Not triaged labels Oct 2, 2024
@gewarren gewarren added 🗺️ reQUEST Triggers an issue to be imported into Quest. and removed ⌚ Not Triaged Not triaged labels Nov 4, 2024
@sequestor sequestor bot added 📌 seQUESTered Identifies that an issue has been imported into Quest. and removed 🗺️ reQUEST Triggers an issue to be imported into Quest. labels Nov 5, 2024
@dotnetrepoman dotnetrepoman bot added ⌚ Not Triaged Not triaged and removed ⌚ Not Triaged Not triaged labels Nov 5, 2024
@gewarren gewarren moved this from 🔖 Ready to 🏗 In progress in dotnet/docs November 2024 sprint Nov 5, 2024
@gewarren gewarren moved this from 🏗 In progress to 👀 In review in dotnet/docs November 2024 sprint Nov 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr This issue will be closed (fixed) by an active pull request. label Nov 5, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in dotnet/docs November 2024 sprint Nov 6, 2024
antonfirsov added a commit that referenced this issue Dec 10, 2024
.NET 9 introduced additions to `System.Net` distributed tracing which define a contract following OTel recommendations:
- dotnet/runtime#104251 adjusted the HTTP client request activity so it natively complies with OTel recommendations. This included adding standard tags.
- dotnet/runtime#103922 introduced activities breaking down the HTTP connection setup.

This PR documents the activities together with their attributes in a manner similar to the metrics docs added by #37213. The matching semconv PR is open-telemetry/semantic-conventions#1192. Conceptual docs are being introduced in #42830.

Moreover, the PR also adjusts certain parts of the metrics doc in order to make it more accurate, synchronize it with the activities doc and incorporate the changes from #42810.
---------

Co-authored-by: Genevieve Warren <[email protected]>
Co-authored-by: Miha Zupan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates a .NET Core breaking change doc-idea Indicates issues that are suggestions for new topics [org][type][category] in-pr This issue will be closed (fixed) by an active pull request. Pri1 High priority, do before Pri2 and Pri3 📌 seQUESTered Identifies that an issue has been imported into Quest.
Projects
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants