Repository navigation
[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
|
Was this metric marked stable in .NET? |
|
I'm not sure I understand the question fully. I've made this change based on your change in open-telemetry/semantic-conventions#3759. Am I missing something? |
The semantic convention in OTel is marked "experimental", so it is free to change including breaking changes. My question was - is this metric marked as experimental in .NET/HTTPClient too? if not, this is breaking users, and may need different treatment. |
|
If you mean if we have it in code marked as "Experimental" then no. This is a bit confusing for me, but how could this differ? Also looking at https://opentelemetry.io/docs/specs/semconv/dotnet/dotnet-http-metrics/#metric-httpclientopen_connections where it says "Stable", how can it be "Experimental" in your case? I can always revert, that's not a problem. But I'm kind of missing the point of making it Opt-In if we wouldn't be able to switch from Recommended to it due to being "Stable" anyway? Or am I missing something else? |
The link you are lookin at is the dotnet specific conventions, not OTel http convention. The .NET is marked stable, and it still have the network.peer.address as recommended (I haven't changed that). What I changed was the OTel semantic conventions for HTTP, and specifically the open_connections one, which is marked "development" stage i.e it is not stable (and hence free to make breaking changes) |
|
I'd assume that the .NET specific conventions are based on OTel semantic ones. And if so, how come the later is experimental while the former is stable? And what effect has the OTel semantic convention change if it doesn't affect the actual implementations? Either way, I will revert this just to be on the safe side. I'm just curious about the ins and outs of this whole specs hierarchy. |
I do not know the full history behind why .NET has own conventions :( @martincostello Does OTel .NET contrib instrumentation enable the .NET specific convention or OTel conventions internally?
Not sure I follow this - if OTel Convention changes, then the implementation would also change in line with the convention change. |
For the code we implement we go with OTel's - where .NET has support for something in-box we delegate to the runtime so we don't have two parallel implementations. |
…29317) Reverts #128940 Apparently, there are 2 different specs, and the one for .NET is stable. See discussion in #128940 (comment)
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