[HTTP] Remove peer.address from connection OTel counters#128940
Conversation
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
There was a problem hiding this comment.
Pull request overview
Removes the network.peer.address tag from System.Net.Http connection metrics tag sets, aligning emitted dimensions with updated OpenTelemetry semantic conventions (where that attribute is now opt-in for http.client.open_connections) and reducing metric cardinality pressure.
Changes:
- Stop emitting
network.peer.addressfromSocketsHttpHandlerconnection metric tags. - Remove peer-address validation helper and usages from
System.Net.Httpfunctional metrics tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Metrics/ConnectionMetrics.cs | Removes network.peer.address from the TagList used by connection metrics. |
| src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs | Updates metrics functional test helpers to no longer validate network.peer.address. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs:137
validPeerAddressesis no longer used to validate peer addresses (the peer-address tag was removed). It now only serves as a non-null sentinel to skip the duration range assertion, so the parameter name is misleading and makes the test harder to understand/maintain.
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);
// This flakes for remote requests on CI.
if (validPeerAddresses is null)
{
|
/azp list |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Maybe this is something worth investigating separately? The OTel HTTP instrumentation delegates to the runtime implementation since .NET 8, so if people wanted these we'd have to re-introduce functionality there to bring these attributes back for users who wanted them. |
|
Filed #128985, likely will happen earliest in .NET 12 (i.e. won't be in 11). |
Fix for httpbin returning 503, which is out of our control. Based on Kusto, started 28.5., seen in #128940
OTel changed specification for these 2 tags to "Opt-In". We do not have any toggles for Opt-In tags and we simply omit them. Therefore, I'm removing them from the code.
OTel change: open-telemetry/semantic-conventions#3759
Contributes to #122752