Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP client metrics: url.scheme should be opt-in, server.port should be required #94829

Closed
lmolkova opened this issue Nov 16, 2023 · 6 comments · Fixed by #104741
Closed

HTTP client metrics: url.scheme should be opt-in, server.port should be required #94829

lmolkova opened this issue Nov 16, 2023 · 6 comments · Fixed by #104741
Assignees
Labels
area-System.Net.Http in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@lmolkova
Copy link

Description

OTel HTTP semconv made the last-minute change open-telemetry/semantic-conventions#459 before stability and now HTTP client metrics are not fully aligned with the spec (sorry).

Even though it's a pretty minor difference it'd be nice to align with the OTel spec at some point.

Reproduction Steps

N/A

Expected behavior

  • url.scheme is now opt-in, so should be set when explicitly enabled (or not set at all)
  • server.port should be set even when it has a default value

Actual behavior

scheme is always set, port is set when not default.

tags.Add("url.scheme", requestUri.Scheme);
tags.Add("server.address", requestUri.Host);
// Add port tag when not the default value for the current scheme
if (!requestUri.IsDefaultPort)
{
tags.Add("server.port", requestUri.Port);
}

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 16, 2023
@ghost
Copy link

ghost commented Nov 16, 2023

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

Issue Details

Description

OTel HTTP semconv made the last-minute change open-telemetry/semantic-conventions#459 before stability and now HTTP client metrics are not fully aligned with the spec (sorry).

Even though it's a pretty minor difference it'd be nice to align with the OTel spec at some point.

Reproduction Steps

N/A

Expected behavior

  • url.scheme is now opt-in, so should be set when explicitly enabled (or not set at all)
  • server.port should be set even when it has a default value

Actual behavior

scheme is always set, port is set when not default.

tags.Add("url.scheme", requestUri.Scheme);
tags.Add("server.address", requestUri.Host);
// Add port tag when not the default value for the current scheme
if (!requestUri.IsDefaultPort)
{
tags.Add("server.port", requestUri.Port);
}

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: lmolkova
Assignees: -
Labels:

area-System.Net.Http, untriaged

Milestone: -

@antonfirsov
Copy link
Member

antonfirsov commented Nov 16, 2023

The current state (having a redundant scheme tag, not reporting default ports) doesn't actively hurt users from a practical POV. Given it's such a minor aspect, I wonder if maintaining maximum standard-compatibility is worth the burden of a breaking change where we may need to implement a compatibility switch, adding complexity to the code. @noahfalk what's your take on this?

@antonfirsov
Copy link
Member

Triage: adding this to 9.0 for decision making, and potential fixing (see comment above).

@antonfirsov
Copy link
Member

Related: #103302. Opt-in attributes likely need an API addition. Punting this to .NET 10.

@antonfirsov antonfirsov modified the milestones: 9.0.0, 10.0.0 Jul 1, 2024
@lmolkova
Copy link
Author

lmolkova commented Jul 1, 2024

@antonfirsov

would it be possible to report server.port (and don't change anything for scheme)? It would not require API changes, it does not seem to have any downsides (telemetry volume is the same, not breaking) and would align .NET metrics with OTel better.

@antonfirsov antonfirsov modified the milestones: 10.0.0, 9.0.0 Jul 1, 2024
@antonfirsov
Copy link
Member

I was confused, since I was looking at the wrong metrics. We should add server.port.

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants