fix(ci-visibility): harden mocktracer, HTTP bodies, and EFD skips#4745
Conversation
0d1dcee to
91d1828
Compare
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 73a66d3 | Docs | Datadog PR Page | Give us feedback! |
171ce2d to
e0158d4
Compare
e0158d4 to
deb1b21
Compare
BenchmarksBenchmark execution time: 2026-05-12 14:06:20 Comparing candidate commit 73a66d3 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 273 metrics, 2 unstable metrics, 1 known flaky benchmarks.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f98d3b0541
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
5fd6d9c
into
main
What does this PR do?
This PR fixes three CI Visibility issues found while validating Test Optimization in downstream repositories:
mocktracerwhen CI Visibility starts after a package has already calledmocktracer.Start().For the mocktracer startup path,
tracer.Start()now keeps the existingcivisibilitymocktracerinstalled as the process global tracer when CI Visibility starts later. Instead of replacing that mock-compatible wrapper, it injects the real CI Visibility tracer into it. Normal application spans keep going to the mock tracer, while CI Visibility test/session/module/suite spans go to the real tracer.The PR also adds a finished-chunk submit hook so CI Visibility chunks are still submitted through the real tracer even though the process global tracer remains the mock wrapper. The mock wrapper now owns its real tracer delegate explicitly, protects delegate replacement and span routing with locks, keeps real-span routing state instance-local, and preserves shutdown behavior so user
mocktracer.Stop()calls do not kill CI Visibility before package exit.For the HTTP cleanup path, the CI Visibility trace writer now owns every non-nil body returned by
ddTransport.send. It drains and closes those bodies on success and on the defensive(body, err)path, preserving retry behavior while allowing the HTTP transport to release keep-alive connections.For the EFD skip path, clean skips now stop at the common EFD retry decision. That keeps pass/fail EFD retries unchanged, but avoids burning the retry budget on executions that only produce
skipsignal. The final-status prediction mirrors the same decision so the single skipped execution receivestest.final_status=skipimmediately and does not get retry tags.No new environment variables are introduced, and this does not change
/civisibilityor/internal/civisibility.Motivation
The downstream mocktracer failure happens when test packages start mocktracer in
TestMain, and the test optimizer later starts CI Visibility throughRunM.Before this change, CI Visibility startup replaced the global
civisibilitymocktracerwith either the real*tracer.traceror the CI Visibility noop wrapper. Code already using the v1 mocktracer path still expected the global tracer to be mock-compatible, so finishing adapted mock spans could panic with an interface conversion error.Preserving the wrapper fixes that ownership problem without losing CI Visibility data: user test spans remain available through
FinishedSpans(), and CI Visibility spans still reach the real tracer and submit to the test-cycle intake.A later downstream run also exposed a goleak failure with
net/http.(*persistConn).readLoopandnet/http.(*persistConn).writeLoopafter tests had passed. The CI Visibility writer was ignoring successful response bodies returned by the transport, leaving the client connection in use. Draining and closing those bodies makes shutdown deterministic for the HTTP transport.Downstream EFD validation also showed output like
skipped after 10 retries by Datadog's early flake detection. Retrying a clean skipped test does not add useful flakiness signal, so this PR stops scheduling EFD retries when the first execution isskipped && !failed.Testing
go test ./ddtrace/mocktracer -count=1go test ./ddtrace/mocktracer -race -count=1go test ./ddtrace/mocktracer -race -count=3go test ./ddtrace/mocktracer -run 'TestCIVisibilityMockTracer' -count=10go test ./ddtrace/tracer -run 'TestSetGlobalTracerPreservingCIVisibilityMockTracer|TestGlobalTracer' -count=1go test ./ddtrace/tracer -run 'TestSetGlobalTracerPreservingCIVisibilityMockTracer|TestGlobalTracer' -race -count=10go test ./ddtrace/tracer -run 'TestCIVisibilityImplementsTraceWriter|TestCiVisibilityTraceWriterFlushRetries|TestCiVisibilityTraceWriterClosesHTTPResponseBody' -count=1go test ./ddtrace/tracer -run 'TestCIVisibilityImplementsTraceWriter|TestCiVisibilityTraceWriterFlushRetries|TestCiVisibilityTraceWriterClosesHTTPResponseBody' -race -count=1go test ./ddtrace/tracer -run 'TestCIVisibility|TestCiVisibility' -count=1go test ./internal/civisibility/integrations/gotesting -count=1go test ./internal/civisibility/integrations/gotesting/subtests -count=1go test ./internal/civisibility/integrations/gotesting/failnow -count=1git diff --checkLocal note: full
go test ./ddtrace/tracer -count=1is blocked in this checkout by environment-sensitive DogStatsD autodetection expectations. The local agent reports DogStatsD port8135, while existing tracer tests expect8125in startup-log/default-config assertions. The focused tracer tests for these changes pass.Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.