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

Attributes should only have default values if supplying the attribute is (otherwise) required #385

Closed
trask opened this issue Oct 11, 2023 · 6 comments · Fixed by #398
Closed
Assignees

Comments

@trask
Copy link
Member

trask commented Oct 11, 2023

In order to avoid ambiguity between default attribute value and unknown attribute value. 

motivated by @lmolkova's #373 (comment):

This way, a lack of protocol would mean a lack of information (and not default value)

@trask
Copy link
Member Author

trask commented Oct 12, 2023

It looks like these are the (HTTP) attributes potentially in this boat:

network.protocol.name

- ref: network.protocol.name
examples: ['http', 'spdy']
requirement_level:
recommended: if not default (`http`).

server.port (on server side)

- ref: server.port
sampling_relevant: true
requirement_level:
recommended: If not default (`80` for `http` scheme, `443` for `https`).

@trask
Copy link
Member Author

trask commented Oct 12, 2023

I'm not sure if we can conditionally require network.protocol.name when it's not the default value, since that's a low-level attribute which may not always be capturable(?). It's also not a generally useful piece of data in the scope of HTTP semconv. Maybe it should be opt-in?

For server.port (on the server side), it seems like that's pretty accessible to HTTP server instrumentation, so maybe we can change it from recommended to conditionally_required(?)

@Oberon00
Copy link
Member

I don't think we can solve this by just saying "there is no default value". In effect, this is a recommendation for backends which assumption to make if they need one. If we remove it and backends still need to know the value, they will still make an assumption.

@lmolkova
Copy link
Contributor

lmolkova commented Oct 12, 2023

I'm not sure if we can conditionally require network.protocol.name when it's not the default value, since that's a low-level attribute which may not always be capturable(?). It's also not a generally useful piece of data in the scope of HTTP semconv. Maybe it should be opt-in?

The alternative is to remove spdy - it's been deprecated and removed from browsers 7+ years ago. Assuming we don't expect spdy, we can remove network.protocol.name from HTTP semconv.
It de-facto makes it opt-in for any custom instrumentation that wants to capture the protocol.

Backends can safely assume http and it will be inaccurate for those who still use spdy. Given they use a deprecated and experimental protocol, things like this should be expected and tolerable.

@lmolkova
Copy link
Contributor

lmolkova commented Oct 12, 2023

For server.port (on the server side), it seems like that's pretty accessible to HTTP server instrumentation, so maybe we can change it from recommended to conditionally_required(?)

Agree, I'm surprised it's recommended

@Oberon00
Copy link
Member

We might have lost the requirement for the port by mistake, since http.host contained the port as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
4 participants