From eb02f968a768d0298fa126a7acd3eaf2d452516f Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Fri, 4 Aug 2023 10:16:12 -0500 Subject: [PATCH 01/10] 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. --- sdk/resource/resource.go | 9 ++++----- sdk/resource/resource_test.go | 4 ++++ 2 files changed, 8 insertions(+), 5 deletions(-) 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 7cdd358caaa..4c922ad7b76 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -67,6 +67,10 @@ func TestNewWithAttributes(t *testing.T) { } for _, c := range cases { t.Run(fmt.Sprintf("case-%s", c.name), func(t *testing.T) { + // Run ctors in parallel to verify that there is no + // race condition. + t.Parallel() + res := resource.NewSchemaless(c.in...) if diff := cmp.Diff( res.Attributes(), From 8c2aa29e3a15d3ff3da0a3901304ccf6246b9f0a Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Fri, 4 Aug 2023 10:21:49 -0500 Subject: [PATCH 02/10] Capture literal --- sdk/resource/resource_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index 4c922ad7b76..b830713d20e 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -66,6 +66,7 @@ func TestNewWithAttributes(t *testing.T) { }, } for _, c := range cases { + c := c t.Run(fmt.Sprintf("case-%s", c.name), func(t *testing.T) { // Run ctors in parallel to verify that there is no // race condition. From 537168916bf2166801edc631cdf88f092e41ba75 Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Sat, 5 Aug 2023 05:49:48 -0500 Subject: [PATCH 03/10] add test --- sdk/resource/resource_test.go | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index b830713d20e..e7a49febe23 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" @@ -68,10 +69,6 @@ func TestNewWithAttributes(t *testing.T) { for _, c := range cases { c := c t.Run(fmt.Sprintf("case-%s", c.name), func(t *testing.T) { - // Run ctors in parallel to verify that there is no - // race condition. - t.Parallel() - res := resource.NewSchemaless(c.in...) if diff := cmp.Diff( res.Attributes(), @@ -774,3 +771,30 @@ func TestWithContainer(t *testing.T) { string(semconv.ContainerIDKey): fakeContainerID, }, toMap(res)) } + +func TestResourceRace(t *testing.T) { + // Creating Resources should also be free of any ASAN issues, + // because Resources are immutable. + var wg sync.WaitGroup + wg.Add(2) + + for i := 0; i < 2; i++ { + 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 memory-safe. + return resource.NewWithAttributes("https://opentelemetry.io/schemas/1.3.0"), nil +} + +var _ resource.Detector = &fakeDetector{} From ed488f8c1a4cedb7e4fe33e120636b97cd840e48 Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Sat, 5 Aug 2023 05:52:07 -0500 Subject: [PATCH 04/10] remove unwanted change --- sdk/resource/resource_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index e7a49febe23..d6693efdc33 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -67,7 +67,6 @@ func TestNewWithAttributes(t *testing.T) { }, } for _, c := range cases { - c := c t.Run(fmt.Sprintf("case-%s", c.name), func(t *testing.T) { res := resource.NewSchemaless(c.in...) if diff := cmp.Diff( From d720ac6aa642dccec5e608fd944da0b5f7ee6b9a Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Mon, 7 Aug 2023 05:14:49 -0500 Subject: [PATCH 05/10] Update sdk/resource/resource_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/resource_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index d6693efdc33..12f8925b830 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -775,9 +775,8 @@ func TestResourceRace(t *testing.T) { // Creating Resources should also be free of any ASAN issues, // because Resources are immutable. var wg sync.WaitGroup - wg.Add(2) - for i := 0; i < 2; i++ { + wg.Add(1) go func() { defer wg.Done() d := &fakeDetector{} From 485442cd0f4f0e22205e3f7c5c6b47a3c19c5051 Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Mon, 7 Aug 2023 05:15:03 -0500 Subject: [PATCH 06/10] Update sdk/resource/resource_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/resource_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index 12f8925b830..aa5eccaac2e 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -791,7 +791,7 @@ 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 memory-safe. + // no attributes specified. We want to make sure that this is concurrent-safe. return resource.NewWithAttributes("https://opentelemetry.io/schemas/1.3.0"), nil } From b07aabf2711baf91c2ce5cdcd1fabefa00fb5832 Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Mon, 7 Aug 2023 05:15:34 -0500 Subject: [PATCH 07/10] Update sdk/resource/resource_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/resource_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index aa5eccaac2e..2a91ab8b59a 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -772,7 +772,7 @@ func TestWithContainer(t *testing.T) { } func TestResourceRace(t *testing.T) { - // Creating Resources should also be free of any ASAN issues, + // Creating Resources should also be free of any data races, // because Resources are immutable. var wg sync.WaitGroup for i := 0; i < 2; i++ { From c2175401e9e4a56f9e981260a36473ae9de91de7 Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Mon, 7 Aug 2023 05:16:08 -0500 Subject: [PATCH 08/10] Update sdk/resource/resource_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/resource_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/resource/resource_test.go b/sdk/resource/resource_test.go index 2a91ab8b59a..da478047753 100644 --- a/sdk/resource/resource_test.go +++ b/sdk/resource/resource_test.go @@ -771,7 +771,7 @@ func TestWithContainer(t *testing.T) { }, toMap(res)) } -func TestResourceRace(t *testing.T) { +func TestResourceConcurrentSafe(t *testing.T) { // Creating Resources should also be free of any data races, // because Resources are immutable. var wg sync.WaitGroup From a220c4b8f2d9a32a9304a7fc338753be8e6669fb Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Mon, 7 Aug 2023 05:21:54 -0500 Subject: [PATCH 09/10] Add changelog. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2044274f5e..1aaf149a9f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Decouple `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc/internal` from `go.opentelemetry.io/otel/exporters/otlp/internal` and `go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal` using gotmpl. (#4400, #3846) - 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) +- Make Resource detection concurrency-safe. ## [1.16.0/0.39.0] 2023-05-18 From 89fcad9040d8aaaf3715ebd31cf7f77b69810a37 Mon Sep 17 00:00:00 2001 From: Jared Jenkins Date: Mon, 7 Aug 2023 05:38:24 -0500 Subject: [PATCH 10/10] Update CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1aaf149a9f6..4c9833281d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,7 +60,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Decouple `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc/internal` from `go.opentelemetry.io/otel/exporters/otlp/internal` and `go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal` using gotmpl. (#4400, #3846) - 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) -- Make Resource detection concurrency-safe. +- Fix resource detection data race in `go.opentelemetry.io/otel/sdk/resource`. (#4409) ## [1.16.0/0.39.0] 2023-05-18