sdk/resource: Fix data race with emptyAttributes

[jaredjenkins]

If mutliple instantiations of a Resources struct are happening in parallel, they can end up modifying the same underlying resource.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jaredjenkins / name: Jared Jenkins (eb02f96, 8c2aa29, 5371689, 1d46dee, ed488f8, d720ac6, 485442c, b07aabf, 16c4d18, c217540, a220c4b, 89fcad9)
• ✅ login: pellared / name: Robert Pająk (d720ac6, 485442c, b07aabf, c217540, 89fcad9)
• ✅ login: MrAlias / name: Tyler Yahn (95eb62b, d111810)

[MrAlias]

...they can end up modifying the same underlying resource.

How? Resources are immutable?

[jaredjenkins]

...they can end up modifying the same underlying resource.

How? Resources are immutable?

If you have two different goroutines trying to create Resources, it looks like they can end up merging into the emptyResource. Here is my sanitized stack trace:

==================
WARNING: DATA RACE
Write at 0x00010a8a2470 by goroutine 45:
  go.opentelemetry.io/otel/sdk/resource.NewWithAttributes()
      /Users/jared/.gvm/pkgsets/go1.20.1/global/pkg/mod/go.opentelemetry.io/otel/sdk@v1.16.0/resource/resource.go:63 +0x36c
  go.opentelemetry.io/otel/sdk/resource.Merge()
      /Users/jared/.gvm/pkgsets/go1.20.1/global/pkg/mod/go.opentelemetry.io/otel/sdk@v1.16.0/resource/resource.go:191 +0x84
  go.opentelemetry.io/otel/sdk/resource.detect()
      /Users/jared/.gvm/pkgsets/go1.20.1/global/pkg/mod/go.opentelemetry.io/otel/sdk@v1.16.0/resource/auto.go:73 +0x208
  go.opentelemetry.io/otel/sdk/resource.New()
      /Users/jared/.gvm/pkgsets/go1.20.1/global/pkg/mod/go.opentelemetry.io/otel/sdk@v1.16.0/resource/resource.go:54 
// OMITTED

Previous read at 0x00010a8a2470 by goroutine 44:
  go.opentelemetry.io/otel/sdk/resource.detect()
      /Users/jared/.gvm/pkgsets/go1.20.1/global/pkg/mod/go.opentelemetry.io/otel/sdk@v1.16.0/resource/auto.go:77 +0x2f8
  go.opentelemetry.io/otel/sdk/resource.New()
      /Users/jared/.gvm/pkgsets/go1.20.1/global/pkg/mod/go.opentelemetry.io/otel/sdk@v1.16.0/resource/resource.go:54 +0x1bc

My calling code inits a resource in the test but otherwise there is no shared state.

[jaredjenkins]

@MrAlias it looks like this is actually stemming from the writing of the schemaVersion field:

func NewWithAttributes(schemaURL string, attrs ...attribute.KeyValue) *Resource {
	resource := NewSchemaless(attrs...)
	resource.schemaURL = schemaURL
	return resource
}

in my case, auto.go takes a copy of the emptyResource struct while we're attempting to write to it's schema version. In any case, making emptyResponse a global doesn't seem worth it?

[MrAlias]

@MrAlias it looks like this is actually stemming from the writing of the schemaVersion field:

func NewWithAttributes(schemaURL string, attrs ...attribute.KeyValue) *Resource {
	resource := NewSchemaless(attrs...)
	resource.schemaURL = schemaURL
	return resource
}

in my case, auto.go takes a copy of the emptyResource struct while we're attempting to write to it's schema version. In any case, making emptyResponse a global doesn't seem worth it?

Ah, gotcha 👍. Thanks for catching this.

Please be sure to sign the CLA so we can accept these changes.

[jaredjenkins]

Yep. I am waiting on my employer to give the go ahead.

…

On Fri, Aug 4, 2023 at 12:15 PM Tyler Yahn ***@***.***> wrote: @MrAlias <https://github.com/MrAlias> it looks like this is actually stemming from the writing of the schemaVersion field: func NewWithAttributes(schemaURL string, attrs ...attribute.KeyValue) *Resource { resource := NewSchemaless(attrs...) resource.schemaURL = schemaURL return resource } in my case, auto.go takes a copy of the emptyResource struct while we're attempting to write to it's schema version. In any case, making emptyResponse a global doesn't seem worth it? Ah, gotcha 👍. Thanks for catching this. Please be sure to sign the CLA so we can accept these changes. — Reply to this email directly, view it on GitHub <#4409 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB74BWXN2FM3YHUFL32JC3XTUU3HANCNFSM6AAAAAA3ELPUHM> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[codecov]

