Comply with OpenTelemetry attributes specification (#1703)

* Add Valid method to KeyValue

* Use KeyValue.Valid in attribute add on Span

* Resource StringDetector errors for invalid attribute

* Ignore invalid attr in NewWithAttributes

The OpenTelemetry specification requires attributes conform to a
standard evaluated and returned by attribute.KeyValue.Valid. To comply
with the specification, Resources created from NewWithAttributes need to
only contain valid attributes. This adds a check to ensure this and
drops invalid attributes passed as arguments.

* Add changes to changelog

* Add nolint comment

The attribute.Set is (possibly overly) optimized to avoid allocations.
The returned value from the constructor is a value of a Set, not a
pointer to the Set. A Set contains a lock value and pointer methods so
passing the Set value raises the copylock go vet error. This copies the
same nolint comment from the `NewSet` method this used to use.

* Apply suggestions from code review

Co-authored-by: Sam Xie <[email protected]>

Co-authored-by: Sam Xie <[email protected]>

Author: MrAlias

CHANGELOG.md

@@ -14,6 +14,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - A `ForceFlush` method to the `"go.opentelemetry.io/otel/sdk/trace".TracerProvider` to flush all registered `SpanProcessor`s. (#1608)
 - Added `WithDefaultSampler` and `WithSpanLimits` to tracer provider. (#1633)
 - Jaeger exporter falls back to `resource.Default`'s `service.name` if the exported Span does not have one. (#1673)
+- A `Valid` method to the `"go.opentelemetry.io/otel/attribute".KeyValue` type. (#1703)
 
 ### Changed
 
@@ -27,6 +28,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
   - `trace.NewSpanContext()` can be used in conjunction with the `trace.SpanContextConfig` struct to initialize a new `SpanContext` where all values are known.
 - Renamed the `LabelSet` method of `"go.opentelemetry.io/otel/sdk/resource".Resource` to `Set`. (#1692)
 - Jaeger exporter populates Jaeger's Span Process from Resource. (#1673)
+- `"go.opentelemetry.io/otel/sdk/resource".NewWithAttributes` will now drop any invalid attributes passed. (#1703)
+- `"go.opentelemetry.io/otel/sdk/resource".StringDetector` will now error if the produced attribute is invalid. (#1703)
 
 ### Removed
 

attribute/kv.go

@@ -26,6 +26,11 @@ type KeyValue struct {
 	Value Value
 }
 
+// Valid returns if kv is a valid OpenTelemetry attribute.
+func (kv KeyValue) Valid() bool {
+	return kv.Key != "" && kv.Value.Type() != INVALID
+}
+
 // Bool creates a new key-value pair with a passed name and a bool
 // value.
 func Bool(k string, v bool) KeyValue {

attribute/kv_test.go

@@ -160,3 +160,62 @@ func TestAny(t *testing.T) {
 		}
 	}
 }
+
+func TestKeyValueValid(t *testing.T) {
+	tests := []struct {
+		desc  string
+		valid bool
+		kv    attribute.KeyValue
+	}{
+		{
+			desc:  "uninitialized KeyValue should be invalid",
+			valid: false,
+			kv:    attribute.KeyValue{},
+		},
+		{
+			desc:  "empty key value should be invalid",
+			valid: false,
+			kv:    attribute.Key("").Bool(true),
+		},
+		{
+			desc:  "INVALID value type should be invalid",
+			valid: false,
+			kv: attribute.KeyValue{
+				Key: attribute.Key("valid key"),
+				// Default type is INVALID.
+				Value: attribute.Value{},
+			},
+		},
+		{
+			desc:  "non-empty key with BOOL type Value should be valid",
+			valid: true,
+			kv:    attribute.Bool("bool", true),
+		},
+		{
+			desc:  "non-empty key with INT64 type Value should be valid",
+			valid: true,
+			kv:    attribute.Int64("int64", 0),
+		},
+		{
+			desc:  "non-empty key with FLOAT64 type Value should be valid",
+			valid: true,
+			kv:    attribute.Float64("float64", 0),
+		},
+		{
+			desc:  "non-empty key with STRING type Value should be valid",
+			valid: true,
+			kv:    attribute.String("string", ""),
+		},
+		{
+			desc:  "non-empty key with ARRAY type Value should be valid",
+			valid: true,
+			kv:    attribute.Array("array", []int{}),
+		},
+	}
+
+	for _, test := range tests {
+		if got, want := test.kv.Valid(), test.valid; got != want {
+			t.Error(test.desc)
+		}
+	}
+}

sdk/resource/builtin.go

@@ -81,6 +81,10 @@ func (sd stringDetector) Detect(ctx context.Context) (*Resource, error) {
 	if err != nil {
 		return nil, fmt.Errorf("%s: %w", string(sd.K), err)
 	}
+	a := sd.K.String(value)
+	if !a.Valid() {
+		return nil, fmt.Errorf("invalid attribute: %q -> %q", a.Key, a.Value.Emit())
+	}
 	return NewWithAttributes(sd.K.String(value)), nil
 }
 

sdk/resource/builtin_test.go

@@ -36,24 +36,44 @@ func TestBuiltinStringDetector(t *testing.T) {
 	require.Nil(t, res)
 }
 
-func TestBuiltinStringConfig(t *testing.T) {
-	res, err := resource.New(
-		context.Background(),
-		resource.WithoutBuiltin(),
-		resource.WithAttributes(attribute.String("A", "B")),
-		resource.WithDetectors(resource.StringDetector(attribute.Key("K"), func() (string, error) {
-			return "", fmt.Errorf("K-IS-MISSING")
-		})),
-	)
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "K-IS-MISSING")
-	require.NotNil(t, res)
-
-	m := map[string]string{}
-	for _, kv := range res.Attributes() {
-		m[string(kv.Key)] = kv.Value.Emit()
+func TestStringDetectorErrors(t *testing.T) {
+	tests := []struct {
+		desc        string
+		s           resource.Detector
+		errContains string
+	}{
+		{
+			desc: "explicit error from func should be returned",
+			s: resource.StringDetector(attribute.Key("K"), func() (string, error) {
+				return "", fmt.Errorf("K-IS-MISSING")
+			}),
+			errContains: "K-IS-MISSING",
+		},
+		{
+			desc: "empty key is an invalid",
+			s: resource.StringDetector(attribute.Key(""), func() (string, error) {
+				return "not-empty", nil
+			}),
+			errContains: "invalid attribute: \"\" -> \"not-empty\"",
+		},
 	}
-	require.EqualValues(t, map[string]string{
-		"A": "B",
-	}, m)
+
+	for _, test := range tests {
+		res, err := resource.New(
+			context.Background(),
+			resource.WithoutBuiltin(),
+			resource.WithAttributes(attribute.String("A", "B")),
+			resource.WithDetectors(test.s),
+		)
+		require.Error(t, err, test.desc)
+		require.Contains(t, err.Error(), test.errContains)
+		require.NotNil(t, res, "resource contains remaining valid entries")
+
+		m := map[string]string{}
+		for _, kv := range res.Attributes() {
+			m[string(kv.Key)] = kv.Value.Emit()
+		}
+		require.EqualValues(t, map[string]string{"A": "B"}, m)
+	}
+
 }

sdk/resource/resource.go

@@ -43,13 +43,26 @@ var (
 	}(Detect(context.Background(), defaultServiceNameDetector{}, TelemetrySDK{}))
 )
 
-// NewWithAttributes creates a resource from a set of attributes.  If there are
-// duplicate keys present in the list of attributes, then the last
-// value found for the key is preserved.
-func NewWithAttributes(kvs ...attribute.KeyValue) *Resource {
-	return &Resource{
-		attrs: attribute.NewSet(kvs...),
+// NewWithAttributes creates a resource from attrs. If attrs contains
+// duplicate keys, the last value will be used. If attrs contains any invalid
+// items those items will be dropped.
+func NewWithAttributes(attrs ...attribute.KeyValue) *Resource {
+	if len(attrs) == 0 {
+		return &emptyResource
 	}
+
+	// Ensure attributes comply with the specification:
+	// https://github.com/open-telemetry/opentelemetry-specification/blob/v1.0.1/specification/common/common.md#attributes
+	s, _ := attribute.NewSetWithFiltered(attrs, func(kv attribute.KeyValue) bool {
+		return kv.Valid()
+	})
+
+	// If attrs only contains invalid entries do not allocate a new resource.
+	if s.Len() == 0 {
+		return &emptyResource
+	}
+
+	return &Resource{s} //nolint
 }
 
 // String implements the Stringer interface and provides a

sdk/resource/resource_test.go

@@ -237,6 +237,14 @@ func TestString(t *testing.T) {
 			kvs:  []attribute.KeyValue{attribute.String(`A=a\,B`, `b`)},
 			want: `A\=a\\\,B=b`,
 		},
+		{
+			kvs:  []attribute.KeyValue{attribute.String("", "invalid")},
+			want: "",
+		},
+		{
+			kvs:  []attribute.KeyValue{attribute.String("", "invalid"), attribute.String("B", "b")},
+			want: "B=b",
+		},
 	} {
 		if got := resource.NewWithAttributes(test.kvs...).String(); got != test.want {
 			t.Errorf("Resource(%v).String() = %q, want %q", test.kvs, got, test.want)

sdk/trace/span.go

@@ -497,7 +497,7 @@ func (s *span) copyToCappedAttributes(attributes ...attribute.KeyValue) {
 	for _, a := range attributes {
 		// Ensure attributes conform to the specification:
 		// https://github.com/open-telemetry/opentelemetry-specification/blob/v1.0.1/specification/common/common.md#attributes
-		if a.Value.Type() != attribute.INVALID && a.Key != "" {
+		if a.Valid() {
 			s.attributes.add(a)
 		}
 	}