-
Notifications
You must be signed in to change notification settings - Fork 183
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
Fix CHANGELOG.md (server.name does not exist) #126
Fix CHANGELOG.md (server.name does not exist) #126
Conversation
Oh, it seems that the GH online editor insists on adding not one but two final newlines, and even creates empty commits to enforce that (53acf50) |
@@ -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), |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`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?
There was a problem hiding this comment.
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:
`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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`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?
Changelog introduced in open-telemetry/opentelemetry-specification#3402 refers to a nonexistent attribute.