Codecov Report

Merging #4409 (d111810) into main (e1e6b96) will decrease coverage by 0.1%.
The diff coverage is 75.0%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #4409     +/-   ##
=======================================
- Coverage   83.5%   83.5%   -0.1%     
=======================================
  Files        229     229             
  Lines      18653   18653             
=======================================
- Hits       15586   15584      -2     
- Misses      2749    2751      +2     
  Partials     318     318             

Files Changed	Coverage Δ
sdk/resource/resource.go	76.5% <75.0%> (ø)

... and 1 file with indirect coverage changes

[jaredjenkins]

Got a repro of the failure without the change:

==================
WARNING: DATA RACE
Write at 0x00010174d090 by goroutine 143:
  go.opentelemetry.io/otel/sdk/resource.NewWithAttributes()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/resource.go:63 +0x5c
  go.opentelemetry.io/otel/sdk/resource_test.fakeDetector.Detect()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/resource_test.go:797 +0x34
  go.opentelemetry.io/otel/sdk/resource_test.(*fakeDetector).Detect()
      <autogenerated>:1 +0x20
  go.opentelemetry.io/otel/sdk/resource.detect()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/auto.go:66 +0xd8
  go.opentelemetry.io/otel/sdk/resource.Detect()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/auto.go:50 +0xc0
  go.opentelemetry.io/otel/sdk/resource_test.TestResourceRace.func1()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/resource_test.go:785 +0x7c

Previous write at 0x00010174d090 by goroutine 144:
  go.opentelemetry.io/otel/sdk/resource.NewWithAttributes()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/resource.go:63 +0x5c
  go.opentelemetry.io/otel/sdk/resource_test.fakeDetector.Detect()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/resource_test.go:797 +0x34
  go.opentelemetry.io/otel/sdk/resource_test.(*fakeDetector).Detect()
      <autogenerated>:1 +0x20
  go.opentelemetry.io/otel/sdk/resource.detect()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/auto.go:66 +0xd8
  go.opentelemetry.io/otel/sdk/resource.Detect()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/auto.go:50 +0xc0
  go.opentelemetry.io/otel/sdk/resource_test.TestResourceRace.func1()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/resource_test.go:785 +0x7c

Goroutine 143 (running) created at:
  go.opentelemetry.io/otel/sdk/resource_test.TestResourceRace()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/resource_test.go:782 +0x58
  testing.tRunner()
      /Users/jared/.gvm/gos/go1.20.1/src/testing/testing.go:1576 +0x180
  testing.(*T).Run.func1()
      /Users/jared/.gvm/gos/go1.20.1/src/testing/testing.go:1629 +0x40

Goroutine 144 (finished) created at:
  go.opentelemetry.io/otel/sdk/resource_test.TestResourceRace()
      /Users/jared/go/src/github.com/open-telemetry/opentelemetry-go/sdk/resource/resource_test.go:782 +0x58
  testing.tRunner()
      /Users/jared/.gvm/gos/go1.20.1/src/testing/testing.go:1576 +0x180
  testing.(*T).Run.func1()
      /Users/jared/.gvm/gos/go1.20.1/src/testing/testing.go:1629 +0x40
==================
--- FAIL: TestResourceRace (0.00s)
    testing.go:1446: race detected during execution of test

[jaredjenkins]

Yep. I am waiting on my employer to give the go ahead.
…
On Fri, Aug 4, 2023 at 12:15 PM Tyler Yahn @.*> wrote: @MrAlias https://github.com/MrAlias it looks like this is actually stemming from the writing of the schemaVersion field: func NewWithAttributes(schemaURL string, attrs ...attribute.KeyValue) Resource { resource := NewSchemaless(attrs...) resource.schemaURL = schemaURL return resource } in my case, auto.go takes a copy of the emptyResource struct while we're attempting to write to it's schema version. In any case, making emptyResponse a global doesn't seem worth it? Ah, gotcha 👍. Thanks for catching this. Please be sure to sign the CLA so we can accept these changes. — Reply to this email directly, view it on GitHub <#4409 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB74BWXN2FM3YHUFL32JC3XTUU3HANCNFSM6AAAAAA3ELPUHM . You are receiving this because you modified the open/close state.Message ID: @.>

Should be resolved by Monday. We've already signed the CLA for corporate contributions.

[pellared]

Please update the changelog.

In general looks OK 👍

[jaredjenkins]

Thanks for the guidance @pellared!