ddtrace/tracer: fix concurrent map writes when applying trace sampling rules and setting tags concurrently #2727
Conversation
…g rules and setting tags concurrently
BenchmarksBenchmark execution time: 2024-06-04 11:18:03 Comparing candidate commit 9b132d7 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 42 metrics, 1 unstable metrics. scenario:BenchmarkStartSpanConcurrent-24
|
| // not (through tracer.Inject when trace sampling rules are in place) does not cause | ||
| // concurrent map writes. It seems to only be consistently reproduced with the -count=100 | ||
| // flag when running go test, but it's a good test to have. | ||
| func TestConcurrentSpanSetTag(t *testing.T) { |
There was a problem hiding this comment.
Yeah, if I revert the fix and run this with -count=1 under the race detector, it passes. But if I do -count=2, it fails... It fails consistently if I swap the go statements (so the Inject one is first, SetTag is second. Strange! It also fails consistently if I do this:
func TestConcurrentSpanSetTag(t *testing.T) {
testConcurrentSpanSetTag(t)
testConcurrentSpanSetTag(t)
}
func testConcurrentSpanSetTag(t *testing.T) {
// the actual implementation
}
I don't have any good ideas for why it doesn't trigger the failure the way you wrote it. But one of the things I did should hopefully catch the bug consistently without requiring -count > 1
There was a problem hiding this comment.
That's really curious, good catch! Although it doesn't fail consistently in my machine, it fails more often. I updated the code to reflect your suggestions. It won't be a flaky test because running it with my fix under the race detecter and -count=100 doesn't trigger any error.
Pull Request is not mergeable
…g rules and setting tags concurrently (#2727)
…g rules and setting tags concurrently (#2727) (#2734) This backports #2727 to the v1.64 release branch. Co-authored-by: Dario Castañé <[email protected]>
What does this PR do?
Fixes a race condition bug reported by TEE, where a span was being modified concurrently to inject context and to set tags and trace sampling rules are in place.
Motivation
Avoid race conditions in our code and improve reliability of our tracer.
Reviewer's Checklist
Unsure? Have a question? Request a review!