fix(symfony): end sub-request spans in handle post-hook to prevent scope leak#526
Open
BTC-Tim wants to merge 1 commit intoopen-telemetry:mainfrom
Open
fix(symfony): end sub-request spans in handle post-hook to prevent scope leak#526BTC-Tim wants to merge 1 commit intoopen-telemetry:mainfrom
BTC-Tim wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
…ope leak When Symfony renders an error response, HttpKernel::handle() is called twice: once for the MAIN_REQUEST and once internally as a SUB_REQUEST (via ExceptionController). The handle post-hook was returning early for both calls because $exception is null for a successfully-rendered error page, leaving two scopes on the OTEL context stack. When terminate() fired it popped only the topmost scope (the sub-request span), ending that one. The main request scope (the root KIND_SERVER span) was never popped, never ended, and therefore never exported. Fix: detect SUB_REQUEST in the handle post-hook and end the span immediately, since sub-requests never receive a terminate() call. This ensures the context stack contains only the main request scope when terminate() fires. Also wrap $prop->inject() in a try-catch in the terminate hook to guarantee $span->end() is always reached even if response propagation fails. Fixes: open-telemetry/opentelemetry-php#1905
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
|
Author
|
Sorry, this CLA is new for me / our organization. Review of it is in progress, but might take a while. Feel free to close or otherwise change this PR. |
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
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.
Problem
When Symfony renders an error response,
HttpKernel::handle()is called twice: once for theMAIN_REQUESTand once internally as aSUB_REQUESTto render the error page viaExceptionController. Thehandlepost-hook returns early for both calls because$exceptionisnullfor a successfully-rendered error page, leaving two scopes on the OTEL context stack:When
terminate()fires it pops only the topmost scope (scope 1, the sub-request span) and ends it. Scope 2 — the rootKIND_SERVERspan — is never popped, never ended, and never exported.In practice: child spans (e.g. Doctrine queries) appear in the trace backend with
rootServiceName: <root span not yet received>because the parent never arrives.This is distinct from the exception case addressed in #317. The bug occurs on normal request handling whenever Symfony produces a non-2xx response (404, 403, 500, etc.).
Fixes open-telemetry/opentelemetry-php#1905
Solution
Detect
SUB_REQUESTin thehandlepost-hook and end those spans immediately, since sub-requests never receive aterminate()call:By the time
terminate()fires, only scope 2 (the main request span) remains on the stack and is properly ended and exported.Also wraps
$prop->inject()in a try-catch in theterminatehook to ensure$span->end()is always reached if response propagation fails.Tests
test_http_kernel_handle_subrequestto remove the erroneousterminate()call — standalone sub-requests don't receiveterminate()in real Symfony, and the span is now correctly exported from the handle post-hook directly.test_main_request_span_is_exported_when_subrequest_occurs: verifies that after a MAIN_REQUEST + SUB_REQUEST sequence followed byterminate(), both spans are exported, the sub-request span is a child of the main span, and the main span is a root span.test_subrequest_scope_does_not_leak_into_next_request: verifies that a subsequent request after a MAIN_REQUEST + SUB_REQUEST cycle starts with a clean context and produces an independent root span.All 12 tests pass.