contrib: fix span start option races

[eliottness]

What does this PR do?

Fix data race from closure on the span options used when starting spans in multiple contribs.

This PR introduces a new package contrib/internal/options with a method called Copy which provides a newly allocated options slice with a copy of the provides options inside it.

Note: This PR does not fix the scenario where a customer keeps a reference to the options they pass to their contrib setup and then alters them later on, which would have the side effect of altering the options in the generated config. That could theoretically create a race condition, but it's not supported behavior. This PR just focused on race conditions caused by our library.

Motivation

This PR originally fixes a data race found on github.com/go-chi.chi.v5 contrib while running some -race tests on dd-source. It was then discovered that a lot of contrib's had the same issue. I noticed 2 mechanisms on every contrib when buildings options to start a new span: either we create a new array and append the global options into it (no issue with this mechanism), or we append options directly into the cfg.spanOpts slice. This second approach could lead to data races in case the array has enough capacity to append in-place the new element appended.

Reviewer's Checklist

• Changed code has unit tests for its functionality at or near 100% coverage.
• System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
• There is a benchmark for any new code, or changes to existing code.
• If this interacts with the agent in a new way, a system test has been added.
• Add an appropriate team label so this PR gets put in the right place for the release notes.

For Datadog employees:

• If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
• This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

[pr-commenter]

Benchmarks

Benchmark execution time: 2023-12-12 18:29:22

Comparing candidate commit 3c089f2 in PR branch eliott.bouhana/fix-span-opts-race with baseline commit aada6eb in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics.

[RomainMuller]

Most likely actually not an issue here, but I'd want to point out that this change affectes the order of the options, which could affect the end result...

[Julio-Guerra]

Yes, this is why a "builder" would be preferable to enforce this too, whereas every contrib is doing its own thing at the moment, and some of them have ordering tests while others don't.

This is a known limitation of the current design that we discussed in the past here #1352

[katiehockman]

It was difficult to use a builder here, since the builder just ended up being shared as well, which led to the same problem. At least for now, I made a helper method to make those copies more explicit, so I'm hoping that'll help a bit with readability. It doesn't fully prevent this in the future, but it does make it more explicit.

[katiehockman]

I've gone ahead and made several changes, and fixed additional races. Given how much of the code I've authored, I shouldn't be the one to approve this, so will seek an additional review from someone else on the team.

[knusbaum]

Looks good, just a few comments about some unnecessary copies.

Only the GRPC one looks like it could actually have a performance impact on customers. The rest are fine either way.

[knusbaum]

Found some changes that we need to make. Not all of them need to go in here, but some of these need to be modified.

[Julio-Guerra]

While I was reading some contribs changed in this PR, I think we could go further to help thanks to some ImmutableOptions wrapper type.

For example avoiding:

func Middleware(opts ...Options) Handler {
    opts = options.Copy(opts) // problem avoided here
    opts = append(opts, foo)

    return func(w http.ResponseWriter, r *http.Request) {
        opts = append(opts, headersTags(r)) // problem not avoided here
    }
}

And doing instead:

func Middleware(opts ...Options) Handler {
    localOpts = options.Copy(opts) // problem avoided here
    localOpts = append(localOpts, foo)
    opts := options.ImmutableOptions(localOpts)
    return func(w http.ResponseWriter, r *http.Request) {
        opts = append(opts, headersTags(r)) // problem not avoided here
    }
}

Or having a full wrapper type Options with an Append method so it can do more safety checks?

My point is: how can we further improve this API to avoid future mistakes again and maybe add this ability to block any further uses of append once we know we are done and anything coming later is in a request handler, from concurrent goroutines.

cc @knusbaum

[knusbaum]

Abstracting the mutations is potentially a good idea, but we would have to be careful to do it in an efficient way so that every Append doesn't cause a copy.

[katiehockman]

We should consider it, but let's do so in a follow-up PR. I'd like to keep this PR as minimal as possible given the already large blast radius of changes.

[katiehockman]

I don't think we need this one anymore.

[knusbaum]

Approved.