[HttpClient & HttpWebRequest] Set network protocol version from response object

[vishweshbankwar]

Fixes #4928

Changes

We are currently setting network.protocol.version using the version that was specified in request object. However, the final version depends on the version negotiated with the server. The PR updates to use version from response object. In case of no response, network.protocol.version will not be set.

Please provide a brief description of the changes here.

Merge requirement checklist

• CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

Merging #5043 (0399ac1) into main (c337d1e) will increase coverage by 0.16%.
The diff coverage is 84.61%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5043      +/-   ##
==========================================
+ Coverage   83.32%   83.48%   +0.16%     
==========================================
  Files         296      296              
  Lines       12473    12481       +8     
==========================================
+ Hits        10393    10420      +27     
+ Misses       2080     2061      -19     

Flag	Coverage Δ
unittests	83.48% <84.61%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...tp/Implementation/HttpHandlerDiagnosticListener.cs	73.50% <100.00%> (ø)
...ementation/HttpHandlerMetricsDiagnosticListener.cs	90.14% <100.00%> (ø)
...plementation/HttpWebRequestActivitySource.netfx.cs	82.60% <81.81%> (-0.12%)	⬇️

... and 6 files with indirect coverage changes

[antonfirsov]

Conceptually LGTM, I'm not familiar with implementation details.

[cijothomas]

[5]: network.protocol.version refers to the version of the protocol used and might be different from the protocol client's version. If the HTTP client has a version of 0.27.2, but sends HTTP version 1.1, this attribute should be set to 1.1.

The above wording from https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpclientrequestduration, does not mention that this attribute will not be set if no response received. Should we clarify it ? What if the otel convention is expecting to populate the client's version in such case, instead of not populating at all?

[antonfirsov]

What if the otel convention is expecting to populate the client's version in such case, instead of not populating at all?

The spec says it's Recommended (vs Required), which in my understanding means that an implementation is allowed to omit it with an explanation.

[cijothomas]

What if the otel convention is expecting to populate the client's version in such case, instead of not populating at all?

The spec says it's Recommended (vs Required), which in my understanding means that an implementation is allowed to omit it with an explanation.

I suggest we need to clarify it from semantic conventions. It does not say "Recommended. Populate if a response is obtained, else omit"... Given it clarifies those for certain other attributes, its best to clarify instead of assuming.

Optional attributes can cause issues like this open-telemetry/semantic-conventions#492, so want to make this sure this is not falling into that category from sem. convention standpoint, but only from .NET implementation only.

[vishweshbankwar]

What if the otel convention is expecting to populate the client's version in such case, instead of not populating at all?

The spec says it's Recommended (vs Required), which in my understanding means that an implementation is allowed to omit it with an explanation.

I suggest we need to clarify it from semantic conventions. It does not say "Recommended. Populate if a response is obtained, else omit"... Given it clarifies those for certain other attributes, its best to clarify instead of assuming.

Optional attributes can cause issues like this open-telemetry/semantic-conventions#492, so want to make this sure this is not falling into that category from sem. convention standpoint, but only from .NET implementation only.

I have opened open-telemetry/semantic-conventions#520 to get the clarification.

[vishweshbankwar]

While we wait for the spec clarity, I think it is ok to consider getting this change merged. Because, in our current state we are not setting the version correctly and this PR is addressing that part.

[cijothomas]

While we wait for the spec clarity, I think it is ok to consider getting this change merged. Because, in our current state we are not setting the version correctly and this PR is addressing that part.

Please put a TODO in the metrics instrumentation code linking to the spec issue, and proceed with this PR.

[cijothomas]

Looks good. We may need adjustment based on clarifications, if any, arising from semantic conventions.

[vishweshbankwar]

FYI - open-telemetry/semantic-conventions#520 (comment)

[TimothyMothra]

Is this meant to be set twice?
It's set here (line 272) in the AddExceptionTags and also the out parameter is used to set it again at Line 543.

It's also odd to me that it's being set in the AddExceptionTags helper method. This may be a candidate for refactor in the future.

[vishweshbankwar]

line 543 sets it on metric.

[utpilla]

HttpWebRequestActivitySource.netfx.cs needs to be refactored to make the code simpler to follow. Not blocking this PR.