Skip to content

plugin/grpc: fix span leak and deadline on error attempt#7487

Merged
yongtang merged 1 commit into
coredns:masterfrom
thevilledev:fix/grpc-span-leak
Sep 2, 2025
Merged

plugin/grpc: fix span leak and deadline on error attempt#7487
yongtang merged 1 commit into
coredns:masterfrom
thevilledev:fix/grpc-span-leak

Conversation

@thevilledev

@thevilledev thevilledev commented Aug 28, 2025

Copy link
Copy Markdown
Collaborator

1. Why is this pull request needed and what does it do?

Fixes leaked child tracing spans in grpc.ServeDNS on error paths. Previously spans were not always finished when a proxy attempt failed.

Each attempt now derives a context with a deadline, derived from the overall request timeline. Previously there was no per-RPC deadline and the loop’s timeout was only evaluated after the call returned.

I added a test to validate the behaviour:

  • Both error and success create and finish their child spans
  • Deadline is always set

If you run the test against current master you'll see that it fails:

grpc_test.go:167: expected 2 finished 'query' spans, got 1 (finished: [traceId=43, spanId=46, parentId=44, sampled=true, name=query])

While at it, I also refactored the code to use opentracing.StartSpanFromContext for a bit cleaner span init.

2. Which issues (if any) are related?

None found.

3. Which documentation changes (if any) need to be made?

None. Purely an implementation internal matter.

4. Does this introduce a backward incompatible change or deprecation?

No. Query handling and responses are unchanged; only tracing and timeout handling improved.

Always finish child spans on success and error paths.
Add a test asserting spans finish and deadlines set.

Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
@codecov

codecov Bot commented Aug 28, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.15%. Comparing base (93c57b6) to head (3c55a64).
⚠️ Report is 1601 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7487      +/-   ##
==========================================
+ Coverage   55.70%   60.15%   +4.45%     
==========================================
  Files         224      274      +50     
  Lines       10016    18144    +8128     
==========================================
+ Hits         5579    10914    +5335     
- Misses       3978     6596    +2618     
- Partials      459      634     +175     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thevilledev thevilledev changed the title plugin/grpc: fix span leak on error attempt plugin/grpc: fix span leak and deadline on error attempt Aug 28, 2025
@yongtang yongtang merged commit 21176fb into coredns:master Sep 2, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants