Discrepancy between OTel semantic conventions and built-in System.Net metrics

[xakep139]

Current implementation of

runtime/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs

Line 12 in 55fbb23

internal sealed class MetricsHandler : HttpMessageHandlerStage

has a discrepancy with OpenTelemetry semantic conventions: server.port is required, but it's set only when a port isn't a default one:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs

Line 195 in 55fbb23

if (!requestUri.IsDefaultPort)

One more thing not related to OTel: the documentation says that network.protocol.version is always present, but it's added only when a response is available. I created a PR to fix the documentation (dotnet/docs#39223), but I'm wondering wether it should be fixed on the implementation side, and the docs are correct.

[ghost]

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

Issue Details

---

Current implementation of

runtime/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs

Line 12 in 55fbb23

internal sealed class MetricsHandler : HttpMessageHandlerStage

has a discrepancy with OpenTelemetry semantic conventions: server.port is required, but it's set only when a port isn't a default one:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs

Line 195 in 55fbb23

if (!requestUri.IsDefaultPort)

One more thing not related to OTel: the documentation says that network.protocol.version is always present, but it's added only when a response is available. I created a PR to fix the documentation (dotnet/docs#39223), but I'm wondering wether it should be fixed on the implementation side, and the docs are correct.

Author:	xakep139
Assignees:	-
Labels:	area-System.Net.Http, untriaged
Milestone:	-

[xakep139]

cc @antonfirsov @CarnaViire

[antonfirsov]

server.port is required, but it's set only when a port isn't a default one

There is a note in the dotnet-specific part of spec:

server.port is not reported when it matches a default one for provided scheme (443 for https and 80 for http)

@lmolkova do you happen to remember why did we specify & implement it like this originally? How did we justify that in this case Required doesn't mean always present in open-telemetry/semantic-conventions#283?

We can certainly think about making server.port always present in 9.0, but we should do it across all the metrics, not just duration and it would be a breaking change. @xakep139 are you facing any practical issues because of the way this works today?

network.protocol.version [...] but I'm wondering wether it should be fixed on the implementation side

It's not possible to include network.protocol.version when there is no response, because of implementation constraints, at least not with the current design that doesn't bake request metrics into SocketsHttpHandler. An HttpResponseMessage is needed to fetch the actual negotiated HTTP version. When an exception is being thrown by the underlying handler, there is no response object to work with.

[xakep139]

I'm trying to understand whether it's a gap in OTel SemConv, or it was a miss on the .NET implementation side.
Now I understand the limitation, and wondering how other SDKs (i.e. programming languages) might follow the semantic conventions if a request might fail even without being sent (e.g. a DNS resolution has failed). In that case the negotiated HTTP version won't be known, right?
@cijothomas @reyang FYI

[antonfirsov]

Triage: we should decide if we want to change the server.port behavior.

[lmolkova]

@lmolkova do you happen to remember why did we specify & implement it like this originally? How did we justify that in this case Required doesn't mean always present in open-telemetry/semantic-conventions#283?

the port was changed after .NET metrics were completely frozen (there were some corner cases where we didn't set url.scheme and server.port making things ambiguous and maybe some other issues) - here's an existing issue on it #94829

[lmolkova]

@antonfirsov, regarding your comment here:

dotnet/docs#39223 (comment)

IMO this would be misleading. Imagine the following scenario:

A user sets request.Version = 2.0
It falls back to 1.1 during negotiation, and an HTTP/1.1 connection starts serving the request
Something goes wrong in the middle (eg. invalid response) and an exception is thrown
Telemetry would log network.protocol.version=2.0 which was not the actual protocol version used for the failing communication attempt

I agree it's misleading, but not having it is also misleading:

• A user sets (incorrectly) request.Version = foo/42
• Server only supports bar/123 and dies in attempt to serve this bogus request and never responds
• User tries to understand why a certain subset of requests fail and does not see foo/42 anywhere

I think we might have a small problem in otel semconv where we have one attribute for something ambiguous. Let me create a spec issue on it - update open-telemetry/semantic-conventions#686

[antonfirsov]

Server only supports bar/123 and dies in attempt to serve this bogus request and never responds

This will result in an HttpRequestException with HttpRequestError.VersionNegotiationError and also logged in error.type, so users won't be left clueless. Note that HttpRequestMessage.Version alone is not sufficient to troubleshoot such problems, VersionPolicy is also needed, which makes this information hard to standardize (like you mention in open-telemetry/semantic-conventions#686 (comment), okhttp uses an array of allowed protocols). Also, we do emit this info in our event logs and IMO such diagnostic information belongs more to logging than to metrics.

[lmolkova]

@antonfirsov makes sense, thank you

[MihaZupan]

Looks like the version part has been answered - #97395 (comment).

Closing this as a dupe of #94829 to continue tracking the default server.port part.