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

attributes: values must be comparable (not documented) #5101

Closed
benpbrown opened this issue Jan 4, 2022 · 1 comment · Fixed by #5109
Closed

attributes: values must be comparable (not documented) #5101

benpbrown opened this issue Jan 4, 2022 · 1 comment · Fixed by #5109
Assignees

Comments

@benpbrown
Copy link

benpbrown commented Jan 4, 2022

Since #4855, attribute values must either implement Equal(o interface{}) bool or be comparable.

This previously wasn't the case and this restriction isn't documented as far as I can tell.

https://pkg.go.dev/google.golang.org/grpc/attributes mentions that keys must be hashable, but does not mention that values must be comparable [if they don't implement Equal]. That information can maybe be derived from If Equal is not implemented, standard equality is used to determine if the two values are equal., but I think it could be more clear.

I would prefer that it was possible to use un-comparable standard types (e.g. using https://pkg.go.dev/github.com/google/go-cmp/cmp as the fallback comparator), but if that isn't possible, I think there should be some documentation indicating directly that values need to be comparable.

We saw this panic in services after a gRPC version bump.

What version of gRPC are you using?

grpc@v1.43.0

What version of Go are you using (go version)?

Go playground

What operating system (Linux, Windows, …) and version?

Go playground

What did you do?

Executed https://go.dev/play/p/TYD55tjljGj

What did you expect to see?

True or false

What did you see instead?

A panic

panic: runtime error: comparing uncomparable type map[string]string

goroutine 1 [running]:
google.golang.org/grpc/attributes.(*Attributes).Equal(0x4887e0, 0xc00009acc0)
	/tmp/gopath3674506896/pkg/mod/google.golang.org/grpc@v1.43.0/attributes/attributes.go:93 +0x194
main.main()
	/tmp/sandbox3370463311/prog.go:16 +0x207

Program exited.
@menghanl
Copy link
Contributor

menghanl commented Jan 5, 2022

We cannot use the cmp package because it's intended to only be used in tests, and it panics (e.g. for structs with unexported fields).

We will update the doc to add that all values must be comparable.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants