8000 Return merged Resource on schema conflict (#4876) · open-telemetry/opentelemetry-go@eabcef4 · GitHub
[go: up one dir, main page]
Skip to content

Commit eabcef4

Browse files
MrAliaspellared
MrAlias
and
pellared
authored
Return merged Resource on schema conflict (#4876)
* 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 <pellared@hotmail.com> * Update TestDetect based on feedback --------- Co-authored-by: Robert Pająk <pellared@hotmail.com>
1 parent 2c7761d commit eabcef4

File tree

5 files changed

+166
-57
lines changed

5 files changed

+166
-57
lines changed

‎CHANGELOG.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,15 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
1313
- Add `WithEndpointURL` option to the `exporters/otlp/otlpmetric/otlpmetricgrpc`, `exporters/otlp/otlpmetric/otlpmetrichttp`, `exporters/otlp/otlptrace/otlptracegrpc` and `exporters/otlp/otlptrace/otlptracehttp` packages. (#4808)
1414
- Experimental exemplar exporting is added to the metric SDK.
1515
See [metric documentation](./sdk/metric/EXPERIMENTAL.md#exemplars) for more information about this feature and how to enable it. (#4871)
16+
- `ErrSchemaURLConflict` is added to `go.opentelemetry.io/otel/sdk/resource`.
17+
This error is returned when a merge of two `Resource`s with different (non-empty) schema URL is attepted. (#4876)
18+
19+
### Changed
20+
21+
- 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.
22+
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.
23+
It is up to the user to decide if they want to use the returned `Resource` or not.
24+
It may have desired attributes overwritten or include stale semantic conventions. (#4876)
1625

1726
### Fixed
1827

‎sdk/resource/auto.go‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,15 +41,31 @@ type Detector interface {
4141
// must never be done outside of a new major release.
4242
}
4343

44-
// Detect calls all input detectors sequentially and merges each result with the previous one.
45-
// It returns the merged error too.
44+
// Detect returns a new [Resource] merged from all the Resources each of the
45+
// detectors produces. Each of the detectors are called sequentially, in the
46+
// order they are passed, merging the produced resource into the previous.
47+
//
48+
// This may return a partial Resource along with an error containing
49+
// [ErrPartialResource] if that error is returned from a detector. It may also
50+
// return a merge-conflicting Resource along with an error containing
51+
// [ErrSchemaURLConflict] if merging Resources from different detectors results
52+
// in a schema URL conflict. It is up to the caller to determine if this
53+
// returned Resource should be used or not.
54+
//
55+
// If one of the detectors returns an error that is not [ErrPartialResource],
56+
// the resource produced by the detector will not be merged and the returned
57+
// error will wrap that detector's error.
4658
func Detect(ctx context.Context, detectors ...Detector) (*Resource, error) {
4759
r := new(Resource)
4860
return r, detect(ctx, r, detectors)
4961
}
5062

5163
// detect runs all detectors using ctx and merges the result into res. This
5264
// assumes res is allocated and not nil, it will panic otherwise.
65+
//
66+
// If the detectors or merging resources produces any errors (i.e.
67+
// [ErrPartialResource] [ErrSchemaURLConflict]), a single error wrapping all of
68+
// these errors will be returned. Otherwise, nil is returned.
5369
func detect(ctx context.Context, res *Resource, detectors []Detector) error {
5470
var (
5571
r *Resource
@@ -78,6 +94,11 @@ func detect(ctx context.Context, res *Resource, detectors []Detector) error {
7894
if len(errs) == 0 {
7995
return nil
8096
}
97+
if errors.Is(errs, ErrSchemaURLConflict) {
98+
// If there has been a merge conflict, ensure the resource has no
99+
// schema URL.
100+
res.schemaURL = ""
101+
}
81102
return errs
82103
}
83104

‎sdk/resource/auto_test.go‎

Lines changed: 61 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,47 +16,89 @@ package resource_test
1616

1717
import (
1818
"context"
19+
"errors"
1920
"fmt"
20-
"os"
2121
"testing"
2222

2323
"github.com/stretchr/testify/assert"
2424

25+
"go.opentelemetry.io/otel/attribute"
2526
"go.opentelemetry.io/otel/sdk/resource"
26-
semconv "go.opentelemetry.io/otel/semconv/v1.24.0"
2727
)
2828

29+
type detector struct {
30+
SchemaURL string
31+
Attributes []attribute.KeyValue
32+
}
33+
34+
func newDetector(schemaURL string, attrs ...attribute.KeyValue) resource.Detector {
35+
return detector{schemaURL, attrs}
36+
}
37+
38+
func (d detector) Detect(context.Context) (*resource.Resource, error) {
39+
return resource.NewWithAttributes(d.SchemaURL, d.Attributes...), nil
40+
}
41+
2942
func TestDetect(t *testing.T) {
43+
v130 := "https://opentelemetry.io/schemas/1.3.0"
44+
v140 := "https://opentelemetry.io/schemas/1.4.0"
45+
v150 := "https://opentelemetry.io/schemas/1.5.0"
46+
47+
alice := attribute.String("name", "Alice")
48+
bob := attribute.String("name", "Bob")
49+
carol := attribute.String("name", "Carol")
50+
51+
admin := attribute.Bool("admin", true)
52+
user := attribute.Bool("admin", false)
53+
3054
cases := []struct {
31-
name string
32-
schema1, schema2 string
33-
isErr bool
55+
name string
56+
detectors []resource.Detector
57+
want *resource.Resource
58+
wantErr error
3459
}{
3560
{
36-
name: "different schema urls",
37-
schema1: "https://opentelemetry.io/schemas/1.3.0",
38-
schema2: "https://opentelemetry.io/schemas/1.4.0",
39-
isErr: true,
61+
name: "two different schema urls",
62+
detectors: []resource.Detector{
63+
newDetector(v130, alice, admin),
64+
newDetector(v140, bob, user),
65+
},
66+
want: resource.NewSchemaless(bob, user),
67+
wantErr: resource.ErrSchemaURLConflict,
68+
},
69+
{
70+
name: "three different schema urls",
71+
detectors: []resource.Detector{
72+
newDetector(v130, alice, admin),
73+
newDetector(v140, bob, user),
74+
newDetector(v150, carol),
75+
},
76+
want: resource.NewSchemaless(carol, user),
77+
wantErr: resource.ErrSchemaURLConflict,
4078
},
4179
{
42-
name: "same schema url",
43-
schema1: "https://opentelemetry.io/schemas/1.4.0",
44-
schema2: "https://opentelemetry.io/schemas/1.4.0",
45-
isErr: false,
80+
name: "same schema url",
81+
detectors: []resource.Detector{
82+
newDetector(v140, alice, admin),
83+
newDetector(v140, bob, user),
84+
},
85+
want: resource.NewWithAttributes(v140, bob, user),
4686
},
4787
}
4888

4989
for _, c := range cases {
5090
t.Run(fmt.Sprintf("case-%s", c.name), func(t *testing.T) {
51-
d1 := resource.StringDetector(c.schema1, semconv.HostNameKey, os.Hostname)
52-
d2 := resource.StringDetector(c.schema2, semconv.HostNameKey, os.Hostname)
53-
r, err := resource.Detect(context.Background(), d1, d2)
54-
assert.NotNil(t, r)
55-
if c.isErr {
56-
assert.Error(t, err)
91+
r, err := resource.Detect(context.Background(), c.detectors...)
92+
if c.wantErr != nil {
93+
assert.ErrorIs(t, err, c.wantErr)
94+
if errors.Is(c.wantErr, resource.ErrSchemaURLConflict) {
95+
assert.Zero(t, r.SchemaURL())
96+
}
5797
} else {
5898
assert.NoError(t, err)
5999
}
100+
assert.Equal(t, c.want.SchemaURL(), r.SchemaURL())
101+
assert.ElementsMatch(t, c.want.Attributes(), r.Attributes())
60102
})
61103
}
62104
}

‎sdk/resource/resource.go‎

Lines changed: 52 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package resource // import "go.opentelemetry.io/otel/sdk/resource"
1717
import (
1818
"context"
1919
"errors"
20+
"fmt"
2021
"sync"
2122

2223
"go.opentelemetry.io/otel"
@@ -40,9 +41,20 @@ var (
4041
defaultResourceOnce sync.Once
4142
)
4243

43-
var errMergeConflictSchemaURL = errors.New("cannot merge resource due to conflicting Schema URL")
44+
// ErrSchemaURLConflict is an error returned when two Resources are merged
45+
// together that contain different, non-empty, schema URLs.
46+
var ErrSchemaURLConflict = errors.New("conflicting Schema URL")
4447

45-
// New returns a Resource combined from the user-provided detectors.
48+
// New returns a [Resource] built using opts.
49+
//
50+
// This may return a partial Resource along with an error containing
51+
// [ErrPartialResource] if options that provide a [Detector] are used and that
52+
// error is returned from one or more of the Detectors. It may also return a
53+
// merge-conflict Resource along with an error containing
54+
// [ErrSchemaURLConflict] if merging Resources from the opts results in a
55+
// schema URL conflict (see [Resource.Merge] for more information). It is up to
56+
// the caller to determine if this returned Resource should be used or not
57+
// based on these errors.
4658
func New(ctx context.Context, opts ...Option) (*Resource, error) {
4759
cfg := config{}
4860
for _, opt := range opts {
@@ -146,16 +158,29 @@ func (r *Resource) Equal(eq *Resource) bool {
146158
return r.Equivalent() == eq.Equivalent()
147159
}
148160

149-
// Merge creates a new resource by combining resource a and b.
161+
// Merge creates a new [Resource] by merging a and b.
162+
//
163+
// If there are common keys between a and b, then the value from b will
164+
// overwrite the value from a, even if b's value is empty.
150165
//
151-
// If there are common keys between resource a and b, then the value
152-
// from resource b will overwrite the value from resource a, even
153-
// if resource b's value is empty.
166+
// The SchemaURL of the resources will be merged according to the
167+
// [OpenTelemetry specification rules]:
154168
//
155-
// The SchemaURL of the resources will be merged according to the spec rules:
156-
// https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/resource/sdk.md#merge
157-
// If the resources have different non-empty schemaURL an empty resource and an error
158-
// will be returned.
169+
// - If a's schema URL is empty then the returned Resource's schema URL will
170+
// be set to the schema URL of b,
171+
// - Else if b's schema URL is empty then the returned Resource's schema URL
172+
// will be set to the schema URL of a,
173+
// - Else if the schema URLs of a and b are the same then that will be the
174+
// schema URL of the returned Resource,
175+
// - Else this is a merging error. If the resources have different,
176+
// non-empty, schema URLs an error containing [ErrSchemaURLConflict] will
177+
// be returned with the merged Resource. The merged Resource will have an
178+
// empty schema URL. It may be the case that some unintended attributes
179+
// have been overwritten or old semantic conventions persisted in the
180+
// returned Resource. It is up to the caller to determine if this returned
181+
// Resource should be used or not.
182+
//
183+
// [OpenTelemetry specification rules]: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/resource/sdk.md#merge
159184
func Merge(a, b *Resource) (*Resource, error) {
160185
if a == nil && b == nil {
161186
return Empty(), nil
@@ -167,28 +192,30 @@ func Merge(a, b *Resource) (*Resource, error) {
167192
return a, nil
168193
}
169194

170-
// Merge the schema URL.
171-
var schemaURL string
172-
switch true {
173-
case a.schemaURL == "":
174-
schemaURL = b.schemaURL
175-
case b.schemaURL == "":
176-
schemaURL = a.schemaURL
177-
case a.schemaURL == b.schemaURL:
178-
schemaURL = a.schemaURL
179-
default:
180-
return Empty(), errMergeConflictSchemaURL
181-
}
182-
183195
// Note: 'b' attributes will overwrite 'a' with last-value-wins in attribute.Key()
184196
// Meaning this is equivalent to: append(a.Attributes(), b.Attributes()...)
185197
mi := attribute.NewMergeIterator(b.Set(), a.Set())
186198
combine := make([]attribute.KeyValue, 0, a.Len()+b.Len())
187199
for mi.Next() {
188200
combine = append(combine, mi.Attribute())
189201
}
190-
merged := NewWithAttributes(schemaURL, combine...)
191-
return merged, nil
202+
203+
switch {
204+
case a.schemaURL == "":
205+
return NewWithAttributes(b.schemaURL, combine...), nil
206+
case b.schemaURL == "":
207+
return NewWithAttributes(a.schemaURL, combine...), nil
208+
case a.schemaURL == b.schemaURL:
209+
return NewWithAttributes(a.schemaURL, combine...), nil
210+
}
211+
// Return the merged resource with an appropriate error. It is up to
212+
// the user to decide if the returned resource can be used or not.
213+
return NewSchemaless(combine...), fmt.Errorf(
214+
"%w: %s and %s",
215+
ErrSchemaURLConflict,
216+
a.schemaURL,
217+
b.schemaURL,
218+
)
192219
}
193220

194221
// Empty returns an instance of Resource with no attributes. It is

‎sdk/resource/resource_test.go‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ func TestMerge(t *testing.T) {
183183
name: "Merge with different schemas",
184184
a: resource.NewWithAttributes("https://opentelemetry.io/schemas/1.4.0", kv41),
185185
b: resource.NewWithAttributes("https://opentelemetry.io/schemas/1.3.0", kv42),
186-
want: nil,
186+
want: []attribute.KeyValue{kv42},
187187
isErr: true,
< 38BA /code>
188188
},
189189
}
@@ -324,7 +324,7 @@ func TestNew(t *testing.T) {
324324

325325
resourceValues map[string]string
326326
schemaURL string
327-
isErr bool
327+
wantErr error
328328
}{
329329
{
330330
name: "No Options returns empty resource",
@@ -406,9 +406,14 @@ func TestNew(t *testing.T) {
406406
),
407407
resource.WithSchemaURL("https://opentelemetry.io/schemas/1.1.0"),
408408
},
409-
resourceValues: map[string]string{},
410-
schemaURL: "",
411-
isErr: true,
409+
resourceValues: map[string]string{
410+
string(semconv.HostNameKey): func() (hostname string) {
411+
hostname, _ = os.Hostname()
412+
return hostname
413+
}(),
414+
},
415+
schemaURL: "",
416+
wantErr: resource.ErrSchemaURLConflict,
412417
},
413418
{
414419
name: "With conflicting detector schema urls",
@@ -420,9 +425,14 @@ func TestNew(t *testing.T) {
420425
),
421426
resource.WithSchemaURL("https://opentelemetry.io/schemas/1.2.0"),
422427
},
423-
resourceValues: map[string]string{},
424-
schemaURL: "",
425-
isErr: true,
428+
resourceValues: map[string]string{
429+
string(semconv.HostNameKey): func() (hostname string) {
430+
hostname, _ = os.Hostname()
431+
return hostname
432+
}(),
433+
},
434+
schemaURL: "",
435+
wantErr: resource.ErrSchemaURLConflict,
426436
},
427437
}
428438
for _, tt := range tc {
@@ -436,10 +446,10 @@ func TestNew(t *testing.T) {
436446
ctx := context.Background()
437447
res, err := resource.New(ctx, tt.options...)
438448

439-
if tt.isErr {
440-
require.Error(t, err)
449+
if tt.wantErr != nil {
450+
assert.ErrorIs(t, err, tt.wantErr)
441451
} else {
442-
require.NoError(t, err)
452+
assert.NoError(t, err)
443453
}
444454

445455
require.EqualValues(t, tt.resourceValues, toMap(res))

0 commit comments

Comments
 (0)
0