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

Use non-deprecated span attributes #184

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KasonBraley
Copy link

Closes #129.

  • net.peer.name -> replaced with server.address on client spans and client.address on server spans.
  • net.peer.port -> replaced with server.port on client spans and client.port on server spans.
  • http.status_code -> replaced with http.response.status_code.

- `net.peer.name` -> replaced with `server.address` on client spans and `client.address` on server spans.
- `net.peer.port` -> replaced with `server.port` on client spans and `client.port` on server spans.
- `http.status_code` -> replaced with `http.response.status_code`.

Signed-off-by: Kason Braley <[email protected]>
return []attribute.KeyValue{
semconv.NetPeerNameKey.String(host),
semconv.NetPeerPortKey.Int(portInt),
Copy link
Collaborator

Choose a reason for hiding this comment

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

For backwards compatibility could we keep the current deprecated attributes too?

Copy link
Author

Choose a reason for hiding this comment

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

How would you like statusCodeAttribute to behave now? Would you like to have it return a slice of both now instead of just the one attribute.KeyValue?

Copy link
Collaborator

@emcfarlane emcfarlane Oct 15, 2024

Choose a reason for hiding this comment

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

Oh yeah that is a bit of an issue, maybe the signature could be changed to append the values or return an array?

There will be a few testcases that require updating from these changes. Mind going through and updating each?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds goo. Yes I noticed that as well, should have ran the tests locally before opening the PR, that's my bad. I'll tackle the tests, might take me some time as there is a lot to update. Thanks!

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.

Migrate off deprecated span attributes
2 participants