fix(http): Prevent concurrent read/writes on http client trace timings#4591
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files🚀 New features to boost your workflow:
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: e97b3d0 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
BenchmarksBenchmark execution time: 2026-03-26 16:44:25 Comparing candidate commit e97b3d0 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 215 metrics, 9 unstable metrics.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| t.mu.Lock() | ||
| defer t.mu.Unlock() |
There was a problem hiding this comment.
Yes, let's add checklocks annotations to the struct. I wonder why it didn't complaint about this
There was a problem hiding this comment.
Looking good. Let's add the checklocks annotations.
On top of that we can consider using atomic.Pointer[time.Time]{} instead of sharing a single lock. In high throughput services this lock can create contention.
Now that I thought about this again, I think this will create additional allocations without a guaranteed gain (we need to measure first), even though multiple goroutines involved in this path the struct is per request,so let's stick with the mutex.
| t.mu.Lock() | ||
| defer t.mu.Unlock() |
There was a problem hiding this comment.
Yes, let's add checklocks annotations to the struct. I wonder why it didn't complaint about this
What does this PR do?
Adds a sync.Mutex on httpTraceTimings struct.
Motivation
Data race on the struct.
http://github.com/DataDog/dd-trace-go/issues/4525/
Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.Unsure? Have a question? Request a review!