[Instrumentation.Http, Instrumentation.AspNetCore] Always set server.port attribute for spans

[Kielek]

Fixes N/A
Design discussion issue N/A

I do not see any conditions related to the default port numbers in https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md

Changes

[Instrumentation.Http, Instrumentation.AspNetCore] Always set server.port attribute

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, 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

All modified and coverable lines are covered by tests ✅

Project coverage is 84.86%. Comparing base (6250307) to head (3da6a7f).
Report is 128 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5419      +/-   ##
==========================================
+ Coverage   83.38%   84.86%   +1.48%     
==========================================
  Files         297      284      -13     
  Lines       12531    12204     -327     
==========================================
- Hits        10449    10357      -92     
+ Misses       2082     1847     -235     

Flag	Coverage Δ
unittests	?
unittests-Instrumentation-Experimental	24.29% <100.00%> (?)
unittests-Instrumentation-Stable	24.34% <100.00%> (?)
unittests-Solution-Experimental	84.79% <100.00%> (?)
unittests-Solution-Stable	84.82% <100.00%> (?)

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

Files	Coverage Δ
...tation.AspNetCore/Implementation/HttpInListener.cs	90.06% <100.00%> (+0.48%)	⬆️
...tp/Implementation/HttpHandlerDiagnosticListener.cs	70.63% <100.00%> (-0.24%)	⬇️
...plementation/HttpWebRequestActivitySource.netfx.cs	80.73% <100.00%> (-0.04%)	⬇️

... and 61 files with indirect coverage changes

[Kielek]

@vishweshbankwar, could you please review?

[vishweshbankwar]

@vishweshbankwar, could you please review?

@Kielek - Thanks, looks like this spec change was a miss on our side.

[vishweshbankwar]

@Kielek - Could you please open an issue on dotnet runtime as well for .NET 8.0? It has the same issue https://github.com/dotnet/runtime/blob/5ade1eeede3b7cefd460351fa589a3f668334a8e/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs#L194-L198

[vishweshbankwar]

LGTM - But we should wait to hear from .NET about #5419 (comment) before we make a change on HttpClient metrics side.

[Kielek]

@vishweshbankwar, I have double checked the metrics part and the current implementation seems to be in sync with .NET specific semantic conventions. The semantic convention is different for part of the attributes, if compare it with https://github.com/open-telemetry/semantic-conventions/blob/v1.24.0/docs/http/http-metrics.md

Before I will create the runtime issue I would like to check with @lmolkova what is the plan for this. Is there any chance, that the common metrics will be handled in the same way for .NET9+?

For this PR - if we agree to keep current metrics behavior, I can drop changes related to metrics. I do not see any conflicts for spans.

[lmolkova]

I think this is tracked on the .NET side via dotnet/runtime#94829 and is considered for the .NET 9. I don't know however if plans have been solidified. Maybe @antonfirsov can comment?

[vishweshbankwar]

I think this is tracked on the .NET side via dotnet/runtime#94829 and is considered for the .NET 9. I don't know however if plans have been solidified. Maybe @antonfirsov can comment?

I think we should not make the change then in instrumentation library. Users moving from .NET6.0/.NET7.0 to .NET8.0 will see a breaking change and they will see another breaking change when moving to .NET9.0 (if it gets changed there).

@antonfirsov - Any chance the change can be backported to 8.0?

[antonfirsov]

Any chance the change can be backported to 8.0?

No, we consider those breaking.

[Kielek]

@vishweshbankwar, reverted changes related to metrics in 446a701

We should consider fixing behavior for .NET Framework metrics. I think that spans related changes should be merged.

[cijothomas]

Fixing adding the port attribute to Metrics - is this considered a breaking change?

[Kielek]

Fixing adding the port attribute to Metrics - is this considered a breaking change?

In scope of the OpenTelemetry packages, I would consider it as a bugfixing. I am not sure what is the perspective on this by .NET team.

If possible, please merge this one, then we can address .NET Framework scenario in separate PR (probably not changing anything for .NET6 and 7 as both will be EoS this year.

[antonfirsov]

Fixing adding the port attribute to Metrics - is this considered a breaking change?

Addition, removal or renaming of attributes may break existing queries therefore it's considered to be a behavioral breaking change. We kept merging last minute changes to maximize compatibility and minimize the changes needed between .NET 8 -> .NET 9, but unfortunately open-telemetry/semantic-conventions#459 landed after the .NET 8 GA snap date.

For Metrics, we are more willing to take these kind of changes between major version than in general in .NET so dotnet/runtime#94829 will be most likely implemented for .NET 9.