From eabcef4c2da6149b7e6dbbb8b7294402fcc287c1 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 5 Feb 2024 07:33:29 -0800 Subject: [PATCH] Return merged Resource on schema conflict (#4876) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Return merged Resource on schema conflict * Add changes to changelog * Doc returned resource to have no schema URL * Refactor Merge based on feedback * Add the schema URLs to the returned error * Ensure no schema URL when merge conflict on detect * Replaced isErr with wantErr in TestNew * Update sdk/resource/auto_test.go Co-authored-by: Robert Pająk * Update TestDetect based on feedback --------- Co-authored-by: Robert Pająk --- CHANGELOG.md | 9 ++++ sdk/resource/auto.go | 25 ++++++++++- sdk/resource/auto_test.go | 80 ++++++++++++++++++++++++++--------- sdk/resource/resource.go | 77 ++++++++++++++++++++++----------- sdk/resource/resource_test.go | 32 +++++++++----- 5 files changed, 166 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b1b82d8296..7cbd34d225b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,15 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add `WithEndpointURL` option to the `exporters/otlp/otlpmetric/otlpmetricgrpc`, `exporters/otlp/otlpmetric/otlpmetrichttp`, `exporters/otlp/otlptrace/otlptracegrpc` and `exporters/otlp/otlptrace/otlptracehttp` packages. (#4808) - Experimental exemplar exporting is added to the metric SDK. See [metric documentation](./sdk/metric/EXPERIMENTAL.md#exemplars) for more information about this feature and how to enable it. (#4871) +- `ErrSchemaURLConflict` is added to `go.opentelemetry.io/otel/sdk/resource`. + This error is returned when a merge of two `Resource`s with different (non-empty) schema URL is attepted. (#4876) + +### Changed + +- The `Merge` and `New` functions in `go.opentelemetry.io/otel/sdk/resource` now returns a partial result if there is a schema URL merge conflict. + Instead of returning `nil` when two `Resource`s with different (non-empty) schema URLs are merged the merged `Resource`, along with the new `ErrSchemaURLConflict` error, is returned. + It is up to the user to decide if they want to use the returned `Resource` or not. + It may have desired attributes overwritten or include stale semantic conventions. (#4876) ### Fixed diff --git a/sdk/resource/auto.go b/sdk/resource/auto.go index 4279013be88..aed756c5e70 100644 --- a/sdk/resource/auto.go +++ b/sdk/resource/auto.go @@ -41,8 +41,20 @@ type Detector interface { // must never be done outside of a new major release. } -// Detect calls all input detectors sequentially and merges each result with the previous one. -// It returns the merged error too. +// Detect returns a new [Resource] merged from all the Resources each of the +// detectors produces. Each of the detectors are called sequentially, in the +// order they are passed, merging the produced resource into the previous. +// +// This may return a partial Resource along with an error containing +// [ErrPartialResource] if that error is returned from a detector. It may also +// return a merge-conflicting Resource along with an error containing +// [ErrSchemaURLConflict] if merging Resources from different detectors results +// in a schema URL conflict. It is up to the caller to determine if this +// returned Resource should be used or not. +// +// If one of the detectors returns an error that is not [ErrPartialResource], +// the resource produced by the detector will not be merged and the returned +// error will wrap that detector's error. func Detect(ctx context.Context, detectors ...Detector) (*Resource, error) { r := new(Resource) return r, detect(ctx, r, detectors) @@ -50,6 +62,10 @@ func Detect(ctx context.Context, detectors ...Detector) (*Resource, error) { // detect runs all detectors using ctx and merges the result into res. This // assumes res is allocated and not nil, it will panic otherwise. +// +// If the detectors or merging resources produces any errors (i.e. +// [ErrPartialResource] [ErrSchemaURLConflict]), a single error wrapping all of +// these errors will be returned. Otherwise, nil is returned. func detect(ctx context.Context, res *Resource, detectors []Detector) error { var ( r *Resource @@ -78,6 +94,11 @@ func detect(ctx context.Context, res *Resource, detectors []Detector) error { if len(errs) == 0 { return nil } + if errors.Is(errs, ErrSchemaURLConflict) { + // If there has been a merge conflict, ensure the resource has no + // schema URL. + res.schemaURL = "" + } return errs } diff --git a/sdk/resource/auto_test.go b/sdk/resource/auto_test.go index 885aa93ed71..fac62f3a344 100644 --- a/sdk/resource/auto_test.go +++ b/sdk/resource/auto_test.go @@ -16,47 +16,89 @@ package resource_test import ( "context" + "errors" "fmt" - "os" "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/resource" - semconv "go.opentelemetry.io/otel/semconv/v1.24.0" ) +type detector struct { + SchemaURL string + Attributes []attribute.KeyValue +} + +func newDetector(schemaURL string, attrs ...attribute.KeyValue) resource.Detector { + return detector{schemaURL, attrs} +} + +func (d detector) Detect(context.Context) (*resource.Resource, error) { + return resource.NewWithAttributes(d.SchemaURL, d.Attributes...), nil +} + func TestDetect(t *testing.T) { + v130 := "https://opentelemetry.io/schemas/1.3.0" + v140 := "https://opentelemetry.io/schemas/1.4.0" + v150 := "https://opentelemetry.io/schemas/1.5.0" + + alice := attribute.String("name", "Alice") + bob := attribute.String("name", "Bob") + carol := attribute.String("name", "Carol") + + admin := attribute.Bool("admin", true) + user := attribute.Bool("admin", false) + cases := []struct { - name string - schema1, schema2 string - isErr bool + name string + detectors []resource.Detector + want *resource.Resource + wantErr error }{ { - name: "different schema urls", - schema1: "https://opentelemetry.io/schemas/1.3.0", - schema2: "https://opentelemetry.io/schemas/1.4.0", - isErr: true, + name: "two different schema urls", + detectors: []resource.Detector{ + newDetector(v130, alice, admin), + newDetector(v140, bob, user), + }, + want: resource.NewSchemaless(bob, user), + wantErr: resource.ErrSchemaURLConflict, + }, + { + name: "three different schema urls", + detectors: []resource.Detector{ + newDetector(v130, alice, admin), + newDetector(v140, bob, user), + newDetector(v150, carol), + }, + want: resource.NewSchemaless(carol, user), + wantErr: resource.ErrSchemaURLConflict, }, { - name: "same schema url", - schema1: "https://opentelemetry.io/schemas/1.4.0", - schema2: "https://opentelemetry.io/schemas/1.4.0", - isErr: false, + name: "same schema url", + detectors: []resource.Detector{ + newDetector(v140, alice, admin), + newDetector(v140, bob, user), + }, + want: resource.NewWithAttributes(v140, bob, user), }, } for _, c := range cases { t.Run(fmt.Sprintf("case-%s", c.name), func(t *testing.T) { - d1 := resource.StringDetector(c.schema1, semconv.HostNameKey, os.Hostname) - d2 := resource.StringDetector(c.schema2, semconv.HostNameKey, os.Hostname) - r, err := resource.Detect(context.Background(), d1, d2) - assert.NotNil(t, r) - if c.isErr { - assert.Error(t, err) + r, err := resource.Detect(context.Background(), c.detectors...) + if c.wantErr != nil { + assert.ErrorIs(t, err, c.wantErr) + if errors.Is(c.wantErr, resource.ErrSchemaURLConflict) { + assert.Zero(t, r.SchemaURL()) + } } else { assert.NoError(t, err) } + assert.Equal(t, c.want.SchemaURL(), r.SchemaURL()) + assert.ElementsMatch(t, c.want.Attributes(), r.Attributes()) }) } } diff --git a/sdk/resource/resource.go b/sdk/resource/resource.go index 8c69ba25a4f..cb1ee0a9ceb 100644 --- a/sdk/resource/resource.go +++ b/sdk/resource/resource.go @@ -17,6 +17,7 @@ package resource // import "go.opentelemetry.io/otel/sdk/resource" import ( "context" "errors" + "fmt" "sync" "go.opentelemetry.io/otel" @@ -40,9 +41,20 @@ var ( defaultResourceOnce sync.Once ) -var errMergeConflictSchemaURL = errors.New("cannot merge resource due to conflicting Schema URL") +// ErrSchemaURLConflict is an error returned when two Resources are merged +// together that contain different, non-empty, schema URLs. +var ErrSchemaURLConflict = errors.New("conflicting Schema URL") -// New returns a Resource combined from the user-provided detectors. +// New returns a [Resource] built using opts. +// +// This may return a partial Resource along with an error containing +// [ErrPartialResource] if options that provide a [Detector] are used and that +// error is returned from one or more of the Detectors. It may also return a +// merge-conflict Resource along with an error containing +// [ErrSchemaURLConflict] if merging Resources from the opts results in a +// schema URL conflict (see [Resource.Merge] for more information). It is up to +// the caller to determine if this returned Resource should be used or not +// based on these errors. func New(ctx context.Context, opts ...Option) (*Resource, error) { cfg := config{} for _, opt := range opts { @@ -146,16 +158,29 @@ func (r *Resource) Equal(eq *Resource) bool { return r.Equivalent() == eq.Equivalent() } -// Merge creates a new resource by combining resource a and b. +// Merge creates a new [Resource] by merging a and b. +// +// If there are common keys between a and b, then the value from b will +// overwrite the value from a, even if b's value is empty. // -// If there are common keys between resource a and b, then the value -// from resource b will overwrite the value from resource a, even -// if resource b's value is empty. +// The SchemaURL of the resources will be merged according to the +// [OpenTelemetry specification rules]: // -// The SchemaURL of the resources will be merged according to the spec rules: -// https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/resource/sdk.md#merge -// If the resources have different non-empty schemaURL an empty resource and an error -// will be returned. +// - If a's schema URL is empty then the returned Resource's schema URL will +// be set to the schema URL of b, +// - Else if b's schema URL is empty then the returned Resource's schema URL +// will be set to the schema URL of a, +// - Else if the schema URLs of a and b are the same then that will be the +// schema URL of the returned Resource, +// - Else this is a merging error. If the resources have different, +// non-empty, schema URLs an error containing [ErrSchemaURLConflict] will +// be returned with the merged Resource. The merged Resource will have an +// empty schema URL. It may be the case that some unintended attributes +// have been overwritten or old semantic conventions persisted in the +// returned Resource. It is up to the caller to determine if this returned +// Resource should be used or not. +// +// [OpenTelemetry specification rules]: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/resource/sdk.md#merge func Merge(a, b *Resource) (*Resource, error) { if a == nil && b == nil { return Empty(), nil @@ -167,19 +192,6 @@ func Merge(a, b *Resource) (*Resource, error) { return a, nil } - // Merge the schema URL. - var schemaURL string - switch true { - case a.schemaURL == "": - schemaURL = b.schemaURL - case b.schemaURL == "": - schemaURL = a.schemaURL - case a.schemaURL == b.schemaURL: - schemaURL = a.schemaURL - default: - return Empty(), errMergeConflictSchemaURL - } - // Note: 'b' attributes will overwrite 'a' with last-value-wins in attribute.Key() // Meaning this is equivalent to: append(a.Attributes(), b.Attributes()...) mi := attribute.NewMergeIterator(b.Set(), a.Set()) @@ -187,8 +199,23 @@ func Merge(a, b *Resource) (*Resource, error) { for mi.Next() { combine = append(combine, mi.Attribute()) } - merged := NewWithAttributes(schemaURL, combine...) - return merged, nil + + switch { + case a.schemaURL == "": + return NewWithAttributes(b.schemaURL, combine...), nil + case b.schemaURL == "": + return NewWithAttributes(a.schemaURL, combine...), nil + case a.schemaURL == b.schemaURL: + return NewWithAttributes(a.schemaURL, combine...), nil + } + // Return the merged resource with an appropriate error. It is up to + // the user to decide if the returned resource can be used or not. + return NewSchemaless(combine...), fmt.Errorf( + "%w: %s and %s", + ErrSchemaURLConflict, + a.schemaURL, + b.schemaURL, + ) } // Empty returns an instance of Resource with no attributes. It is diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index baed4a31336..de5cd9a7c2a 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -183,7 +183,7 @@ func TestMerge(t *testing.T) { name: "Merge with different schemas", a: resource.NewWithAttributes("https://opentelemetry.io/schemas/1.4.0", kv41), b: resource.NewWithAttributes("https://opentelemetry.io/schemas/1.3.0", kv42), - want: nil, + want: []attribute.KeyValue{kv42}, isErr: true, }, } @@ -324,7 +324,7 @@ func TestNew(t *testing.T) { resourceValues map[string]string schemaURL string - isErr bool + wantErr error }{ { name: "No Options returns empty resource", @@ -406,9 +406,14 @@ func TestNew(t *testing.T) { ), resource.WithSchemaURL("https://opentelemetry.io/schemas/1.1.0"), }, - resourceValues: map[string]string{}, - schemaURL: "", - isErr: true, + resourceValues: map[string]string{ + string(semconv.HostNameKey): func() (hostname string) { + hostname, _ = os.Hostname() + return hostname + }(), + }, + schemaURL: "", + wantErr: resource.ErrSchemaURLConflict, }, { name: "With conflicting detector schema urls", @@ -420,9 +425,14 @@ func TestNew(t *testing.T) { ), resource.WithSchemaURL("https://opentelemetry.io/schemas/1.2.0"), }, - resourceValues: map[string]string{}, - schemaURL: "", - isErr: true, + resourceValues: map[string]string{ + string(semconv.HostNameKey): func() (hostname string) { + hostname, _ = os.Hostname() + return hostname + }(), + }, + schemaURL: "", + wantErr: resource.ErrSchemaURLConflict, }, } for _, tt := range tc { @@ -436,10 +446,10 @@ func TestNew(t *testing.T) { ctx := context.Background() res, err := resource.New(ctx, tt.options...) - if tt.isErr { - require.Error(t, err) + if tt.wantErr != nil { + assert.ErrorIs(t, err, tt.wantErr) } else { - require.NoError(t, err) + assert.NoError(t, err) } require.EqualValues(t, tt.resourceValues, toMap(res))