From 5660b0b59f4c0fbb9e2879bb645b632b50f5f3bd Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 5 Oct 2020 17:09:03 -0700 Subject: [PATCH] Update label.ArrayValue to store copies of 1D arrays (#1226) * Update label.ArrayValue to store copies of 1D arrays * Update OTLP transform of array attributes * Add changes to CHANGELOG * Add PR number to changes * Update value function documentation * Remove redundant checks * Add Array test for invalid array types * Add test to ensure return from AsArray is a type * Clean up iteration --- CHANGELOG.md | 5 + .../otlp/internal/transform/attribute.go | 208 +++++------------- .../otlp/internal/transform/attribute_test.go | 34 ++- label/value.go | 52 +++-- label/value_test.go | 38 ++-- 5 files changed, 137 insertions(+), 200 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 950e2c03647a..95efab780754 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The `StatusCode` field of the `SpanData` struct in the `go.opentelemetry.io/otel/sdk/export/trace` package now uses the codes package from this package instead of the gRPC project. (#1214) - Move the `go.opentelemetry.io/otel/api/baggage` package into `go.opentelemetry.io/otel/propagators`. (#1217) +### Fixed + +- Copies of data from arrays and slices passed to `go.opentelemetry.io/otel/label.ArrayValue()` are now used in the returned `Value` instead of using the mutable data itself. (#1226) + ### Removed - The `ExtractHTTP` and `InjectHTTP` fuctions from the `go.opentelemetry.io/otel/api/propagation` package were removed. (#1212) @@ -33,6 +37,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The `SetAttribute` method of the `Span` from the `go.opentelemetry.io/otel/api/trace` package was removed given its redundancy with the `SetAttributes` method. (#1216) - The internal implementation of Baggage storage is removed in favor of using the new Baggage API functionality. (#1217) - Remove duplicate hostname key `HostHostNameKey` in Resource semantic conventions. (#1219) +- Nested array/slice support has been removed. (#1226) ## [0.12.0] - 2020-09-24 diff --git a/exporters/otlp/internal/transform/attribute.go b/exporters/otlp/internal/transform/attribute.go index b50ff25c0716..99a91a49d616 100644 --- a/exporters/otlp/internal/transform/attribute.go +++ b/exporters/otlp/internal/transform/attribute.go @@ -15,6 +15,8 @@ package transform import ( + "reflect" + commonpb "go.opentelemetry.io/otel/exporters/otlp/internal/opentelemetry-proto-gen/common/v1" "go.opentelemetry.io/otel/label" @@ -75,7 +77,11 @@ func toAttribute(v label.KeyValue) *commonpb.KeyValue { StringValue: v.Value.AsString(), } case label.ARRAY: - result.Value.Value = toArrayAttribute(v) + result.Value.Value = &commonpb.AnyValue_ArrayValue{ + ArrayValue: &commonpb.ArrayValue{ + Values: arrayValues(v), + }, + } default: result.Value.Value = &commonpb.AnyValue_StringValue{ StringValue: "INVALID", @@ -84,164 +90,56 @@ func toAttribute(v label.KeyValue) *commonpb.KeyValue { return result } -func toArrayAttribute(v label.KeyValue) *commonpb.AnyValue_ArrayValue { - array := v.Value.AsArray() - var resultValues []*commonpb.AnyValue - - switch typedArray := array.(type) { - case []bool: - resultValues = getValuesFromBoolArray(typedArray) - case []int: - resultValues = getValuesFromIntArray(typedArray) - case []int32: - resultValues = getValuesFromInt32Array(typedArray) - case []int64: - resultValues = getValuesFromInt64Array(typedArray) - case []uint: - resultValues = getValuesFromUIntArray(typedArray) - case []uint32: - resultValues = getValuesFromUInt32Array(typedArray) - case []uint64: - resultValues = getValuesFromUInt64Array(typedArray) - case []float32: - resultValues = getValuesFromFloat32Array(typedArray) - case []float64: - resultValues = getValuesFromFloat64Array(typedArray) - case []string: - resultValues = getValuesFromStringArray(typedArray) - default: - resultValues = []*commonpb.AnyValue{ - { +func arrayValues(kv label.KeyValue) []*commonpb.AnyValue { + a := kv.Value.AsArray() + aType := reflect.TypeOf(a) + var valueFunc func(reflect.Value) *commonpb.AnyValue + switch aType.Elem().Kind() { + case reflect.Bool: + valueFunc = func(v reflect.Value) *commonpb.AnyValue { + return &commonpb.AnyValue{ + Value: &commonpb.AnyValue_BoolValue{ + BoolValue: v.Bool(), + }, + } + } + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + valueFunc = func(v reflect.Value) *commonpb.AnyValue { + return &commonpb.AnyValue{ + Value: &commonpb.AnyValue_IntValue{ + IntValue: v.Int(), + }, + } + } + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: + valueFunc = func(v reflect.Value) *commonpb.AnyValue { + return &commonpb.AnyValue{ + Value: &commonpb.AnyValue_IntValue{ + IntValue: int64(v.Uint()), + }, + } + } + case reflect.Float32, reflect.Float64: + valueFunc = func(v reflect.Value) *commonpb.AnyValue { + return &commonpb.AnyValue{ + Value: &commonpb.AnyValue_DoubleValue{ + DoubleValue: v.Float(), + }, + } + } + case reflect.String: + valueFunc = func(v reflect.Value) *commonpb.AnyValue { + return &commonpb.AnyValue{ Value: &commonpb.AnyValue_StringValue{ - StringValue: "INVALID", + StringValue: v.String(), }, - }, + } } } - return &commonpb.AnyValue_ArrayValue{ - ArrayValue: &commonpb.ArrayValue{ - Values: resultValues, - }, - } -} - -func getValuesFromBoolArray(boolArray []bool) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, b := range boolArray { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_BoolValue{ - BoolValue: b, - }, - }) - } - return result -} - -func getValuesFromIntArray(intArray []int) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, i := range intArray { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_IntValue{ - IntValue: int64(i), - }, - }) - } - return result -} - -func getValuesFromInt32Array(int32Array []int32) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, i := range int32Array { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_IntValue{ - IntValue: int64(i), - }, - }) - } - return result -} - -func getValuesFromInt64Array(int64Array []int64) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, i := range int64Array { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_IntValue{ - IntValue: i, - }, - }) - } - return result -} - -func getValuesFromUIntArray(uintArray []uint) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, i := range uintArray { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_IntValue{ - IntValue: int64(i), - }, - }) - } - return result -} - -func getValuesFromUInt32Array(uint32Array []uint32) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, i := range uint32Array { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_IntValue{ - IntValue: int64(i), - }, - }) - } - return result -} - -func getValuesFromUInt64Array(uint64Array []uint64) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, i := range uint64Array { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_IntValue{ - IntValue: int64(i), - }, - }) - } - return result -} - -func getValuesFromFloat32Array(float32Array []float32) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, f := range float32Array { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_DoubleValue{ - DoubleValue: float64(f), - }, - }) - } - return result -} - -func getValuesFromFloat64Array(float64Array []float64) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, f := range float64Array { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_DoubleValue{ - DoubleValue: f, - }, - }) - } - return result -} - -func getValuesFromStringArray(stringArray []string) []*commonpb.AnyValue { - result := []*commonpb.AnyValue{} - for _, s := range stringArray { - result = append(result, &commonpb.AnyValue{ - Value: &commonpb.AnyValue_StringValue{ - StringValue: s, - }, - }) + results := make([]*commonpb.AnyValue, aType.Len()) + for i, aValue := 0, reflect.ValueOf(a); i < aValue.Len(); i++ { + results[i] = valueFunc(aValue.Index(i)) } - return result + return results } diff --git a/exporters/otlp/internal/transform/attribute_test.go b/exporters/otlp/internal/transform/attribute_test.go index 7cb2a56d4308..68ae923baf2b 100644 --- a/exporters/otlp/internal/transform/attribute_test.go +++ b/exporters/otlp/internal/transform/attribute_test.go @@ -156,6 +156,21 @@ func TestArrayAttributes(t *testing.T) { // "uint", "uint32", "uint64" for _, test := range []attributeTest{ {nil, nil}, + { + []label.KeyValue{ + label.Array("invalid", [][]string{{"1", "2"}, {"a"}}), + }, + []*commonpb.KeyValue{ + { + Key: "invalid", + Value: &commonpb.AnyValue{ + Value: &commonpb.AnyValue_StringValue{ + StringValue: "INVALID", + }, + }, + }, + }, + }, { []label.KeyValue{ label.Array("bool array to bool array", []bool{true, false}), @@ -191,17 +206,20 @@ func TestArrayAttributes(t *testing.T) { for i, actualArrayAttr := range actualArrayAttributes { expectedArrayAttr := expectedArrayAttributes[i] - if !assert.Equal(t, expectedArrayAttr.Key, actualArrayAttr.Key) { + expectedKey, actualKey := expectedArrayAttr.Key, actualArrayAttr.Key + if !assert.Equal(t, expectedKey, actualKey) { continue } - expectedArrayValue := expectedArrayAttr.Value.GetArrayValue() - assert.NotNil(t, expectedArrayValue) - - actualArrayValue := actualArrayAttr.Value.GetArrayValue() - assert.NotNil(t, actualArrayValue) - - assertExpectedArrayValues(t, expectedArrayValue.Values, actualArrayValue.Values) + expected := expectedArrayAttr.Value.GetArrayValue() + actual := actualArrayAttr.Value.GetArrayValue() + if expected == nil { + assert.Nil(t, actual) + continue + } + if assert.NotNil(t, actual, "expected not nil for %s", actualKey) { + assertExpectedArrayValues(t, expected.Values, actual.Values) + } } } diff --git a/label/value.go b/label/value.go index 633979f1b381..679009b1a737 100644 --- a/label/value.go +++ b/label/value.go @@ -19,7 +19,6 @@ import ( "fmt" "reflect" "strconv" - "strings" "unsafe" "go.opentelemetry.io/otel/internal" @@ -93,7 +92,7 @@ func Int32Value(v int32) Value { } } -// Uint32 creates a UINT32 Value. +// Uint32Value creates a UINT32 Value. func Uint32Value(v uint32) Value { return Value{ vtype: UINT32, @@ -101,7 +100,7 @@ func Uint32Value(v uint32) Value { } } -// Float32 creates a FLOAT32 Value. +// Float32Value creates a FLOAT32 Value. func Float32Value(v float32) Value { return Value{ vtype: FLOAT32, @@ -109,7 +108,7 @@ func Float32Value(v float32) Value { } } -// String creates a STRING Value. +// StringValue creates a STRING Value. func StringValue(v string) Value { return Value{ vtype: STRING, @@ -117,7 +116,7 @@ func StringValue(v string) Value { } } -// Int creates either an INT32 or an INT64 Value, depending on whether +// IntValue creates either an INT32 or an INT64 Value, depending on whether // the int type is 32 or 64 bits wide. func IntValue(v int) Value { if unsafe.Sizeof(v) == 4 { @@ -126,8 +125,8 @@ func IntValue(v int) Value { return Int64Value(int64(v)) } -// Uint creates either a UINT32 or a UINT64 Value, depending on -// whether the uint type is 32 or 64 bits wide. +// UintValue creates either a UINT32 or a UINT64 Value, depending on whether +// the uint type is 32 or 64 bits wide. func UintValue(v uint) Value { if unsafe.Sizeof(v) == 4 { return Uint32Value(uint32(v)) @@ -135,27 +134,32 @@ func UintValue(v uint) Value { return Uint64Value(uint64(v)) } -// Array creates an ARRAY value. -func ArrayValue(array interface{}) Value { - switch reflect.TypeOf(array).Kind() { +// ArrayValue creates an ARRAY value from an array or slice. +// Only arrays or slices of bool, int, int32, int64, uint, uint32, uint64, +// float, float32, float64, or string types are allowed. Specifically, arrays +// and slices can not contain other arrays, slices, structs, or non-standard +// types. If the passed value is not an array or slice of these types an +// INVALID value is returned. +func ArrayValue(v interface{}) Value { + switch reflect.TypeOf(v).Kind() { case reflect.Array, reflect.Slice: - isValidType := func() bool { - // get array type regardless of dimensions - typeName := reflect.TypeOf(array).String() - typeName = typeName[strings.LastIndex(typeName, "]")+1:] - switch typeName { - case "bool", "int", "int32", "int64", - "float32", "float64", "string", - "uint", "uint32", "uint64": - return true - } - return false - }() - if isValidType { + // get array type regardless of dimensions + typ := reflect.TypeOf(v).Elem() + kind := typ.Kind() + switch kind { + case reflect.Bool, reflect.Int, reflect.Int32, reflect.Int64, + reflect.Float32, reflect.Float64, reflect.String, + reflect.Uint, reflect.Uint32, reflect.Uint64: + val := reflect.ValueOf(v) + length := val.Len() + frozen := reflect.Indirect(reflect.New(reflect.ArrayOf(length, typ))) + reflect.Copy(frozen, val) return Value{ vtype: ARRAY, - array: array, + array: frozen.Interface(), } + default: + return Value{vtype: INVALID} } } return Value{vtype: INVALID} diff --git a/label/value_test.go b/label/value_test.go index 70685945b50d..7842fceef089 100644 --- a/label/value_test.go +++ b/label/value_test.go @@ -15,6 +15,7 @@ package label_test import ( + "reflect" "testing" "unsafe" @@ -42,7 +43,7 @@ func TestValue(t *testing.T) { name: "Key.Array([]bool) correctly return key's internal bool values", value: k.Array([]bool{true, false}).Value, wantType: label.ARRAY, - wantValue: []bool{true, false}, + wantValue: [2]bool{true, false}, }, { name: "Key.Int64() correctly returns keys's internal int64 value", @@ -102,67 +103,70 @@ func TestValue(t *testing.T) { name: "Key.Array([]int64) correctly returns keys's internal int64 values", value: k.Array([]int64{42, 43}).Value, wantType: label.ARRAY, - wantValue: []int64{42, 43}, + wantValue: [2]int64{42, 43}, }, { name: "KeyArray([]uint64) correctly returns keys's internal uint64 values", value: k.Array([]uint64{42, 43}).Value, wantType: label.ARRAY, - wantValue: []uint64{42, 43}, + wantValue: [2]uint64{42, 43}, }, { name: "Key.Array([]float64) correctly returns keys's internal float64 values", value: k.Array([]float64{42, 43}).Value, wantType: label.ARRAY, - wantValue: []float64{42, 43}, + wantValue: [2]float64{42, 43}, }, { name: "Key.Array([]int32) correctly returns keys's internal int32 values", value: k.Array([]int32{42, 43}).Value, wantType: label.ARRAY, - wantValue: []int32{42, 43}, + wantValue: [2]int32{42, 43}, }, { name: "Key.Array([]uint32) correctly returns keys's internal uint32 values", value: k.Array([]uint32{42, 43}).Value, wantType: label.ARRAY, - wantValue: []uint32{42, 43}, + wantValue: [2]uint32{42, 43}, }, { name: "Key.Array([]float32) correctly returns keys's internal float32 values", value: k.Array([]float32{42, 43}).Value, wantType: label.ARRAY, - wantValue: []float32{42, 43}, + wantValue: [2]float32{42, 43}, }, { name: "Key.Array([]string) correctly return key's internal string values", value: k.Array([]string{"foo", "bar"}).Value, wantType: label.ARRAY, - wantValue: []string{"foo", "bar"}, + wantValue: [2]string{"foo", "bar"}, }, { name: "Key.Array([]int) correctly returns keys's internal signed integral values", value: k.Array([]int{42, 43}).Value, wantType: label.ARRAY, - wantValue: []int{42, 43}, + wantValue: [2]int{42, 43}, }, { name: "Key.Array([]uint) correctly returns keys's internal unsigned integral values", value: k.Array([]uint{42, 43}).Value, wantType: label.ARRAY, - wantValue: []uint{42, 43}, + wantValue: [2]uint{42, 43}, }, { - name: "Key.Array([][]int) correctly return key's multi dimensional array", + name: "Key.Array([][]int) refuses to create multi-dimensional array", value: k.Array([][]int{{1, 2}, {3, 4}}).Value, - wantType: label.ARRAY, - wantValue: [][]int{{1, 2}, {3, 4}}, + wantType: label.INVALID, + wantValue: nil, }, } { t.Logf("Running test case %s", testcase.name) if testcase.value.Type() != testcase.wantType { t.Errorf("wrong value type, got %#v, expected %#v", testcase.value.Type(), testcase.wantType) } + if testcase.wantType == label.INVALID { + continue + } got := testcase.value.AsInterface() if diff := cmp.Diff(testcase.wantValue, got); diff != "" { t.Errorf("+got, -want: %s", diff) @@ -199,3 +203,11 @@ func getBitlessInfo(i int) bitlessInfo { unsignedValue: uint64(i), } } + +func TestAsArrayValue(t *testing.T) { + v := label.ArrayValue([]uint{1, 2, 3}).AsArray() + // Ensure the returned dynamic type is stable. + if got, want := reflect.TypeOf(v).Kind(), reflect.Array; got != want { + t.Errorf("AsArray() returned %T, want %T", got, want) + } +}