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

[trask]

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

• CONTRIBUTING.md guidelines followed.
• CHANGELOG.md updated for non-trivial changes.
• schema-next.yaml updated with changes to existing conventions.

[mateuszrzeszutek]

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]

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]

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]

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]

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]

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]

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]

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