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

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

Merged

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Jun 21, 2023

Changelog introduced in open-telemetry/opentelemetry-specification#3402 refers to a nonexistent attribute.

@Oberon00 Oberon00 requested review from a team June 21, 2023 07:50
@Oberon00 Oberon00 requested a review from lmolkova June 21, 2023 07:51
@Oberon00
Copy link
Member Author

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),
Copy link
Member Author

@Oberon00 Oberon00 Jun 21, 2023

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

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

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),
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?

@Oberon00
Copy link
Member Author

I suggest to merge this as-is, since it fixes an obvious editorial mistake, and any remaining changes regarding client vs. server side can/should be discussed in #43 (and possibly #62).

@reyang
Copy link
Member

reyang commented Jun 22, 2023

I suggest to merge this as-is, since it fixes an obvious editorial mistake, and any remaining changes regarding client vs. server side can/should be discussed in #43 (and possibly #62).

+1

@reyang

This comment was marked as resolved.

@Oberon00

This comment was marked as resolved.

@arminru arminru merged commit 9b45531 into open-telemetry:main Jun 22, 2023
@arminru arminru deleted the Oberon00-fix-changelog branch June 22, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants