fix(symfony): clean up sub-request scope and record exceptions on spans#662
fix(symfony): clean up sub-request scope and record exceptions on spans#662PuvaanRaaj wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #662 +/- ##
============================================
- Coverage 81.69% 80.77% -0.92%
+ Complexity 1821 1448 -373
============================================
Files 129 95 -34
Lines 7144 5347 -1797
============================================
- Hits 5836 4319 -1517
+ Misses 1308 1028 -280 Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b6543922d
ℹ️ 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".
| if ($isSubRequest && null !== $response) { | ||
| $span->setAttribute(TraceAttributes::HTTP_RESPONSE_STATUS_CODE, $response->getStatusCode()); |
There was a problem hiding this comment.
When a Symfony error-controller sub-request returns a 500 response without throwing, this new post-hook path is now the only place that completes that sub-request span, but it only records http.response.status_code before ending it. The terminate() path still marks 5xx responses with STATUS_ERROR, so moving sub-request completion here causes exported 500 error-controller spans to have an unset status and lose the error signal.
Useful? React with 👍 / 👎.
Detach the scope attached by the kernel handle hook on every request type so main-request root spans end and export (#1905); record exceptions surfaced during (sub-)request handling on the active span (#1844). Also resolves pre-existing Symfony psalm CI failures in the files touched here: add the missing @psalm-suppress UnusedFunctionCall on the terminate() hook, and add open-telemetry/opentelemetry-propagation-traceresponse to require-dev (plus the use import) so SymfonyInstrumentationTest can resolve TraceResponsePropagator. Fixes open-telemetry/opentelemetry-php#1905 and open-telemetry/opentelemetry-php#1844. Risk-Level: medium AI-Agent: claude-opus-4-8
1b65439 to
29dde05
Compare
1 participant
Summary
Fixes two related bugs in the Symfony auto-instrumentation
HttpKernel::handle()hook lifecycle, both rooted in the same sub-request handling code path.Root cause
Symfony calls
HttpKernel::handle()twice when it renders an error response internally: once for theMAIN_REQUESTand again as a nestedSUB_REQUESTdispatched to the error controller. The oldhandlepost-hook returned early whenever there was no exception:The error-controller sub-request renders successfully (no exception), so its span was never ended and its scope was never detached. This left the sub-request scope leaked on top of the context stack.
terminate()popped only that topmost (sub-request) scope. The main-request rootSERVERspan was never popped, ended, or exported, so child spans showedrootServiceName: <root span not yet received>.handleThrowablerecords the exception on the currently-active span (the main span) before the error sub-request is dispatched. Because the leaked sub-request scope displaced the main scope,terminate()ended the wrong span and the main span — the one carrying the exception event andStatus: Error— was never exported.The fix
In the
handlepost-hook:MAIN_REQUESTthat returned normally still returns early and is ended by theterminatehook (unchanged behaviour).handle()— now detaches its scope and ends its span in the post-hook, since neither is ever passed toterminate(). This ensures every attached scope is detached exactly once and no scope leaks onto the context stack.recordException+STATUS_ERROR) on the active span when present.With the sub-request scope cleaned up immediately,
terminate()sees the main-request scope again and ends/exports the root span with its exception event intact.Testing
test_http_kernel_sub_request_scope_does_not_leak_main_span: a main request followed by an internal sub-request leaves no leaked scope; the sub-request span exports immediately and the main rootSERVERspan is ended and exported byterminate().test_http_kernel_records_exception_on_main_span_during_sub_request_error_handling: an exception recorded on the active span during error handling ends up on the exported main span.terminate()after aSUB_REQUESThandle (Symfony never does this for sub-requests; the span is now ended by the post-hook).vendor/bin/phpunit→ 24 tests, 132 assertions, OK.php-cs-fixer --dry-runclean.Fixes open-telemetry/opentelemetry-php#1905
Fixes open-telemetry/opentelemetry-php#1844