The attribute.Set map key values are not always comparisons of value #3702
Comments
|
A proposal from today's SIG meeting is to look into having a set return a "hash". That hash could generated by value and be used as a map key. Also, our attributes should be converting the slice types to arrays which should compare values. However, it is storing them all as an interface which is likely the reason the reference is compared. opentelemetry-go/attribute/value.go Line 37 in e9bdda0 An investigation into possibly storing this differently might help resolve the issue. |
|
PoC hashing function for attribute sets using built-in import (
"hash/fnv"
"math"
"go.opentelemetry.io/otel/attribute"
)
func hash(s attribute.Set) uint64 {
h := fnv.New64a()
iter := s.Iter()
for iter.Next() {
a := iter.Attribute()
_, _ = h.Write([]byte(a.Key))
val := a.Value
switch val.Type() {
case attribute.BOOL:
b := boolBytes(val.AsBool())
_, _ = h.Write(b[:])
case attribute.BOOLSLICE:
for _, v := range val.AsBoolSlice() {
b := boolBytes(v)
_, _ = h.Write(b[:])
}
case attribute.INT64:
b := intBytes(val.AsInt64())
_, _ = h.Write(b[:])
case attribute.INT64SLICE:
for _, v := range val.AsInt64Slice() {
b := intBytes(v)
_, _ = h.Write(b[:])
}
case attribute.FLOAT64:
b := float64Bytes(val.AsFloat64())
_, _ = h.Write(b[:])
case attribute.FLOAT64SLICE:
for _, v := range val.AsFloat64Slice() {
b := float64Bytes(v)
_, _ = h.Write(b[:])
}
case attribute.STRING:
_, _ = h.Write([]byte(val.AsString()))
case attribute.STRINGSLICE:
for _, v := range val.AsStringSlice() {
_, _ = h.Write([]byte(v))
}
}
}
return h.Sum64()
}
func boolBytes(val bool) [1]byte {
if val {
return [1]byte{1}
}
return [1]byte{0}
}
func intBytes[N int64 | uint64](val N) [8]byte {
// Used for hashing, endianness doesn't matter.
return [8]byte{
byte(0xff & val),
byte(0xff & (val >> 8)),
byte(0xff & (val >> 16)),
byte(0xff & (val >> 24)),
byte(0xff & (val >> 32)),
byte(0xff & (val >> 40)),
byte(0xff & (val >> 48)),
byte(0xff & (val >> 56)),
}
}
func float64Bytes(val float64) [8]byte {
return intBytes(math.Float64bits(val))
}tests/benchmarks: import (
"testing"
"github.com/stretchr/testify/assert"
"go.opentelemetry.io/otel/attribute"
)
var tests = []struct {
name string
attr attribute.Set
attrPrime attribute.Set
attrAltKey attribute.Set
attrAltVal attribute.Set
}{
{
name: "Bool",
attr: attribute.NewSet(attribute.Bool("k", true)),
attrPrime: attribute.NewSet(attribute.Bool("k", true)),
attrAltKey: attribute.NewSet(attribute.Bool("j", true)),
attrAltVal: attribute.NewSet(attribute.Bool("k", false)),
},
{
name: "BoolSlice",
attr: attribute.NewSet(attribute.BoolSlice("k", []bool{true, false, true})),
attrPrime: attribute.NewSet(attribute.BoolSlice("k", []bool{true, false, true})),
attrAltKey: attribute.NewSet(attribute.BoolSlice("j", []bool{true, false, true})),
attrAltVal: attribute.NewSet(attribute.BoolSlice("k", []bool{false, true, true})),
},
{
name: "Int",
attr: attribute.NewSet(attribute.Int("k", 1)),
attrPrime: attribute.NewSet(attribute.Int("k", 1)),
attrAltKey: attribute.NewSet(attribute.Int("j", 1)),
attrAltVal: attribute.NewSet(attribute.Int("k", -1)),
},
{
name: "IntSlice",
attr: attribute.NewSet(attribute.IntSlice("k", []int{-3, 0, 1})),
attrPrime: attribute.NewSet(attribute.IntSlice("k", []int{-3, 0, 1})),
attrAltKey: attribute.NewSet(attribute.IntSlice("j", []int{-3, 0, 1})),
attrAltVal: attribute.NewSet(attribute.IntSlice("k", []int{-4, 0, 2})),
},
{
name: "Int64",
attr: attribute.NewSet(attribute.Int64("k", -1)),
attrPrime: attribute.NewSet(attribute.Int64("k", -1)),
attrAltKey: attribute.NewSet(attribute.Int64("j", -1)),
attrAltVal: attribute.NewSet(attribute.Int64("k", 1)),
},
{
name: "Int64Slice",
attr: attribute.NewSet(attribute.Int64Slice("k", []int64{-4, 0, 2})),
attrPrime: attribute.NewSet(attribute.Int64Slice("k", []int64{-4, 0, 2})),
attrAltKey: attribute.NewSet(attribute.Int64Slice("j", []int64{-4, 0, 2})),
attrAltVal: attribute.NewSet(attribute.Int64Slice("k", []int64{-3, 0, 1})),
},
{
name: "Float64",
attr: attribute.NewSet(attribute.Float64("k", 2.3)),
attrPrime: attribute.NewSet(attribute.Float64("k", 2.3)),
attrAltKey: attribute.NewSet(attribute.Float64("j", 2.3)),
attrAltVal: attribute.NewSet(attribute.Float64("k", -1e3)),
},
{
name: "Float64Slice",
attr: attribute.NewSet(attribute.Float64Slice("k", []float64{-1.2, 0.32, 1e9})),
attrPrime: attribute.NewSet(attribute.Float64Slice("k", []float64{-1.2, 0.32, 1e9})),
attrAltKey: attribute.NewSet(attribute.Float64Slice("j", []float64{-1.2, 0.32, 1e9})),
attrAltVal: attribute.NewSet(attribute.Float64Slice("k", []float64{-1.2, 1e9, 0.32})),
},
{
name: "String",
attr: attribute.NewSet(attribute.String("k", "val")),
attrPrime: attribute.NewSet(attribute.String("k", "val")),
attrAltKey: attribute.NewSet(attribute.String("j", "val")),
attrAltVal: attribute.NewSet(attribute.String("k", "alt")),
},
{
name: "StringSlice",
attr: attribute.NewSet(attribute.StringSlice("k", []string{"zero", "one", ""})),
attrPrime: attribute.NewSet(attribute.StringSlice("k", []string{"zero", "one", ""})),
attrAltKey: attribute.NewSet(attribute.StringSlice("j", []string{"zero", "one", ""})),
attrAltVal: attribute.NewSet(attribute.StringSlice("k", []string{"", "one", "zero"})),
},
{
name: "All",
attr: attribute.NewSet(
attribute.Bool("k", true),
attribute.BoolSlice("k", []bool{true, false, true}),
attribute.Int("k", 1),
attribute.IntSlice("k", []int{-3, 0, 1}),
attribute.Int64("k", -1),
attribute.Int64Slice("k", []int64{-4, 0, 2}),
attribute.Float64("k", 2.3),
attribute.Float64Slice("k", []float64{-1.2, 0.32, 1e9}),
attribute.String("k", "val"),
attribute.StringSlice("k", []string{"zero", "one", ""}),
),
attrPrime: attribute.NewSet(
attribute.Bool("k", true),
attribute.BoolSlice("k", []bool{true, false, true}),
attribute.Int("k", 1),
attribute.IntSlice("k", []int{-3, 0, 1}),
attribute.Int64("k", -1),
attribute.Int64Slice("k", []int64{-4, 0, 2}),
attribute.Float64("k", 2.3),
attribute.Float64Slice("k", []float64{-1.2, 0.32, 1e9}),
attribute.String("k", "val"),
attribute.StringSlice("k", []string{"zero", "one", ""}),
),
attrAltKey: attribute.NewSet(
attribute.Bool("j", true),
attribute.BoolSlice("j", []bool{true, false, true}),
attribute.Int("j", 1),
attribute.IntSlice("j", []int{-3, 0, 1}),
attribute.Int64("j", -1),
attribute.Int64Slice("j", []int64{-4, 0, 2}),
attribute.Float64("j", 2.3),
attribute.Float64Slice("j", []float64{-1.2, 0.32, 1e9}),
attribute.String("j", "val"),
attribute.StringSlice("j", []string{"zero", "one", ""}),
),
attrAltVal: attribute.NewSet(
attribute.Bool("k", false),
attribute.BoolSlice("k", []bool{false, true, true}),
attribute.Int("k", -1),
attribute.IntSlice("k", []int{-4, 0, 2}),
attribute.Int64("k", 1),
attribute.Int64Slice("k", []int64{-3, 0, 1}),
attribute.Float64("k", -1e3),
attribute.Float64Slice("k", []float64{-1.2, 1e9, 0.32}),
attribute.String("k", "alt"),
attribute.StringSlice("k", []string{"", "one", "zero"}),
),
},
}
func TestHash(t *testing.T) {
for _, test := range tests {
t.Run(test.name, testHash(test.attr, test.attrPrime, test.attrAltKey, test.attrAltVal))
}
}
func testHash(a, ap, ak, av attribute.Set) func(*testing.T) {
return func(t *testing.T) {
h := hash(a)
assert.Equal(t, h, hash(ap), "same keys/values")
assert.NotEqual(t, h, hash(ak), "different keys")
assert.NotEqual(t, h, hash(av), "different values")
}
}
var benchmarks = []struct {
name string
attr attribute.Set
}{
{name: "Bool", attr: attribute.NewSet(attribute.Bool("k", true))},
{name: "BoolSlice", attr: attribute.NewSet(attribute.BoolSlice("k", []bool{true, false, true}))},
{name: "Int", attr: attribute.NewSet(attribute.Int("k", 1))},
{name: "IntSlice", attr: attribute.NewSet(attribute.IntSlice("k", []int{-3, 0, 1}))},
{name: "Int64", attr: attribute.NewSet(attribute.Int64("k", -1))},
{name: "Int64Slice", attr: attribute.NewSet(attribute.Int64Slice("k", []int64{-4, 0, 2}))},
{name: "Float64", attr: attribute.NewSet(attribute.Float64("k", 2.3))},
{name: "Float64Slice", attr: attribute.NewSet(attribute.Float64Slice("k", []float64{-1.2, 0.32, 1e9}))},
{name: "String", attr: attribute.NewSet(attribute.String("k", "val"))},
{name: "StringSlice", attr: attribute.NewSet(attribute.StringSlice("k", []string{"zero", "one", ""}))},
{
name: "All",
attr: attribute.NewSet(
attribute.Bool("k", true),
attribute.BoolSlice("k", []bool{true, false, true}),
attribute.Int("k", 1),
attribute.IntSlice("k", []int{-3, 0, 1}),
attribute.Int64("k", -1),
attribute.Int64Slice("k", []int64{-4, 0, 2}),
attribute.Float64("k", 2.3),
attribute.Float64Slice("k", []float64{-1.2, 0.32, 1e9}),
attribute.String("k", "val"),
attribute.StringSlice("k", []string{"zero", "one", ""}),
),
},
}
func BenchmarkHash(b *testing.B) {
for _, bench := range benchmarks {
b.Run(bench.name, benchHash(bench.attr))
}
}
var hashResult uint64
func benchHash(s attribute.Set) func(*testing.B) {
return func(b *testing.B) {
b.ReportAllocs()
for n := 0; n < b.N; n++ {
hashResult = hash(s)
}
}
} |
|
If the underlying func hash(s Set) uint64 {
h := fnv.New64a()
iter := s.Iter()
for iter.Next() {
a := iter.Attribute()
_, _ = h.Write([]byte(a.Key))
val := a.Value
switch val.Type() {
case BOOL:
b := boolBytes(val.AsBool())
_, _ = h.Write(b[:])
case BOOLSLICE:
rv := reflect.ValueOf(val.slice)
if rv.Type().Kind() != reflect.Array {
// FIXME: continue and skip
return 0
}
for i := 0; i < rv.Len(); i++ {
v := rv.Index(i)
if v.Type().Kind() != reflect.Bool {
// Should never happen.
continue
}
b := boolBytes(v.Bool())
_, _ = h.Write(b[:])
}
case INT64:
b := intBytes(val.AsInt64())
_, _ = h.Write(b[:])
case INT64SLICE:
rv := reflect.ValueOf(val.slice)
if rv.Type().Kind() != reflect.Array {
// FIXME: continue and skip
return 0
}
for i := 0; i < rv.Len(); i++ {
v := rv.Index(i)
if v.Type().Kind() != reflect.Int64 {
// Should never happen.
continue
}
b := intBytes(v.Int())
_, _ = h.Write(b[:])
}
case FLOAT64:
b := float64Bytes(val.AsFloat64())
_, _ = h.Write(b[:])
case FLOAT64SLICE:
rv := reflect.ValueOf(val.slice)
if rv.Type().Kind() != reflect.Array {
// FIXME: continue and skip
return 0
}
for i := 0; i < rv.Len(); i++ {
v := rv.Index(i)
if v.Type().Kind() != reflect.Float64 {
// Should never happen.
continue
}
b := float64Bytes(v.Float())
_, _ = h.Write(b[:])
}
case STRING:
_, _ = h.Write([]byte(val.AsString()))
case STRINGSLICE:
rv := reflect.ValueOf(val.slice)
if rv.Type().Kind() != reflect.Array {
// FIXME: continue and skip
return 0
}
for i := 0; i < rv.Len(); i++ {
v := rv.Index(i)
if v.Type().Kind() != reflect.String {
// Should never happen.
continue
}
_, _ = h.Write([]byte(v.String()))
}
}
}
return h.Sum64()
} |
Adding a |
Looking into updating the |
|
Testing against the latest version of otel resolves the test failures. |
|
This looks to have been addressed in #3252 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
The
attribute.Settype is used as a key for manymaps in the metric SDK.opentelemetry-go/sdk/metric/internal/lastvalue.go
Line 35 in e9bdda0
opentelemetry-go/sdk/metric/internal/sum.go
Line 28 in e9bdda0
opentelemetry-go/sdk/metric/internal/sum.go
Line 174 in e9bdda0
opentelemetry-go/sdk/metric/internal/sum.go
Line 241 in e9bdda0
opentelemetry-go/sdk/metric/internal/histogram.go
Line 55 in e9bdda0
opentelemetry-go/sdk/metric/internal/aggregator_test.go
Line 56 in e9bdda0
opentelemetry-go/sdk/metric/internal/filter.go
Line 49 in e9bdda0
opentelemetry-go/sdk/metric/internal/filter.go
Line 95 in e9bdda0
opentelemetry-go/sdk/metric/meter_test.go
Line 1270 in e9bdda0
The
attribute.Setis syntactically comparable, but for sets that contain slice values it does not compare the underlying value.Steps To Reproduce
go.mod:Expected behavior
The provided test should pass.
The text was updated successfully, but these errors were encountered: