attribute: fix slice related function bug (#3252)

* attribute: fix slice related function bug

Signed-off-by: Ziqi Zhao <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Chester Cheung <[email protected]>

CHANGELOG.md

@@ -12,6 +12,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - `sdktrace.TraceProvider.Shutdown` and `sdktrace.TraceProvider.ForceFlush` to not return error when no processor register. (#3268)
 
+### Fixed
+
+- Slice attributes of `attribute` package are now comparable based on their value, not instance. (#3108 #3252)
+
 ## [1.11.0/0.32.3] 2022-10-12
 
 ### Added

attribute/value.go

@@ -17,9 +17,11 @@ package attribute // import "go.opentelemetry.io/otel/attribute"
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"strconv"
 
 	"go.opentelemetry.io/otel/internal"
+	"go.opentelemetry.io/otel/internal/attribute"
 )
 
 //go:generate stringer -type=Type
@@ -66,12 +68,7 @@ func BoolValue(v bool) Value {
 
 // BoolSliceValue creates a BOOLSLICE Value.
 func BoolSliceValue(v []bool) Value {
-	cp := make([]bool, len(v))
-	copy(cp, v)
-	return Value{
-		vtype: BOOLSLICE,
-		slice: &cp,
-	}
+	return Value{vtype: BOOLSLICE, slice: attribute.SliceValue(v)}
 }
 
 // IntValue creates an INT64 Value.
@@ -81,13 +78,14 @@ func IntValue(v int) Value {
 
 // IntSliceValue creates an INTSLICE Value.
 func IntSliceValue(v []int) Value {
-	cp := make([]int64, 0, len(v))
-	for _, i := range v {
-		cp = append(cp, int64(i))
+	var int64Val int64
+	cp := reflect.New(reflect.ArrayOf(len(v), reflect.TypeOf(int64Val)))
+	for i, val := range v {
+		cp.Elem().Index(i).SetInt(int64(val))
 	}
 	return Value{
 		vtype: INT64SLICE,
-		slice: &cp,
+		slice: cp.Elem().Interface(),
 	}
 }
 
@@ -101,12 +99,7 @@ func Int64Value(v int64) Value {
 
 // Int64SliceValue creates an INT64SLICE Value.
 func Int64SliceValue(v []int64) Value {
-	cp := make([]int64, len(v))
-	copy(cp, v)
-	return Value{
-		vtype: INT64SLICE,
-		slice: &cp,
-	}
+	return Value{vtype: INT64SLICE, slice: attribute.SliceValue(v)}
 }
 
 // Float64Value creates a FLOAT64 Value.
@@ -119,12 +112,7 @@ func Float64Value(v float64) Value {
 
 // Float64SliceValue creates a FLOAT64SLICE Value.
 func Float64SliceValue(v []float64) Value {
-	cp := make([]float64, len(v))
-	copy(cp, v)
-	return Value{
-		vtype: FLOAT64SLICE,
-		slice: &cp,
-	}
+	return Value{vtype: FLOAT64SLICE, slice: attribute.SliceValue(v)}
 }
 
 // StringValue creates a STRING Value.
@@ -137,12 +125,7 @@ func StringValue(v string) Value {
 
 // StringSliceValue creates a STRINGSLICE Value.
 func StringSliceValue(v []string) Value {
-	cp := make([]string, len(v))
-	copy(cp, v)
-	return Value{
-		vtype: STRINGSLICE,
-		slice: &cp,
-	}
+	return Value{vtype: STRINGSLICE, slice: attribute.SliceValue(v)}
 }
 
 // Type returns a type of the Value.
@@ -159,10 +142,7 @@ func (v Value) AsBool() bool {
 // AsBoolSlice returns the []bool value. Make sure that the Value's type is
 // BOOLSLICE.
 func (v Value) AsBoolSlice() []bool {
-	if s, ok := v.slice.(*[]bool); ok {
-		return *s
-	}
-	return nil
+	return attribute.AsSlice[bool](v.slice)
 }
 
 // AsInt64 returns the int64 value. Make sure that the Value's type is
@@ -174,10 +154,7 @@ func (v Value) AsInt64() int64 {
 // AsInt64Slice returns the []int64 value. Make sure that the Value's type is
 // INT64SLICE.
 func (v Value) AsInt64Slice() []int64 {
-	if s, ok := v.slice.(*[]int64); ok {
-		return *s
-	}
-	return nil
+	return attribute.AsSlice[int64](v.slice)
 }
 
 // AsFloat64 returns the float64 value. Make sure that the Value's
@@ -189,10 +166,7 @@ func (v Value) AsFloat64() float64 {
 // AsFloat64Slice returns the []float64 value. Make sure that the Value's type is
 // FLOAT64SLICE.
 func (v Value) AsFloat64Slice() []float64 {
-	if s, ok := v.slice.(*[]float64); ok {
-		return *s
-	}
-	return nil
+	return attribute.AsSlice[float64](v.slice)
 }
 
 // AsString returns the string value. Make sure that the Value's type
@@ -204,10 +178,7 @@ func (v Value) AsString() string {
 // AsStringSlice returns the []string value. Make sure that the Value's type is
 // STRINGSLICE.
 func (v Value) AsStringSlice() []string {
-	if s, ok := v.slice.(*[]string); ok {
-		return *s
-	}
-	return nil
+	return attribute.AsSlice[string](v.slice)
 }
 
 type unknownValueType struct{}
@@ -239,19 +210,19 @@ func (v Value) AsInterface() interface{} {
 func (v Value) Emit() string {
 	switch v.Type() {
 	case BOOLSLICE:
-		return fmt.Sprint(*(v.slice.(*[]bool)))
+		return fmt.Sprint(v.AsBoolSlice())
 	case BOOL:
 		return strconv.FormatBool(v.AsBool())
 	case INT64SLICE:
-		return fmt.Sprint(*(v.slice.(*[]int64)))
+		return fmt.Sprint(v.AsInt64Slice())
 	case INT64:
 		return strconv.FormatInt(v.AsInt64(), 10)
 	case FLOAT64SLICE:
-		return fmt.Sprint(*(v.slice.(*[]float64)))
+		return fmt.Sprint(v.AsFloat64Slice())
 	case FLOAT64:
 		return fmt.Sprint(v.AsFloat64())
 	case STRINGSLICE:
-		return fmt.Sprint(*(v.slice.(*[]string)))
+		return fmt.Sprint(v.AsStringSlice())
 	case STRING:
 		return v.stringly
 	default:

attribute/value_test.go

@@ -18,6 +18,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
 
 	"go.opentelemetry.io/otel/attribute"
 )
@@ -104,3 +105,82 @@ func TestValue(t *testing.T) {
 		}
 	}
 }
+
+func TestSetComparability(t *testing.T) {
+	pairs := [][2]attribute.KeyValue{
+		{
+			attribute.Bool("Bool", true),
+			attribute.Bool("Bool", true),
+		},
+		{
+			attribute.BoolSlice("BoolSlice", []bool{true, false, true}),
+			attribute.BoolSlice("BoolSlice", []bool{true, false, true}),
+		},
+		{
+			attribute.Int("Int", 34),
+			attribute.Int("Int", 34),
+		},
+		{
+			attribute.IntSlice("IntSlice", []int{312, 1, -2}),
+			attribute.IntSlice("IntSlice", []int{312, 1, -2}),
+		},
+		{
+			attribute.Int64("Int64", 98),
+			attribute.Int64("Int64", 98),
+		},
+		{
+			attribute.Int64Slice("Int64Slice", []int64{12, 1298, -219, 2}),
+			attribute.Int64Slice("Int64Slice", []int64{12, 1298, -219, 2}),
+		},
+		{
+			attribute.Float64("Float64", 19.09),
+			attribute.Float64("Float64", 19.09),
+		},
+		{
+			attribute.Float64Slice("Float64Slice", []float64{12398.1, -37.1713873737, 3}),
+			attribute.Float64Slice("Float64Slice", []float64{12398.1, -37.1713873737, 3}),
+		},
+		{
+			attribute.String("String", "string value"),
+			attribute.String("String", "string value"),
+		},
+		{
+			attribute.StringSlice("StringSlice", []string{"one", "two", "three"}),
+			attribute.StringSlice("StringSlice", []string{"one", "two", "three"}),
+		},
+	}
+
+	for _, p := range pairs {
+		s0, s1 := attribute.NewSet(p[0]), attribute.NewSet(p[1])
+		m := map[attribute.Set]struct{}{s0: {}}
+		_, ok := m[s1]
+		assert.Truef(t, ok, "%s not comparable", p[0].Value.Type())
+	}
+}
+
+func TestAsSlice(t *testing.T) {
+	bs1 := []bool{true, false, true}
+	kv := attribute.BoolSlice("BoolSlice", bs1)
+	bs2 := kv.Value.AsBoolSlice()
+	assert.Equal(t, bs1, bs2)
+
+	i64s1 := []int64{12, 1298, -219, 2}
+	kv = attribute.Int64Slice("Int64Slice", i64s1)
+	i64s2 := kv.Value.AsInt64Slice()
+	assert.Equal(t, i64s1, i64s2)
+
+	is1 := []int{12, 1298, -219, 2}
+	kv = attribute.IntSlice("IntSlice", is1)
+	i64s2 = kv.Value.AsInt64Slice()
+	assert.Equal(t, i64s1, i64s2)
+
+	fs1 := []float64{12398.1, -37.1713873737, 3}
+	kv = attribute.Float64Slice("Float64Slice", fs1)
+	fs2 := kv.Value.AsFloat64Slice()
+	assert.Equal(t, fs1, fs2)
+
+	ss1 := []string{"one", "two", "three"}
+	kv = attribute.StringSlice("StringSlice", ss1)
+	ss2 := kv.Value.AsStringSlice()
+	assert.Equal(t, ss1, ss2)
+}

internal/attribute/attribute.go

@@ -0,0 +1,45 @@
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/*
+Package attribute provide several helper functions for some commonly used
+logic of processing attributes.
+*/
+package attribute // import "go.opentelemetry.io/otel/internal/attribute"
+
+import (
+	"reflect"
+)
+
+// SliceValue convert a slice into an array with same elements as slice.
+func SliceValue[T bool | int64 | float64 | string](v []T) any {
+	var zero T
+	cp := reflect.New(reflect.ArrayOf(len(v), reflect.TypeOf(zero)))
+	copy(cp.Elem().Slice(0, len(v)).Interface().([]T), v)
+	return cp.Elem().Interface()
+}
+
+// AsSlice convert an array into a slice into with same elements as array.
+func AsSlice[T bool | int64 | float64 | string](v any) []T {
+	rv := reflect.ValueOf(v)
+	if rv.Type().Kind() != reflect.Array {
+		return nil
+	}
+	var zero T
+	correctLen := rv.Len()
+	correctType := reflect.ArrayOf(correctLen, reflect.TypeOf(zero))
+	cpy := reflect.New(correctType)
+	_ = reflect.Copy(cpy.Elem(), rv)
+	return cpy.Elem().Slice(0, correctLen).Interface().([]T)
+}

sdk/trace/span.go

@@ -313,26 +313,13 @@ func truncateAttr(limit int, attr attribute.KeyValue) attribute.KeyValue {
 			return attr.Key.String(safeTruncate(v, limit))
 		}
 	case attribute.STRINGSLICE:
-		// Do no mutate the original, make a copy.
-		trucated := attr.Key.StringSlice(attr.Value.AsStringSlice())
-		// Do not do this.
-		//
-		//   v := trucated.Value.AsStringSlice()
-		//   cp := make([]string, len(v))
-		//   /* Copy and truncate values to cp ... */
-		//   trucated.Value = attribute.StringSliceValue(cp)
-		//
-		// Copying the []string and then assigning it back as a new value with
-		// attribute.StringSliceValue will copy the data twice. Instead, we
-		// already made a copy above that only this function owns, update the
-		// underlying slice data of our copy.
-		v := trucated.Value.AsStringSlice()
+		v := attr.Value.AsStringSlice()
 		for i := range v {
 			if len(v[i]) > limit {
 				v[i] = safeTruncate(v[i], limit)
 			}
 		}
-		return trucated
+		return attr.Key.StringSlice(v)
 	}
 	return attr
 }