Fix built-in HttpClient metrics documentation

[xakep139]

Summary

Right now docs say that the network protocol version for http.client.request.duration metric is always available, which isn't true: https://github.com/dotnet/runtime/blob/55fbb23f8c4e05a17ee27ea19679813f7f5cf285/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs#L113

---

Internal previews

📄 File	🔗 Preview link
docs/core/diagnostics/built-in-metrics-system-net.md	System.Net metrics

[xakep139]

cc @JamesNK @antonfirsov

[antonfirsov]

Thanks! 💯

@lmolkova we cannot populate network.protocol.version when HttpResponseMessage is missing because of an exception. Can we keep the attribute's Requirement Level Recommended in the spec, or shall we change it to Conditionally Required?
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/dotnet/dotnet-http-metrics.md#metric-httpclientconnectionduration

[xakep139]

Please merge the PR if you think it's good to go. Thanks 🙇🏻‍♂️

[lmolkova]

@lmolkova we cannot populate network.protocol.version when HttpResponseMessage is missing because of an exception. Can we keep the attribute's Requirement Level Recommended in the spec, or shall we change it to Conditionally Required? https://github.com/open-telemetry/semantic-conventions/blob/main/docs/dotnet/dotnet-http-metrics.md#metric-httpclientconnectionduration

We can keep Recommended level - it implies that instrumentation can have good reasons not to populate it. It also means that observability backends should not expect it to be there.

I'll send a tiny PR to the spec to add a description on the level.

[lmolkova]

@antonfirsov @xakep139 does this change apply to all client metrics or to http request duration only?

[xakep139]

@antonfirsov @xakep139 does this change apply to all client metrics or to http request duration only?

I've found only these usages:

• MetricsHandler
• ConnectionMetrics
• SocketsHttpHandlerMetrics

@antonfirsov can you please check whether last two are available if there's no response?

[lmolkova]

BTW, any reason to report response version and not a request.Version?

since it's a request duration, it seems reasonable to report request version there, no?

[antonfirsov]

@antonfirsov @xakep139 does this change apply to all client metrics or to http request duration only?

For the other metrics network.protocol.version is always deducable because they are implemented in SocketsHttpHandler.

any reason to report response version and not a request.Version?

HttpResponseMessage.Version is the actual protocol version used to serve the request, which will be result of the ALPN negotiation and may differ from HttpRequestMessage.Version (or HttpClient.DefaultRequestVersion) set before calling Send. IMO this is the value telemetry users are interested in, request.Version and request.VersionPolicy are only inputs for the version negotiation.

[lmolkova]

@antonfirsov thanks for the explanation! Do you think general otel semconv could be more specific and specify that the actual protocol version after negotiation should be used?

[lmolkova]

Also, is there any benefit in falling back to request.Version if there was no response? I'm thinking about misconfigured clients and buggy servers/routers that don't respond when protocol is not supported and then you would be interested in request.Version. Super edge-case, but why not use request version as a fallback?

[antonfirsov]

Do you think general otel semconv could be more specific and specify that the actual protocol version after negotiation should be used

This kind of clarification never hurts, but one should look at version management in other libraries/APIs to phrase it correctly.

Also, is there any benefit in falling back to request.Version if there was no response?

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

[xakep139]

Folks, this PR was intended to fix the documentation and to reflect the current behavior there. My idea was to fix the docs first, and then if we decide - fix the logic/metrics.
I would suggest to continue discussion in the related issue: dotnet/runtime#97395

[antonfirsov]

Apologies for delaying the merge, I wanted to make sure we are on the same page before going ahead.