-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 slice-valued attributes when used as map keys #2223
Conversation
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
5c4e702
to
fe29e9f
Compare
Codecov Report
@@ Coverage Diff @@
## main #2223 +/- ##
=======================================
- Coverage 72.8% 72.7% -0.1%
=======================================
Files 176 176
Lines 12178 12186 +8
=======================================
Hits 8866 8866
- Misses 3070 3078 +8
Partials 242 242
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we need to rewrite the whole logic of all Slice operations. The problem is Go
can't hash []string
, but it can hash *[]string
. So if we store/get a pointer of the slice, we can solve this problem.
For instance, for the StringSlice
:
func StringSliceValue(v []string) Value {
cp := make([]string, len(v))
copy(cp, v)
return Value{
vtype: STRINGSLICE,
- slice: cp,
+ slice: &cp,
}
}
func (v Value) AsStringSlice() []string {
- if s, ok := v.slice.([]string); ok {
- return s
+ if s, ok := v.slice.(*[]string); ok {
+ return *s
}
return nil
}
func (v Value) Emit() string {
switch v.Type() {
case ARRAY, BOOLSLICE, INT64SLICE, FLOAT64SLICE, STRINGSLICE:
- return fmt.Sprint(v.slice)
+ return fmt.Sprint(reflect.ValueOf(v.slice).Elem())
I think there is a secondary concern with the current implementation that this PR addresses which is that it does not freeze the representation of the provided slice as was done with the prior array implementation. This means that slice values can be changed after they are set as attributes and will change the attribute values, which may lead to unexpected results. |
The passed slice values are copied to a new slice to prevent just this. |
This looks to be adding an allocation to most of the slice methods. I'm guessing because references are escaping to the $ go test -bench=.
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/attribute
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkBool/Value-8 843200180 1.362 ns/op 0 B/op 0 allocs/op
BenchmarkBool/KeyValue-8 141956227 8.504 ns/op 0 B/op 0 allocs/op
BenchmarkBool/AsBool-8 835002997 1.432 ns/op 0 B/op 0 allocs/op
BenchmarkBool/Any-8 56752633 20.81 ns/op 0 B/op 0 allocs/op
BenchmarkBool/Emit-8 149114601 8.439 ns/op 0 B/op 0 allocs/op
BenchmarkBool/Array/KeyValue-8 6628058 176.9 ns/op 6 B/op 2 allocs/op
BenchmarkBool/Array/Emit-8 4142497 276.9 ns/op 24 B/op 1 allocs/op
BenchmarkBoolSlice/Value-8 5420106 220.6 ns/op 30 B/op 3 allocs/op
BenchmarkBoolSlice/KeyValue-8 5002653 295.3 ns/op 30 B/op 3 allocs/op
BenchmarkBoolSlice/AsBoolSlice-8 19226403 56.29 ns/op 8 B/op 1 allocs/op
BenchmarkBoolSlice/Any-8 4851915 240.2 ns/op 30 B/op 3 allocs/op
BenchmarkBoolSlice/Emit-8 4447971 270.8 ns/op 24 B/op 1 allocs/op
BenchmarkInt/Value-8 863892535 1.382 ns/op 0 B/op 0 allocs/op
BenchmarkInt/KeyValue-8 141507870 8.492 ns/op 0 B/op 0 allocs/op
BenchmarkInt/Any-8 60095469 19.39 ns/op 0 B/op 0 allocs/op
BenchmarkInt/Emit-8 100000000 10.31 ns/op 0 B/op 0 allocs/op
BenchmarkInt/Array/KeyValue-8 6309688 190.2 ns/op 48 B/op 2 allocs/op
BenchmarkInt/Array/Emit-8 4139844 294.6 ns/op 16 B/op 1 allocs/op
BenchmarkIntSlice/Value-8 5289876 228.9 ns/op 72 B/op 3 allocs/op
BenchmarkIntSlice/KeyValue-8 4230032 255.3 ns/op 72 B/op 3 allocs/op
BenchmarkIntSlice/Any-8 4029014 264.6 ns/op 72 B/op 3 allocs/op
BenchmarkIntSlice/Emit-8 4086672 296.7 ns/op 16 B/op 1 allocs/op
BenchmarkInt8/Any-8 56067631 19.33 ns/op 0 B/op 0 allocs/op
BenchmarkInt16/Any-8 62364078 19.12 ns/op 0 B/op 0 allocs/op
BenchmarkInt32/Any-8 58327666 21.49 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/Value-8 875788021 1.437 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/KeyValue-8 141532423 8.448 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/AsInt64-8 832411033 1.478 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/Any-8 59651652 21.10 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/Emit-8 100000000 10.98 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/Array/KeyValue-8 5856169 247.7 ns/op 48 B/op 2 allocs/op
BenchmarkInt64/Array/Emit-8 3235882 362.7 ns/op 16 B/op 1 allocs/op
BenchmarkInt64Slice/Value-8 5114036 235.6 ns/op 72 B/op 3 allocs/op
BenchmarkInt64Slice/KeyValue-8 4943851 237.8 ns/op 72 B/op 3 allocs/op
BenchmarkInt64Slice/AsInt64Slice-8 10167373 116.0 ns/op 56 B/op 3 allocs/op
BenchmarkInt64Slice/Any-8 4635265 264.1 ns/op 72 B/op 3 allocs/op
BenchmarkInt64Slice/Emit-8 3556449 347.7 ns/op 16 B/op 1 allocs/op
BenchmarkFloat64/Value-8 830878761 1.455 ns/op 0 B/op 0 allocs/op
BenchmarkFloat64/KeyValue-8 138633600 8.516 ns/op 0 B/op 0 allocs/op
BenchmarkFloat64/AsFloat64-8 795928496 1.501 ns/op 0 B/op 0 allocs/op
BenchmarkFloat64/Any-8 59382187 24.10 ns/op 0 B/op 0 allocs/op
BenchmarkFloat64/Emit-8 8588161 139.5 ns/op 16 B/op 2 allocs/op
BenchmarkFloat64/Array/KeyValue-8 5874248 207.6 ns/op 48 B/op 2 allocs/op
BenchmarkFloat64/Array/Emit-8 2531536 496.9 ns/op 16 B/op 1 allocs/op
BenchmarkFloat64Slice/Value-8 4871056 356.5 ns/op 72 B/op 3 allocs/op
BenchmarkFloat64Slice/KeyValue-8 4563218 253.2 ns/op 72 B/op 3 allocs/op
BenchmarkFloat64Slice/AsFloat64Slice-8 9809776 124.2 ns/op 56 B/op 3 allocs/op
BenchmarkFloat64Slice/Any-8 4445758 274.7 ns/op 72 B/op 3 allocs/op
BenchmarkFloat64Slice/Emit-8 2461478 499.1 ns/op 16 B/op 1 allocs/op
BenchmarkString/Value-8 177216351 6.725 ns/op 0 B/op 0 allocs/op
BenchmarkString/KeyValue-8 127416421 9.414 ns/op 0 B/op 0 allocs/op
BenchmarkString/AsString-8 648566374 2.079 ns/op 0 B/op 0 allocs/op
BenchmarkString/Any-8 52950224 23.96 ns/op 0 B/op 0 allocs/op
BenchmarkString/Emit-8 156119772 8.155 ns/op 0 B/op 0 allocs/op
BenchmarkString/Array/KeyValue-8 4489150 262.0 ns/op 96 B/op 2 allocs/op
BenchmarkString/Array/Emit-8 3600332 337.2 ns/op 48 B/op 1 allocs/op
BenchmarkStringSlice/Value-8 3690963 316.7 ns/op 120 B/op 3 allocs/op
BenchmarkStringSlice/KeyValue-8 3766921 333.4 ns/op 120 B/op 3 allocs/op
BenchmarkStringSlice/AsStringSlice-8 4997282 238.1 ns/op 112 B/op 3 allocs/op
BenchmarkStringSlice/Any-8 2928495 397.7 ns/op 120 B/op 3 allocs/op
BenchmarkStringSlice/Emit-8 3196767 416.4 ns/op 48 B/op 1 allocs/op
BenchmarkBytes/Any-8 56228438 20.27 ns/op 0 B/op 0 allocs/op
PASS
ok go.opentelemetry.io/otel/attribute 96.952s
go test -bench=. 101.63s user 1.59s system 106% cpu 1:37.29 total main for reference: $ go test -bench=.
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/attribute
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
BenchmarkBool/Value-8 836777030 1.362 ns/op 0 B/op 0 allocs/op
BenchmarkBool/KeyValue-8 142302812 8.453 ns/op 0 B/op 0 allocs/op
BenchmarkBool/AsBool-8 829789330 1.440 ns/op 0 B/op 0 allocs/op
BenchmarkBool/Any-8 57961297 20.32 ns/op 0 B/op 0 allocs/op
BenchmarkBool/Emit-8 151251918 8.039 ns/op 0 B/op 0 allocs/op
BenchmarkBool/Array/KeyValue-8 6854890 180.8 ns/op 6 B/op 2 allocs/op
BenchmarkBool/Array/Emit-8 4348897 277.3 ns/op 24 B/op 1 allocs/op
BenchmarkBoolSlice/Value-8 27577914 43.28 ns/op 27 B/op 2 allocs/op
BenchmarkBoolSlice/KeyValue-8 24781233 49.58 ns/op 27 B/op 2 allocs/op
BenchmarkBoolSlice/AsBoolSlice-8 518330706 2.273 ns/op 0 B/op 0 allocs/op
BenchmarkBoolSlice/Any-8 16825333 71.73 ns/op 27 B/op 2 allocs/op
BenchmarkBoolSlice/Emit-8 3429951 356.3 ns/op 27 B/op 4 allocs/op
BenchmarkInt/Value-8 872055820 1.415 ns/op 0 B/op 0 allocs/op
BenchmarkInt/KeyValue-8 142396792 8.417 ns/op 0 B/op 0 allocs/op
BenchmarkInt/Any-8 60824240 20.68 ns/op 0 B/op 0 allocs/op
BenchmarkInt/Emit-8 100000000 12.04 ns/op 0 B/op 0 allocs/op
BenchmarkInt/Array/KeyValue-8 4797216 215.7 ns/op 48 B/op 2 allocs/op
BenchmarkInt/Array/Emit-8 3560059 331.1 ns/op 16 B/op 1 allocs/op
BenchmarkIntSlice/Value-8 21344516 59.54 ns/op 48 B/op 2 allocs/op
BenchmarkIntSlice/KeyValue-8 17772919 66.70 ns/op 48 B/op 2 allocs/op
BenchmarkIntSlice/Any-8 14000749 87.23 ns/op 48 B/op 2 allocs/op
BenchmarkIntSlice/Emit-8 2634862 451.8 ns/op 40 B/op 4 allocs/op
BenchmarkInt8/Any-8 60368385 20.26 ns/op 0 B/op 0 allocs/op
BenchmarkInt16/Any-8 58757239 21.29 ns/op 0 B/op 0 allocs/op
BenchmarkInt32/Any-8 57048292 22.17 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/Value-8 880253342 1.412 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/KeyValue-8 142218999 8.432 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/AsInt64-8 802441760 1.531 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/Any-8 59352590 21.40 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/Emit-8 100000000 10.58 ns/op 0 B/op 0 allocs/op
BenchmarkInt64/Array/KeyValue-8 6246181 195.4 ns/op 48 B/op 2 allocs/op
BenchmarkInt64/Array/Emit-8 3535020 334.1 ns/op 16 B/op 1 allocs/op
BenchmarkInt64Slice/Value-8 23638441 50.27 ns/op 48 B/op 2 allocs/op
BenchmarkInt64Slice/KeyValue-8 20832636 59.44 ns/op 48 B/op 2 allocs/op
BenchmarkInt64Slice/AsInt64Slice-8 476564612 2.406 ns/op 0 B/op 0 allocs/op
BenchmarkInt64Slice/Any-8 15291912 76.81 ns/op 48 B/op 2 allocs/op
BenchmarkInt64Slice/Emit-8 2635155 450.9 ns/op 40 B/op 4 allocs/op
BenchmarkFloat64/Value-8 852685413 1.435 ns/op 0 B/op 0 allocs/op
BenchmarkFloat64/KeyValue-8 142022947 8.589 ns/op 0 B/op 0 allocs/op
BenchmarkFloat64/AsFloat64-8 827919326 1.511 ns/op 0 B/op 0 allocs/op
BenchmarkFloat64/Any-8 61388091 21.64 ns/op 0 B/op 0 allocs/op
BenchmarkFloat64/Emit-8 8640360 145.5 ns/op 16 B/op 2 allocs/op
BenchmarkFloat64/Array/KeyValue-8 5193076 207.1 ns/op 48 B/op 2 allocs/op
BenchmarkFloat64/Array/Emit-8 2415973 496.5 ns/op 16 B/op 1 allocs/op
BenchmarkFloat64Slice/Value-8 23691865 50.90 ns/op 48 B/op 2 allocs/op
BenchmarkFloat64Slice/KeyValue-8 20460295 57.43 ns/op 48 B/op 2 allocs/op
BenchmarkFloat64Slice/AsFloat64Slice-8 494869453 2.541 ns/op 0 B/op 0 allocs/op
BenchmarkFloat64Slice/Any-8 15183091 80.96 ns/op 48 B/op 2 allocs/op
BenchmarkFloat64Slice/Emit-8 1959252 626.9 ns/op 40 B/op 4 allocs/op
BenchmarkString/Value-8 182819140 6.571 ns/op 0 B/op 0 allocs/op
BenchmarkString/KeyValue-8 134711890 8.999 ns/op 0 B/op 0 allocs/op
BenchmarkString/AsString-8 609514870 2.034 ns/op 0 B/op 0 allocs/op
BenchmarkString/Any-8 54585226 23.48 ns/op 0 B/op 0 allocs/op
BenchmarkString/Emit-8 164563641 7.809 ns/op 0 B/op 0 allocs/op
BenchmarkString/Array/KeyValue-8 4976325 249.6 ns/op 96 B/op 2 allocs/op
BenchmarkString/Array/Emit-8 3592950 330.3 ns/op 48 B/op 1 allocs/op
BenchmarkStringSlice/Value-8 13330621 92.57 ns/op 72 B/op 2 allocs/op
BenchmarkStringSlice/KeyValue-8 12136434 99.54 ns/op 72 B/op 2 allocs/op
BenchmarkStringSlice/AsStringSlice-8 493171647 2.840 ns/op 0 B/op 0 allocs/op
BenchmarkStringSlice/Any-8 9076705 131.2 ns/op 72 B/op 2 allocs/op
BenchmarkStringSlice/Emit-8 2652626 455.4 ns/op 96 B/op 4 allocs/op
BenchmarkBytes/Any-8 65258142 20.59 ns/op 0 B/op 0 allocs/op
PASS
ok go.opentelemetry.io/otel/attribute 90.838s
go test -bench=. 96.13s user 1.49s system 107% cpu 1:31.18 total |
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
The allocations appear to be a shell game. The initial approach had one more allocation when creating and some number more for
|
Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Could we instead prevent whatever is doing so from using attribute.Value in a map key? |
I see that this is because an attribute.Distinct value is being used, which has a Resource value with a []string in it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don't think the performance of slice-valued attributes is important. The extra allocation cost may be exchanged for a smaller-size Value object.)
Signed-off-by: Anthony J Mirabella a9@aneurysm9.com
Fixes #2220