[HttpClient] Remove http.host and add net.peer.name & net.peer.port to match spec

[vishweshbankwar]

Towards #3373

Changes

Spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-client

e.g. from spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-client-server-example

For significant contributions please make sure you have completed the following items:

• Appropriate CHANGELOG.md updated for non-trivial changes
• Design discussion issue #
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #3832 (4b7825b) into main (82941d6) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4b7825b differs from pull request most recent head 4c760c2. Consider uploading reports for the commit 4c760c2 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3832      +/-   ##
==========================================
- Coverage   87.31%   87.30%   -0.01%     
==========================================
  Files         279      279              
  Lines       10782    10774       -8     
==========================================
- Hits         9414     9406       -8     
  Misses       1368     1368              

Impacted Files	Coverage Δ
...strumentation.Http/Implementation/HttpTagHelper.cs	100.00% <ø> (ø)
...tp/Implementation/HttpHandlerDiagnosticListener.cs	74.22% <100.00%> (+0.54%)	⬆️
...plementation/HttpWebRequestActivitySource.netfx.cs	80.23% <100.00%> (+0.09%)	⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs	56.25% <0.00%> (-6.25%)	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	97.14% <0.00%> (-2.86%)	⬇️
...tpListener/Internal/PrometheusCollectionManager.cs	75.82% <0.00%> (+2.19%)	⬆️

[CodeBlanch]

This LGTM but FYI it will break the peer.service resolution stuff which specifically checks for http.host:

opentelemetry-dotnet/src/OpenTelemetry/Internal/PeerServiceResolver.cs

Line 31 in 131b350

[SemanticConventions.AttributeHttpHost] = 2, // peer.service for Http.

We should update that code as well for this change (can be done as a follow-up).

[vishweshbankwar]

This LGTM but FYI it will break the peer.service resolution stuff which specifically checks for http.host:

opentelemetry-dotnet/src/OpenTelemetry/Internal/PeerServiceResolver.cs

Line 31 in 131b350

[SemanticConventions.AttributeHttpHost] = 2, // peer.service for Http.

We should update that code as well for this change (can be done as a follow-up).

@CodeBlanch - I am not completely familiar with this part. Do you think that will be a breaking change? Given it will change some logic on resolving peer.service in stable exporters?

[CodeBlanch]

@vishweshbankwar It will be annoying to people using Jaeger. It has a strange quirk in that unless all services report into the same instance, it won't identify/render spans as dependencies without peer.service. Beyond that, hard to say. Not sure what all the possible downstream consumers do with it.

[vishweshbankwar]

@vishweshbankwar It will be annoying to people using Jaeger. It has a strange quirk in that unless all services report into the same instance, it won't identify/render spans as dependencies without peer.service. Beyond that, hard to say. Not sure what all the possible downstream consumers do with it.

@CodeBlanch - I looked at this bit more. I think we do not need to change PeerServiceResolver as it falls back to looking for net.peer.name and net.peer.port here. I was thinking that we need to change for this https://github.com/open-telemetry/opentelemetry-dotnet/pull/3858/files but on checking it looks like we set peer.service only for Client and Producer

[CodeBlanch]

LGTM