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

Plans for opt-in OpenTelemetry attribute implementation and configuration? #103302

Closed
matt-hensley opened this issue Jun 11, 2024 · 12 comments
Closed

Comments

@matt-hensley
Copy link

Opening this against the runtime repo as it covers ASP.NET Core and HttpClient but may have further implications, please let me know if this is the wrong location.

For opt-in OpenTelemetry attributes, what is the strategy for implementation and configuration?

A good example is the http.server.request.duration metric which has two opt-in attributes: server.address and server.port.

As noted in the .NET-specific OTel semantic conventions, these opt-in attributes are not reported and do not have an opt-in mechanism.

It is possible use tag enrichment via IHttpMetricsTagsFeature to add both attributes. Any user interested in opt-in attributes would need to write enrichment code to populate the values.

I am interested if there are plans for the addition of opt-in OpenTelemety attributes, and if consideration has been given to a common opt-in pattern.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 11, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 11, 2024
@cijothomas
Copy link
Contributor

open-telemetry/opentelemetry-dotnet#3373 (comment) This maybe a good option to consider.

@cijothomas
Copy link
Contributor

If dotnet/aspnetcore#52439 is not landing in .NET9 timeframe, then probably a good idea to implement something in OTel Instrumentation libraries itself: open-telemetry/opentelemetry-dotnet-contrib#1786

@ericstj ericstj added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jun 18, 2024
@ericstj ericstj added this to the 9.0.0 milestone Jun 18, 2024
@ericstj
Copy link
Member

ericstj commented Jun 18, 2024

cc @tarekgh

@vcsjones vcsjones removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 18, 2024
@tarekgh tarekgh removed their assignment Jun 18, 2024
@tarekgh
Copy link
Member

tarekgh commented Jun 18, 2024

@tarekgh
Copy link
Member

tarekgh commented Jun 18, 2024

CC @samsp-msft

@JamesNK
Copy link
Member

JamesNK commented Jun 18, 2024

There are other things to configure, such as the list of available HTTP method values in metrics.

This configuration is difficult because I don't think it should be static (yuck), but there also isn't anywhere good to place it. The http handler maybe?

It's not urgent and can be considered based on demand.

@CarnaViire
Copy link
Member

Triage: Not urgent, already marked as Future.

If you need the feature, please upvote the top post to help us prioritize. Thanks!

@CarnaViire CarnaViire removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Jul 1, 2024
@antonfirsov
Copy link
Member

antonfirsov commented Jul 1, 2024

Weird, #94829 states that server.port should be Required. @lmolkova has this changed since open-telemetry/semantic-conventions#459?

I agree with #103302 (comment). This needs an API, for that we are already too late in the release cycle for .NET 9.

@antonfirsov

This comment was marked as resolved.

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

lmolkova commented Jul 1, 2024

Weird, #94829 states that server.port should be Required. @lmolkova has this changed since open-telemetry/semantic-conventions#459?

@antonfirsov the port used to be opt-in when .NET semconv were stabilized and then was changed to required right before OTel HTTP was stabilized.

@antonfirsov
Copy link
Member

Nevermind my #103302 (comment), I was looking at the server metric.

@antonfirsov
Copy link
Member

Duplicate of #104738 and #94829.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants