From 72384fd12182f819ded619479232ff71565e2cd2 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 31 Jan 2024 08:45:28 -0800 Subject: [PATCH 1/9] Return merged Resource on schema conflict --- sdk/resource/auto.go | 20 +++++++++++-- sdk/resource/resource.go | 56 +++++++++++++++++++++++++++-------- sdk/resource/resource_test.go | 24 ++++++++++----- 3 files changed, 78 insertions(+), 22 deletions(-) diff --git a/sdk/resource/auto.go b/sdk/resource/auto.go index 4279013be88..718157288a9 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 diff --git a/sdk/resource/resource.go b/sdk/resource/resource.go index 8c69ba25a4f..5b2678671a8 100644 --- a/sdk/resource/resource.go +++ b/sdk/resource/resource.go @@ -40,9 +40,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 +157,30 @@ 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. +// +// The SchemaURL of the resources will be merged according to the +// [OpenTelemetry specification rules]: +// +// - 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 (the schema URL of b and a are not empty +// and different). See below for how this is handled. // -// 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. +// If the resources have different, non-empty, schema URLs an error containing +// [ErrSchemaURLConflict] will be returned with the merged Resource. 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. // -// 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. +// [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 @@ -168,7 +193,10 @@ func Merge(a, b *Resource) (*Resource, error) { } // Merge the schema URL. - var schemaURL string + var ( + schemaURL string + err error + ) switch true { case a.schemaURL == "": schemaURL = b.schemaURL @@ -177,7 +205,9 @@ func Merge(a, b *Resource) (*Resource, error) { case a.schemaURL == b.schemaURL: schemaURL = a.schemaURL default: - return Empty(), errMergeConflictSchemaURL + // 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. + err = ErrSchemaURLConflict } // Note: 'b' attributes will overwrite 'a' with last-value-wins in attribute.Key() @@ -188,7 +218,7 @@ func Merge(a, b *Resource) (*Resource, error) { combine = append(combine, mi.Attribute()) } merged := NewWithAttributes(schemaURL, combine...) - return merged, nil + return merged, err } // 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..5238877dc0e 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, }, } @@ -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: "", + isErr: true, }, { 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: "", + isErr: true, }, } for _, tt := range tc { From 0239f22c1b394c74b85a349447a5e8c2c97c21a2 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 31 Jan 2024 14:13:40 -0800 Subject: [PATCH 2/9] Add changes to changelog --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1437444568a..dc84527525b 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 From 14a73bbaec9863a93f867e7f93461d7fb6ce8540 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 1 Feb 2024 07:30:00 -0800 Subject: [PATCH 3/9] Doc returned resource to have no schema URL --- sdk/resource/resource.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/sdk/resource/resource.go b/sdk/resource/resource.go index 5b2678671a8..230e06f6e27 100644 --- a/sdk/resource/resource.go +++ b/sdk/resource/resource.go @@ -171,14 +171,13 @@ func (r *Resource) Equal(eq *Resource) bool { // 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 (the schema URL of b and a are not empty -// and different). See below for how this is handled. -// -// If the resources have different, non-empty, schema URLs an error containing -// [ErrSchemaURLConflict] will be returned with the merged Resource. 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. +// - 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) { From 47e7b94e5190d25576160403b698181301d04b90 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 1 Feb 2024 07:37:30 -0800 Subject: [PATCH 4/9] Refactor Merge based on feedback --- sdk/resource/resource.go | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/sdk/resource/resource.go b/sdk/resource/resource.go index 230e06f6e27..e1ce4161b59 100644 --- a/sdk/resource/resource.go +++ b/sdk/resource/resource.go @@ -191,24 +191,6 @@ func Merge(a, b *Resource) (*Resource, error) { return a, nil } - // Merge the schema URL. - var ( - schemaURL string - err error - ) - switch true { - case a.schemaURL == "": - schemaURL = b.schemaURL - case b.schemaURL == "": - schemaURL = a.schemaURL - case a.schemaURL == b.schemaURL: - schemaURL = a.schemaURL - default: - // 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. - err = ErrSchemaURLConflict - } - // 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()) @@ -216,8 +198,18 @@ func Merge(a, b *Resource) (*Resource, error) { for mi.Next() { combine = append(combine, mi.Attribute()) } - merged := NewWithAttributes(schemaURL, combine...) - return merged, err + + 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...), ErrSchemaURLConflict } // Empty returns an instance of Resource with no attributes. It is From 9f55256de3c62bd6542e7f5ea2fb8433189b1877 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 1 Feb 2024 07:47:01 -0800 Subject: [PATCH 5/9] Add the schema URLs to the returned error --- sdk/resource/resource.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sdk/resource/resource.go b/sdk/resource/resource.go index e1ce4161b59..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" @@ -209,7 +210,12 @@ func Merge(a, b *Resource) (*Resource, error) { } // 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...), ErrSchemaURLConflict + 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 From 9f117abee3249b49e0a2f88b3f2f591fcb52716c Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 1 Feb 2024 08:14:18 -0800 Subject: [PATCH 6/9] Ensure no schema URL when merge conflict on detect --- sdk/resource/auto.go | 5 ++++ sdk/resource/auto_test.go | 50 ++++++++++++++++++++++++++------------- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/sdk/resource/auto.go b/sdk/resource/auto.go index 718157288a9..aed756c5e70 100644 --- a/sdk/resource/auto.go +++ b/sdk/resource/auto.go @@ -94,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..1eb6f2e82af 100644 --- a/sdk/resource/auto_test.go +++ b/sdk/resource/auto_test.go @@ -16,6 +16,7 @@ package resource_test import ( "context" + "errors" "fmt" "os" "testing" @@ -28,32 +29,49 @@ import ( func TestDetect(t *testing.T) { cases := []struct { - name string - schema1, schema2 string - isErr bool + name string + schema []string + 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", + schema: []string{ + "https://opentelemetry.io/schemas/1.3.0", + "https://opentelemetry.io/schemas/1.4.0", + }, + 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: "three different schema urls", + schema: []string{ + "https://opentelemetry.io/schemas/1.3.0", + "https://opentelemetry.io/schemas/1.4.0", + "https://opentelemetry.io/schemas/1.5.0", + }, + wantErr: resource.ErrSchemaURLConflict, + }, + { + name: "same schema url", + schema: []string{ + "https://opentelemetry.io/schemas/1.4.0", + "https://opentelemetry.io/schemas/1.4.0", + }, }, } 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) + detectors := make([]resource.Detector, len(c.schema)) + for i, s := range c.schema { + detectors[i] = resource.StringDetector(s, semconv.HostNameKey, os.Hostname) + } + r, err := resource.Detect(context.Background(), detectors...) assert.NotNil(t, r) - if c.isErr { - assert.Error(t, err) + if c.wantErr != nil { + assert.ErrorIs(t, err, c.wantErr) + if errors.Is(c.wantErr, resource.ErrSchemaURLConflict) { + assert.Equal(t, "", r.SchemaURL()) + } } else { assert.NoError(t, err) } From b9ea2aa0fa6eb1aa7c16c211cac2b83abffdb4cc Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 1 Feb 2024 08:18:29 -0800 Subject: [PATCH 7/9] Replaced isErr with wantErr in TestNew --- sdk/resource/resource_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index 5238877dc0e..de5cd9a7c2a 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -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", @@ -413,7 +413,7 @@ func TestNew(t *testing.T) { }(), }, schemaURL: "", - isErr: true, + wantErr: resource.ErrSchemaURLConflict, }, { name: "With conflicting detector schema urls", @@ -432,7 +432,7 @@ func TestNew(t *testing.T) { }(), }, schemaURL: "", - isErr: true, + wantErr: resource.ErrSchemaURLConflict, }, } for _, tt := range tc { @@ -446,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)) From edb034349e14fada4fe5c1a7d179f5f7551509ff Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 1 Feb 2024 08:40:53 -0800 Subject: [PATCH 8/9] Update sdk/resource/auto_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- sdk/resource/auto_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/resource/auto_test.go b/sdk/resource/auto_test.go index 1eb6f2e82af..040c9fc6220 100644 --- a/sdk/resource/auto_test.go +++ b/sdk/resource/auto_test.go @@ -70,7 +70,7 @@ func TestDetect(t *testing.T) { if c.wantErr != nil { assert.ErrorIs(t, err, c.wantErr) if errors.Is(c.wantErr, resource.ErrSchemaURLConflict) { - assert.Equal(t, "", r.SchemaURL()) + assert.Zero(t, r.SchemaURL()) } } else { assert.NoError(t, err) From b0625132a6079de73abd54e07a2b06071245352e Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 1 Feb 2024 09:56:03 -0800 Subject: [PATCH 9/9] Update TestDetect based on feedback --- sdk/resource/auto_test.go | 66 ++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 21 deletions(-) diff --git a/sdk/resource/auto_test.go b/sdk/resource/auto_test.go index 040c9fc6220..fac62f3a344 100644 --- a/sdk/resource/auto_test.go +++ b/sdk/resource/auto_test.go @@ -18,55 +18,77 @@ 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 - schema []string - wantErr error + name string + detectors []resource.Detector + want *resource.Resource + wantErr error }{ { name: "two different schema urls", - schema: []string{ - "https://opentelemetry.io/schemas/1.3.0", - "https://opentelemetry.io/schemas/1.4.0", + detectors: []resource.Detector{ + newDetector(v130, alice, admin), + newDetector(v140, bob, user), }, + want: resource.NewSchemaless(bob, user), wantErr: resource.ErrSchemaURLConflict, }, { name: "three different schema urls", - schema: []string{ - "https://opentelemetry.io/schemas/1.3.0", - "https://opentelemetry.io/schemas/1.4.0", - "https://opentelemetry.io/schemas/1.5.0", + 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", - schema: []string{ - "https://opentelemetry.io/schemas/1.4.0", - "https://opentelemetry.io/schemas/1.4.0", + 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) { - detectors := make([]resource.Detector, len(c.schema)) - for i, s := range c.schema { - detectors[i] = resource.StringDetector(s, semconv.HostNameKey, os.Hostname) - } - r, err := resource.Detect(context.Background(), detectors...) - assert.NotNil(t, r) + 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) { @@ -75,6 +97,8 @@ func TestDetect(t *testing.T) { } else { assert.NoError(t, err) } + assert.Equal(t, c.want.SchemaURL(), r.SchemaURL()) + assert.ElementsMatch(t, c.want.Attributes(), r.Attributes()) }) } }