tracer: Check for nil interface types in span.Finish(WithError())

[ajgajg1134]

What does this PR do?

Fixes a bug where if a user created a nil type of their own implementation of the error interface the nil check we do is not sufficient to ensure we do not panic later. (For context checkout this old thread which includes a playground link)

Motivation

Closes #2029

Describe how to test/QA your changes

Covered with Unit test

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.

[pr-commenter[bot]]

Benchmarks

Benchmark execution time: 2024-07-10 08:27:43

Comparing candidate commit 817c87de441754e7d4b8ee6b3543316f19c8b7a9 in PR branch andrew.glaude/nilSpanError with baseline commit a20143b9a340309f97a0ac90e90960d3bf753f76 in branch main.

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

scenario:BenchmarkTracerAddSpans-24

• 🟥 allocated_mem [+2.377KB; +2.377KB] or [+103.890%; +103.890%]
• 🟥 allocations [+16; +16] or [+64.000%; +64.000%]
• 🟥 execution_time [+8.946µs; +9.042µs] or [+219.229%; +221.574%]

[katiehockman]

@ajgajg1134 did you ultimately decide to abandon this PR, or do you still want review in it's current state? I remember there being discussion, but don't recall our final decision on that.

[ajgajg1134]

@ajgajg1134 did you ultimately decide to abandon this PR, or do you still want review in it's current state? I remember there being discussion, but don't recall our final decision on that.

I believe we decided to move forward with our best effort approach here, but I had some unresolved (and unposted) comments in my review here from our discussion. I've just posted and addressed them so I would say this is ready for final review.

[ajgajg1134]

In looking at this again I realized our existing benchmarks didn't actually really cover this WithError case. Modifying the benchmark to create only non-nil error spans gives the following:

benchstat original.out withReflect.out
goos: darwin
goarch: arm64
pkg: gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer
                  │ original.out │          withReflect.out          │
                  │    sec/op    │   sec/op     vs base              │
TracerAddSpans-10    5.013µ ± 1%   5.047µ ± 0%  +0.68% (p=0.035 n=6)

This seems like a reasonable change to me? Less than 1% and it's only incurred when the error is non-nil

[katiehockman]

I realized our existing benchmarks didn't actually really cover this WithError case.

@ajgajg1134 do you want to add those benchmarks here, so we don't lose them? Agreed that 1% isn't a big enough concern to worry about here, so should be good to merge it.

[darccio]

After some consideration and more discussion, we've decided to close this PR and the related issue. It shows a consistent impact in performance, as shown here.