Remove BYTES values, as they are are not thread safe (open-telemetry#249

) * Remove BYTES values, as they are mutable * Remove more BYTES
ctsquires · Oct 30, 2019 · 320c62a · 320c62a
1 parent a2f3dca
commit 320c62a
Show file tree

Hide file tree

Showing 11 changed files with 11 additions and 86 deletions.
diff --git a/api/core/key.go b/api/core/key.go
@@ -21,10 +21,14 @@ type Value struct {
 	Uint64  uint64
 	Float64 float64
 	String  string
-	Bytes   []byte
 
-	// TODO See how segmentio/stats handles this type, it's much smaller.
-	// TODO Lazy value type?
+	// Note: this type could be made smaller by using a
+	// core.Number to represent four of these fields, e.g.,
+	// struct {
+	//   Type   ValueType
+	//   String string
+	//   Number Number
+	// }
 }
 
 const (
@@ -37,7 +41,6 @@ const (
 	FLOAT32
 	FLOAT64
 	STRING
-	BYTES
 )
 
 func (k Key) Bool(v bool) KeyValue {
@@ -120,16 +123,6 @@ func (k Key) String(v string) KeyValue {
 	}
 }
 
-func (k Key) Bytes(v []byte) KeyValue {
-	return KeyValue{
-		Key: k,
-		Value: Value{
-			Type:  BYTES,
-			Bytes: v,
-		},
-	}
-}
-
 func (k Key) Int(v int) KeyValue {
 	if unsafe.Sizeof(v) == 4 {
 		return k.Int32(int32(v))
@@ -148,7 +141,6 @@ func (k Key) Defined() bool {
 	return len(k) != 0
 }
 
-// TODO make this a lazy one-time conversion.
 func (v Value) Emit() string {
 	switch v.Type {
 	case BOOL:
@@ -161,8 +153,6 @@ func (v Value) Emit() string {
 		return fmt.Sprint(v.Float64)
 	case STRING:
 		return v.String
-	case BYTES:
-		return string(v.Bytes)
 	}
 	return "unknown"
 }
diff --git a/api/core/key_test.go b/api/core/key_test.go
@@ -209,31 +209,6 @@ func TestString(t *testing.T) {
 	}
 }
 
-func TestBytes(t *testing.T) {
-	for _, testcase := range []struct {
-		name string
-		v    []byte
-		want core.Value
-	}{
-		{
-			name: "Key.Bytes() correctly returns keys's internal []byte value",
-			v:    []byte{'f', 'o', 'o'},
-			want: core.Value{
-				Type:  core.BYTES,
-				Bytes: []byte{'f', 'o', 'o'},
-			},
-		},
-	} {
-		t.Run(testcase.name, func(t *testing.T) {
-			//proto: func (k core.Key) Bytes(v []byte) KeyValue {
-			have := core.Key("").Bytes(testcase.v)
-			if diff := cmp.Diff(testcase.want, have.Value); diff != "" {
-				t.Fatal(diff)
-			}
-		})
-	}
-}
-
 func TestInt(t *testing.T) {
 	WTYPE := core.INT64
 	if unsafe.Sizeof(int(42)) == 4 {
@@ -393,14 +368,6 @@ func TestEmit(t *testing.T) {
 			},
 			want: "foo",
 		},
-		{
-			name: `test Key.Emit() can emit a string representing self.BYTES`,
-			v: core.Value{
-				Type:  core.BYTES,
-				Bytes: []byte{'f', 'o', 'o'},
-			},
-			want: "foo",
-		},
 	} {
 		t.Run(testcase.name, func(t *testing.T) {
 			//proto: func (v core.Value) Emit() string {

diff --git a/api/key/key.go b/api/key/key.go
@@ -40,10 +40,6 @@ func String(k, v string) core.KeyValue {
 	return New(k).String(v)
 }
 
-func Bytes(k string, v []byte) core.KeyValue {
-	return New(k).Bytes(v)
-}
-
 func Int(k string, v int) core.KeyValue {
 	return New(k).Int(v)
 }

diff --git a/api/key/key_test.go b/api/key/key_test.go
@@ -94,17 +94,6 @@ func TestKeyValueConstructors(t *testing.T) {
 				},
 			},
 		},
-		{
-			name:   "Bytes",
-			actual: key.Bytes("k1", []byte("v1")),
-			expected: core.KeyValue{
-				Key: "k1",
-				Value: core.Value{
-					Type:  core.BYTES,
-					Bytes: []byte("v1"),
-				},
-			},
-		},
 		{
 			name:   "Int",
 			actual: key.Int("k1", 123),

diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go
@@ -422,8 +422,6 @@ func otTagToOtelCoreKeyValue(k string, v interface{}) otelcore.KeyValue {
 		return key.Uint(val)
 	case string:
 		return key.String(val)
-	case []byte:
-		return key.Bytes(val)
 	default:
 		return key.String(fmt.Sprint(v))
 	}

diff --git a/example/jaeger/main.go b/example/jaeger/main.go
@@ -38,7 +38,6 @@ func initTracer() func() {
 			Tags: []core.KeyValue{
 				key.String("exporter", "jaeger"),
 				key.Float64("float", 312.23),
-				key.Bytes("bytes", []byte("byte array")),
 			},
 		}),
 	)

diff --git a/exporter/trace/jaeger/jaeger.go b/exporter/trace/jaeger/jaeger.go
@@ -248,12 +248,6 @@ func keyValueToTag(kv core.KeyValue) *gen.Tag {
 			VDouble: &kv.Value.Float64,
 			VType:   gen.TagType_DOUBLE,
 		}
-	case core.BYTES:
-		tag = &gen.Tag{
-			Key:     string(kv.Key),
-			VBinary: kv.Value.Bytes,
-			VType:   gen.TagType_BINARY,
-		}
 	}
 	return tag
 }

diff --git a/exporter/trace/jaeger/jaeger_test.go b/exporter/trace/jaeger/jaeger_test.go
@@ -45,7 +45,6 @@ func Test_spanDataToThrift(t *testing.T) {
 	keyValue := "value"
 	statusCodeValue := int64(2)
 	doubleValue := float64(123.456)
-	bytesValue := []byte("byte array")
 	boolTrue := true
 	statusMessage := "Unknown"
 
@@ -75,7 +74,6 @@ func Test_spanDataToThrift(t *testing.T) {
 				Attributes: []core.KeyValue{
 					key.String("key", keyValue),
 					key.Float64("double", doubleValue),
-					key.Bytes("bytes", bytesValue),
 					// Jaeger doesn't handle Uint tags, this should be ignored.
 					key.Uint64("ignored", 123),
 				},
@@ -92,7 +90,6 @@ func Test_spanDataToThrift(t *testing.T) {
 				Tags: []*gen.Tag{
 					{Key: "double", VType: gen.TagType_DOUBLE, VDouble: &doubleValue},
 					{Key: "key", VType: gen.TagType_STRING, VStr: &keyValue},
-					{Key: "bytes", VType: gen.TagType_BINARY, VBinary: bytesValue},
 					{Key: "error", VType: gen.TagType_BOOL, VBool: &boolTrue},
 					{Key: "status.code", VType: gen.TagType_LONG, VLong: &statusCodeValue},
 					{Key: "status.message", VType: gen.TagType_STRING, VStr: &statusMessage},

diff --git a/exporter/trace/stdout/stdout_test.go b/exporter/trace/stdout/stdout_test.go
@@ -82,11 +82,11 @@ func TestExporter_ExportSpan(t *testing.T) {
 		`"Attributes":[` +
 		`{` +
 		`"Key":"key",` +
-		`"Value":{"Type":8,"Bool":false,"Int64":0,"Uint64":0,"Float64":0,"String":"value","Bytes":null}` +
+		`"Value":{"Type":8,"Bool":false,"Int64":0,"Uint64":0,"Float64":0,"String":"value"}` +
 		`},` +
 		`{` +
 		`"Key":"double",` +
-		`"Value":{"Type":7,"Bool":false,"Int64":0,"Uint64":0,"Float64":123.456,"String":"","Bytes":null}` +
+		`"Value":{"Type":7,"Bool":false,"Int64":0,"Uint64":0,"Float64":123.456,"String":""}` +
 		`}` +
 		`],` +
 		`"MessageEvents":null,` +

diff --git a/propagation/http_trace_context_propagator_test.go b/propagation/http_trace_context_propagator_test.go
@@ -436,7 +436,6 @@ func TestInjectCorrelationContextToHTTPReq(t *testing.T) {
 				key.New("key7").Uint64(123),
 				key.New("key8").Float64(123.567),
 				key.New("key9").Float32(123.567),
-				key.New("key10").Bytes([]byte{0x68, 0x69}),
 			},
 			wantInHeader: []string{
 				"key1=true",
@@ -448,7 +447,6 @@ func TestInjectCorrelationContextToHTTPReq(t *testing.T) {
 				"key7=123",
 				"key8=123.567",
 				"key9=123.56700134277344",
-				"key10=hi",
 			},
 		},
 	}

diff --git a/sdk/trace/benchmark_test.go b/sdk/trace/benchmark_test.go
@@ -99,9 +99,8 @@ func BenchmarkSpanWithAttributes_all(b *testing.B) {
 				key.New("key6").Uint32(123),
 				key.New("key7").Float64(123.456),
 				key.New("key8").Float32(123.456),
-				key.New("key9").Bytes([]byte{1, 2, 3, 4}),
-				key.New("key10").Int(123),
-				key.New("key11").Uint(123),
+				key.New("key9").Int(123),
+				key.New("key10").Uint(123),
 			)
 			span.End()
 		}
@@ -125,7 +124,6 @@ func BenchmarkSpanWithAttributes_all_2x(b *testing.B) {
 				key.New("key6").Uint32(123),
 				key.New("key7").Float64(123.456),
 				key.New("key8").Float32(123.456),
-				key.New("key9").Bytes([]byte{1, 2, 3, 4}),
 				key.New("key10").Int(123),
 				key.New("key11").Uint(123),
 				key.New("key21").Bool(false),
@@ -136,7 +134,6 @@ func BenchmarkSpanWithAttributes_all_2x(b *testing.B) {
 				key.New("key26").Uint32(123),
 				key.New("key27").Float64(123.456),
 				key.New("key28").Float32(123.456),
-				key.New("key29").Bytes([]byte{1, 2, 3, 4}),
 				key.New("key210").Int(123),
 				key.New("key211").Uint(123),
 			)