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.

Sign up for GitHub

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

Fix CHANGELOG.md (server.name does not exist) #126

Merged
merged 6 commits into from
Jun 22, 2023
Merged

Fix CHANGELOG.md (server.name does not exist) #126

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9711133
Fix CHANGELOG.md (server.name does not exist)
Oberon00 Jun 21, 2023
53acf50
Revert stray newline
Oberon00 Jun 21, 2023
e8c8012
Really fix stray newline
Oberon00 Jun 21, 2023
c4c9007
Fix accidental reversal/reduncancy
Oberon00 Jun 21, 2023
15ba2fd
Minimize
Oberon00 Jun 21, 2023
e0e5eb3
Merge branch 'main' into Oberon00-fix-changelog
Oberon00 Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ release.
([#3402](https://github.com/open-telemetry/opentelemetry-specification/pull/3402))
BREAKING: rename `net.peer.name` to `server.address` on client side and to `client.address` on server side,
`net.peer.port` to `server.port` on client side and to `client.port` on server side,
`net.host.name` and `net.host.port` to `server.name` and `server.port` (since `net.host.*` attributes only applied to server instrumentation)
`net.host.name` and `net.host.port` to `server.address` and `server.port` (since `net.host.*` attributes only applied to server instrumentation),
Copy link
Member Author

@Oberon00 Oberon00 Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

BTW, the staments of the form "net.* only applied to client/server" are not true. The mentioned "non-applicable" cases may have been less commonly used or exotic or even never used, but purely from the spec / conventions point of view for the general attributes (not talking about specific contexts like http), peer and host were defined completely independent of any higher-level protocol's logical role like "client" or "server"

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`net.host.name` and `net.host.port` to `server.address` and `server.port` (since `net.host.*` attributes only applied to server instrumentation),
`net.host.name` and `net.host.port` to `server.address` and `server.port` (since `net.host.*` attributes primarily applied to server instrumentation),

How about this then?

Copy link
Member Author

@Oberon00 Oberon00 Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is IMHO not enough of a reason and should be changed to the more wordy:

Suggested change
`net.host.name` and `net.host.port` to `server.address` and `server.port` (since `net.host.*` attributes only applied to server instrumentation),
`net.host.name` and `net.host.port` to `server.address` and `server.port` on server side (typical case) and `client.address` and `client.port` on client side,

But that would need to be changed in all the other points below as well.

It seems odd that a CHANGELOG entry produces so much food for discussion, how about removing the mapping from here entirely for now and instead doing what #43 suggests with a proper document?

Copy link
Member

Choose a reason for hiding this comment

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

Resolving for now per #126 (comment).

Copy link
Member Author

@Oberon00 Oberon00 Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(unresolved after merge for easier future reference)

`net.sock.peer.addr` to `server.socket.address` on client side and to `client.socket.address` on server side,
`net.sock.peer.port` to `server.socket.port` on client side and to `client.socket.port` on server side,
`net.sock.peer.name` to `server.socket.domain` (since `net.sock.peer.name` only applied to client instrumentation),
Expand Down Expand Up @@ -94,4 +94,3 @@ release.
([#17](https://github.com/open-telemetry/opentelemetry-specification/pull/17))
- Mark service.version as stable.
([#106](https://github.com/open-telemetry/semantic-conventions/pull/106))