fix(lint): clear pre-existing goleak, checklocks and nolint findings - #5126
Merged
Conversation
…sabled TestInternalMetricsDisabled builds tracers with newUnstartedTracer, which leaves keep-alives enabled and triggers the /info agent-discovery request. Without Stop() the idle pooled connection's readLoop/writeLoop goroutines outlive the test, failing TestMain's goleak check whenever a local agent answers on :8126. Pass withNoopInfoHTTPClient() so /info never touches the network, matching the pattern used by other newUnstartedTracer call sites that don't Stop().
…ctive configaudit is a standalone scanner, not a golangci-lint linter, so naming it in //nolint:configaudit made golangci-lint's nolint filter warn about an unknown linter on every run. Give it its own directive so it's invisible to golangci-lint's nolint parsing.
Update the //nolint:configaudit sites to the new //configaudit:ignore directive, and resolve checklocks' "may require annotation" suggestions for traceMaxSize (package-level knob, only mutated by tests) and baseSize (always accessed under otlpTraceWriter.mu, like its sibling fields).
Contributor
Config Audit |
Contributor
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 3f408cc | Docs | Datadog PR Page | Give us feedback! |
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ 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". |
BenchmarksBenchmark execution time: 2026-08-04 14:25:55 Comparing candidate commit 3f408cc in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 326 metrics, 0 unstable metrics, 1 flaky benchmarks without significant changes.
|
Member
Author
|
Admin merged because it doesn't modify production code and these warnings are interfiring our agents. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cleans up four small pre-existing, non-gating issues on
main— none of these currently fail CI, but they're noise that trains reviewers to ignore linter/goleak output.TestInternalMetricsDisabledbuilds tracers withnewUnstartedTracer, which leaves HTTP keep-alives enabled and triggers the/infoagent-discovery request. WithoutStop(), the idle pooled connection'sreadLoop/writeLoopgoroutines outlive the test, soTestMain'sgoleak.Find()fails whenever a local agent answers on:8126(see CONTRIBUTING.md § Goroutine Leaks). Bisected via-run/-skip: this is the only leaking test in the package. Fixed by passing the existingwithNoopInfoHTTPClient()helper, matching othernewUnstartedTracercall sites that don'tStop().configaudit(our own env-var migration scanner inscripts/configaudit) was suppressed via//nolint:configaudit, but it isn't a golangci-lint linter, so the nolint filter logsFound unknown linters in //nolint directives: configauditon every run — and there's no config knob to allow-list it. Gave it its own//configaudit:ignoredirective instead, invisible to golangci-lint's nolint parsing.may require checklocks annotation for mu, used with lock held 100% of the timefortraceMaxSize(package-level knob, only mutated by tests — annotated+checklocksignore) andotlpTraceWriter.baseSize(always accessed underw.mu, like its sibling fieldsspans/buffSize— annotated+checklocks:mu).golangci-lint run ./...was already0 issuesbefore this change;scripts/checklocks.shalready exits 0 (suggestions don't fail it). This PR removes the warning noise and the goleak failure rather than fixing a gating check.Test plan
GOTOOLCHAIN=go1.25.0 go test ./ddtrace/tracer/... -count=1→ ok (goleak passes with a local agent on:8126)GOTOOLCHAIN=go1.25.0 go test ./ddtrace/tracer/ -run '^TestInternalMetricsDisabled$' -v→ all 3 subtests passGOTOOLCHAIN=go1.25.0 ./bin/golangci-lint run ./...→0 issues., no more nolint_filter warningGOTOOLCHAIN=go1.25.0 ./scripts/checklocks.sh ./ddtrace/tracer→No errors found(0 suggestions)(cd scripts/configaudit && GOWORK=off go test ./...)→ ok(cd scripts/configaudit && GOWORK=off go run . -root ../.. -package ddtrace/tracer -format table)→DD_ENV/DD_VERSION/DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLEDcorrectly absent (still suppressed)