Skip to content

Remove conditional requirement on network.peer.address and network.peer.port#449

Merged
AlexanderWert merged 11 commits into
open-telemetry:mainfrom
trask:fix-conditional-requirement
Oct 30, 2023
Merged

Remove conditional requirement on network.peer.address and network.peer.port#449
AlexanderWert merged 11 commits into
open-telemetry:mainfrom
trask:fix-conditional-requirement

Conversation

@trask

@trask trask commented Oct 25, 2023

Copy link
Copy Markdown
Member

I missed these in #342.

Based on #342 (comment):

I disagree with not populating. Then you'll need a database of instrumentation library version/name to find if absence means "not implemented" or "same".

we decided to remove the conditional and always populate these attributes when network-layer instrumentation exists.

Changes

Remove conditional requirement on network.peer.address and network.peer.port.

Merge requirement checklist

@trask trask marked this pull request as ready for review October 25, 2023 22:38
@trask trask requested review from a team October 25, 2023 22:38

@mateuszrzeszutek mateuszrzeszutek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do the same thing for the server.port in HTTP server spans? Currently the spec says it's conditionally required:

If server.address is set and the port is not default (80 for http scheme, 443 for https).

Should we remove the "If server.address is set" part?

@trask

trask commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

Should we do the same thing for the server.port in HTTP server spans? Currently the spec says it's conditionally required:

If server.address is set and the port is not default (80 for http scheme, 443 for https).

Should we remove the "If server.address is set" part?

My understanding is that if we remove that part, then server.port becomes required (when non-default) even if you don't set server.address (which we dropped from required to recommended in #114)

@trask

trask commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

oh, maybe you're suggesting then that we (similarly) make network.peer.port conditionally required (on HTTP client spans):

If network.peer.address is set and the port is not default (80 for http scheme, 443 for https).

that seems to make sense to me

@mateuszrzeszutek

Copy link
Copy Markdown
Member

oh, maybe you're suggesting then that we (similarly) make network.peer.port conditionally required (on HTTP client spans):

I didn't mean that actually.
This change makes it possible to set network.peer.port without network.peer.address -- or, at the very least, that's how I read it. I thought it introduces some asymmetry between how we treat the server.* and network.(peer|local).* attributes.

@trask

trask commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

This change makes it possible to set network.peer.port without network.peer.address -- or, at the very least, that's how I read it. I thought it introduces some asymmetry between how we treat the server.* and network.(peer|local).* attributes.

ya, it's a good point

do you think of adding similar condition on network.peer.port addresses the issue?

If network.peer.address is set and the port is not default (80 for http scheme, 443 for https).

@mateuszrzeszutek

Copy link
Copy Markdown
Member

If network.peer.address is set

This PR removed that part 😄 -- yes, I think reverting it back would resolve the confusion.
About the port condition: I think it's not relevant to my concern, personally I'd probably prefer to leave it as it is -- while the server.* attributes are semantically tied to HTTP (in the context of this document), the network level attributes may provide a bit broader context without HTTP-specific restrictions (e.g. like network.transport and .type if they weren't opt in).

@trask

trask commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

If network.peer.address is set

This PR removed that part 😄 -- yes, I think reverting it back would resolve the confusion. About the port condition: I think it's not relevant to my concern, personally I'd probably prefer to leave it as it is -- while the server.* attributes are semantically tied to HTTP (in the context of this document), the network level attributes may provide a bit broader context without HTTP-specific restrictions (e.g. like network.transport and .type if they weren't opt in).

👍 fixed

@trask

trask commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

@AlexanderWert, @lmolkova, @mateuszrzeszutek and @trisch-me - I reset all the approvals because I've made additional changes, ptal, thx

Comment thread model/trace/http.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants