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

Change server.address and server.port from required to recommended on HTTP Server Spans #114

Merged

Conversation

trask
Copy link
Member

@trask trask commented Jun 15, 2023

Fixes #111

@trask trask requested review from a team June 15, 2023 02:46
@Oberon00
Copy link
Member

From the issue:

because you can typically get similar identification of the service from service.name

Citation needed. I think getting similar information from a resource attribute is fine, but is service.name really the right one? Maybe it should be host.name?
There should be a recommendation which (resource) attributes to set & form the URL from in case server.name is missing.

server.port will be even harder to get from resource attributes, the current conditional requirement seems fine?

Given the recent discussion, I think it is also fine to add an additional condition like "if available from a trusted source" (since the Host header cannot be trusted and a local configuration is sometimes not available)

@lmolkova
Copy link
Contributor

lmolkova commented Jun 15, 2023

Citation needed. I think getting similar information from a resource attribute is fine, but is service.name really the right one? Maybe it should be host.name?

it doesn't feel like HTTP concern (a set of resource attributes that describe host precisely). It's the same for all RPC calls and resource attribute definitions would be a good source of information. In practice, it can be a chain of fallbacks where host.name has the highest priority, service.name has the lowest one.

There could be more attributes added to resources in the future that can be even more precise, and HTTP spec should not need to be updated when it happens.

@trask
Copy link
Member Author

trask commented Jun 19, 2023

@open-telemetry/specs-semconv-approvers this could use additional reviews, as we are trying to freeze HTTP semantic conventions soon, thx!

@reyang reyang merged commit 2055854 into open-telemetry:main Jun 21, 2023
@trask trask deleted the change-server-address-port-to-recommended branch October 14, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should server.address and server.port be changed from required to recommended on SERVER spans?
6 participants