diff --git a/CHANGELOG.md b/CHANGELOG.md index a57e7a48bd3..b062c7f833b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/attribute/kv.go b/attribute/kv.go index 0e5b06d8273..2fcc8863f73 100644 --- a/attribute/kv.go +++ b/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 { diff --git a/attribute/kv_test.go b/attribute/kv_test.go index 3f77fdf2913..907e20e9766 100644 --- a/attribute/kv_test.go +++ b/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) + } + } +} diff --git a/sdk/resource/builtin.go b/sdk/resource/builtin.go index 4bf09e19647..c80ab697c6f 100644 --- a/sdk/resource/builtin.go +++ b/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 } diff --git a/sdk/resource/builtin_test.go b/sdk/resource/builtin_test.go index aac9e5d4943..0e93aa17828 100644 --- a/sdk/resource/builtin_test.go +++ b/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) + } + } diff --git a/sdk/resource/resource.go b/sdk/resource/resource.go index 169da2ae55a..a2db66cf3b5 100644 --- a/sdk/resource/resource.go +++ b/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 diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index 0531ae65144..00f0c6cca18 100644 --- a/sdk/resource/resource_test.go +++ b/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) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 4d31579a690..e303ca22b96 100644 --- a/sdk/trace/span.go +++ b/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) } }