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

Use non-deprecated span attributes #184

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

Use non-deprecated span attributes #184

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3a1126
Use non-deprecated span attributes
KasonBraley Oct 10, 2024
9805b2c
Keep deprecated NetPeer attributes for backwards compat
KasonBraley Oct 11, 2024
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
21 changes: 15 additions & 6 deletions attributes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func procedureAttributes(procedure string) []attribute.KeyValue {
func requestAttributes(spec connect.Spec, peer connect.Peer) []attribute.KeyValue {
var attrs []attribute.KeyValue
if addr := peer.Addr; addr != "" {
attrs = append(attrs, addressAttributes(addr)...)
attrs = append(attrs, addressAttributes(spec, addr)...)
}
name := strings.TrimLeft(spec.Procedure, "/")
protocol := protocolToSemConv(peer.Protocol)
Expand All @@ -83,17 +83,26 @@ func requestAttributes(spec connect.Spec, peer connect.Peer) []attribute.KeyValu
return attrs
}

func addressAttributes(address string) []attribute.KeyValue {
func addressAttributes(spec connect.Spec, address string) []attribute.KeyValue {
if host, port, err := net.SplitHostPort(address); err == nil {
portInt, err := strconv.Atoi(port)
if err == nil {
if spec.IsClient {
return []attribute.KeyValue{
semconv.ServerAddressKey.String(host),
semconv.ServerPortKey.Int(portInt),
}
}
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 •
edited
Loading

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!

semconv.ClientAddressKey.String(host),
semconv.ClientPortKey.Int(portInt),
}
}
}
return []attribute.KeyValue{semconv.NetPeerNameKey.String(address)}
if spec.IsClient {
return []attribute.KeyValue{semconv.ServerAddressKey.String(address)}
}
return []attribute.KeyValue{semconv.ClientAddressKey.String(address)}
}

func statusCodeAttribute(protocol string, serverErr error) (attribute.KeyValue, bool) {
Expand All @@ -111,7 +120,7 @@ func statusCodeAttribute(protocol string, serverErr error) (attribute.KeyValue,
// A "not modified" error is special: it's code is technically "unknown" but
// it would be misleading to label it as an unknown error since it's not really
// an error, but rather a sentinel to trigger a "304 Not Modified" HTTP status.
return semconv.HTTPStatusCodeKey.Int(http.StatusNotModified), true
return semconv.HTTPResponseStatusCodeKey.Int(http.StatusNotModified), true
}
codeKey := attribute.Key("rpc." + protocol + ".error_code")
if serverErr != nil {
Expand Down
Loading