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

Refactor attribute.Value similar to log.Value #4983

Closed
MrAlias opened this issue Feb 26, 2024 · 5 comments
Closed

Refactor attribute.Value similar to log.Value #4983

MrAlias opened this issue Feb 26, 2024 · 5 comments
Assignees
Labels
pkg:attribute Related to the attribute package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Feb 26, 2024

For example:

type Value struct {
	num uint64
	any any
}

Initial testing shows large performance improvements for slice types:

$ go test -bench="./As."
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/attribute
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkBool/AsBool-8    	885851749	         1.277 ns/op	       0 B/op	       0 allocs/op
BenchmarkBoolSlice/AsBoolSlice-8         	 6934695	       168.7 ns/op	      27 B/op	       2 allocs/op
BenchmarkInt64/AsInt64-8                 	937453924	         1.260 ns/op	       0 B/op	       0 allocs/op
BenchmarkInt64Slice/AsInt64Slice-8       	 6373528	       183.6 ns/op	      48 B/op	       2 allocs/op
BenchmarkFloat64/AsFloat64-8             	939700726	         1.263 ns/op	       0 B/op	       0 allocs/op
BenchmarkFloat64Slice/AsFloat64Slice-8   	 6258823	       185.6 ns/op	      48 B/op	       2 allocs/op
BenchmarkString/AsString-8               	701729328	         1.688 ns/op	       0 B/op	       0 allocs/op
BenchmarkStringSlice/AsStringSlice-8     	 5726421	       211.4 ns/op	      72 B/op	       2 allocs/op

vs

$ go test -run="^$" -bench="./As."
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/attribute
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkBool/AsBool-8    	592013719	         1.886 ns/op	       0 B/op	       0 allocs/op
BenchmarkBoolSlice/AsBoolSlice-8         	891150913	         1.371 ns/op	       0 B/op	       0 allocs/op
BenchmarkInt64/AsInt64-8                 	811699467	         1.481 ns/op	       0 B/op	       0 allocs/op
BenchmarkInt64Slice/AsInt64Slice-8       	624627015	         1.905 ns/op	       0 B/op	       0 allocs/op
BenchmarkFloat64/AsFloat64-8             	497588934	         2.519 ns/op	       0 B/op	       0 allocs/op
BenchmarkFloat64Slice/AsFloat64Slice-8   	621052142	         1.942 ns/op	       0 B/op	       0 allocs/op
BenchmarkString/AsString-8               	967753633	         1.273 ns/op	       0 B/op	       0 allocs/op
BenchmarkStringSlice/AsStringSlice-8     	611442519	         1.977 ns/op	       0 B/op	       0 allocs/op
PASS
@MrAlias MrAlias self-assigned this Feb 26, 2024
@MrAlias MrAlias added the pkg:attribute Related to the attribute package label Feb 26, 2024
@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 26, 2024

The fact that Value comparison is used to equate a Set prevents this refactor currently (i.e. slice values become non-equal with this change, though they are still comparable).

@pellared
Copy link
Member

AFAIK there is also a behavioral difference. attribute package returns copies and log package returns actual values. However, this behavior is not documented so the change could be acceptable.

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 27, 2024

The fact that Value comparison is used to equate a Set prevents this refactor currently (i.e. slice values become non-equal with this change, though they are still comparable).

A way to solve this is to actually use the Distinct type instead of direct comparison of the Set (which is documented to not be used for comparison and has an Equals method). The Distinct type could hold a hash of the passed values to a new Set. This hash would be a uint64 similar to this.

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 27, 2024

@pellared follow up to our offline conversation: those benchmark numbers include the copy (using slices.Clone)!

https://github.com/MrAlias/opentelemetry-go/blob/set-registry/attribute/slice_utils.go

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 27, 2024

Given the Value is comparable and this refactor would make that comparison not equate similar values due to pointer refs being different, a Equals method should likely be added to the Value type and it should be documented as such.

@MrAlias MrAlias closed this as not planned Won't fix, can't repro, duplicate, stale Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:attribute Related to the attribute package
Projects
None yet
Development

No branches or pull requests

2 participants