ddtrace/tracer: [bugfix] Atomically load error count when creating child spans

[JDeuce]

What does this PR do?

The error count is written atomically but it can trigger data race when reading here if multiple child spans are created concurrently while errors are happening.

Motivation

This data race excerpt I captured as part of a larger application that does race detection while running complex unit tests on systems that create traces. Somehow it never triggered this with dd-trace-go v1.48.0 but blocks us from updating to v1.49.1.

WARNING: DATA RACE
Write at 0x00c00049c638 by goroutine 1991:
  sync/atomic.AddInt32()
      /usr/local/Cellar/go@1.19/1.19.7/libexec/src/runtime/race_amd64.s:281 +0xb
  sync/atomic.AddInt32()
      <autogenerated>:1 +0x1a
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*span).setTagError()
      /Users/jdeuce/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.49.1/ddtrace/tracer/span.go:284 +0x176
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*span).Finish()


Previous read at 0x00c00049c638 by goroutine 1997:
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.newSpanContext()
      /Users/jdeuce/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.49.1/ddtrace/tracer/spancontext.go:60 +0x27c
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*tracer).StartSpan()
      /Users/jdeuce/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.49.1/ddtrace/tracer/tracer.go:473 +0xcb1
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.StartSpan()
      /Users/jdeuce/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.49.1/ddtrace/tracer/tracer.go:166 +0x8f
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.StartSpanFromContext()

Describe how to test/QA your changes

Locally used a version with this patch and saw the race go away

Reviewer's Checklist

• Changed code has unit tests for its functionality.
• If this interacts with the agent in a new way, a system test has been added.

[JDeuce]

@ahmed-mez 👋 any way you can help me get this reviewed?

[dianashevchenko]

@JDeuce Can you please share the steps to reproduce / regression test to ensure that's the fix or the only fix needed?

[JDeuce]

In testing with a private application it did seem to resolve the race condition.

I think it has to do with potentially continuing to create new child spans after span.Finish(WithError(...)) has been called, we are often creating new child spans within child functions before checking if the context was canceled, and we Finish aggressively before waiting for all child tasks to finish.

[katiehockman]

@JDeuce Sorry for the delay here. From your latest message, it sounds like this PR doesn't fix the issue, is that correct? If that's the case, would you mind testing this with the latest dd-trace-go release and filing an issue so we can keep track of it there? Then we can close this PR and investigate the issue separately. We don't want to lose track of this issue if there is a race condition hidden somewhere.

Thank you!

[darccio]

Closing due to inactivity.