From b221025a4cf87778f28ea0879ef5bcf29a3c849d Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Mon, 7 Aug 2023 12:22:48 -0500 Subject: [PATCH] sdk/resource: Fix data race with emptyAttributes (#4409) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix ASAN bug with emptyAttributes. If mutliple instantiations of a Resources struct are happening in parallel, they can end up modifying the same underlying resource. * Capture literal * add test * remove unwanted change * Update sdk/resource/resource_test.go Co-authored-by: Robert Pająk * Update sdk/resource/resource_test.go Co-authored-by: Robert Pająk * Update sdk/resource/resource_test.go Co-authored-by: Robert Pająk * Update sdk/resource/resource_test.go Co-authored-by: Robert Pająk * Add changelog. * Update CHANGELOG.md Co-authored-by: Robert Pająk --------- Co-authored-by: Robert Pająk Co-authored-by: Tyler Yahn --- CHANGELOG.md | 1 + sdk/resource/resource.go | 9 ++++----- sdk/resource/resource_test.go | 27 +++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccb9399c7e5..5194b363501 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Decouple `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp/internal` from `go.opentelemetry.io/otel/exporters/otlp/internal` and `go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal` using gotmpl. (#4401, #3846) - Do not block the metric SDK when OTLP metric exports are blocked in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`. (#3925, #4395) - Do not append _total if the counter already ends in total `go.opentelemetry.io/otel/exporter/prometheus`. (#4373) +- Fix resource detection data race in `go.opentelemetry.io/otel/sdk/resource`. (#4409) ## [1.16.0/0.39.0] 2023-05-18 diff --git a/sdk/resource/resource.go b/sdk/resource/resource.go index 139dc7e8f92..176ff106668 100644 --- a/sdk/resource/resource.go +++ b/sdk/resource/resource.go @@ -36,7 +36,6 @@ type Resource struct { } var ( - emptyResource Resource defaultResource *Resource defaultResourceOnce sync.Once ) @@ -70,7 +69,7 @@ func NewWithAttributes(schemaURL string, attrs ...attribute.KeyValue) *Resource // of the attrs is known use NewWithAttributes instead. func NewSchemaless(attrs ...attribute.KeyValue) *Resource { if len(attrs) == 0 { - return &emptyResource + return &Resource{} } // Ensure attributes comply with the specification: @@ -81,7 +80,7 @@ func NewSchemaless(attrs ...attribute.KeyValue) *Resource { // If attrs only contains invalid entries do not allocate a new resource. if s.Len() == 0 { - return &emptyResource + return &Resource{} } return &Resource{attrs: s} //nolint @@ -195,7 +194,7 @@ func Merge(a, b *Resource) (*Resource, error) { // Empty returns an instance of Resource with no attributes. It is // equivalent to a `nil` Resource. func Empty() *Resource { - return &emptyResource + return &Resource{} } // Default returns an instance of Resource with a default @@ -214,7 +213,7 @@ func Default() *Resource { } // If Detect did not return a valid resource, fall back to emptyResource. if defaultResource == nil { - defaultResource = &emptyResource + defaultResource = &Resource{} } }) return defaultResource diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index b59b093422b..34d45b99c0f 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "strings" + "sync" "testing" "github.com/google/go-cmp/cmp" @@ -769,3 +770,29 @@ func TestWithContainer(t *testing.T) { string(semconv.ContainerIDKey): fakeContainerID, }, toMap(res)) } + +func TestResourceConcurrentSafe(t *testing.T) { + // Creating Resources should also be free of any data races, + // because Resources are immutable. + var wg sync.WaitGroup + for i := 0; i < 2; i++ { + wg.Add(1) + go func() { + defer wg.Done() + d := &fakeDetector{} + _, err := resource.Detect(context.Background(), d) + assert.NoError(t, err) + }() + } + wg.Wait() +} + +type fakeDetector struct{} + +func (f fakeDetector) Detect(_ context.Context) (*resource.Resource, error) { + // A bit pedantic, but resource.NewWithAttributes returns an empty Resource when + // no attributes specified. We want to make sure that this is concurrent-safe. + return resource.NewWithAttributes("https://opentelemetry.io/schemas/1.3.0"), nil +} + +var _ resource.Detector = &fakeDetector{}