From 29dde05661c5edbb99e1ed18b6055634df68d807 Mon Sep 17 00:00:00 2001 From: PuvaanRaaj <112681813+PuvaanRaaj@users.noreply.github.com> Date: Sat, 18 Jul 2026 16:05:15 +0800 Subject: [PATCH] fix(symfony): clean up sub-request scope and record exceptions on spans 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 --- src/Instrumentation/Symfony/composer.json | 1 + .../Symfony/src/SymfonyInstrumentation.php | 29 ++++++- .../SymfonyInstrumentationTest.php | 80 ++++++++++++++++--- 3 files changed, 95 insertions(+), 15 deletions(-) diff --git a/src/Instrumentation/Symfony/composer.json b/src/Instrumentation/Symfony/composer.json index 83e69d0c5..fd6cd84db 100644 --- a/src/Instrumentation/Symfony/composer.json +++ b/src/Instrumentation/Symfony/composer.json @@ -27,6 +27,7 @@ "phpstan/phpstan": "^1.1", "phpstan/phpstan-phpunit": "^1.0", "psalm/plugin-phpunit": "^0.19.2", + "open-telemetry/opentelemetry-propagation-traceresponse": "^0.3", "open-telemetry/sdk": "^1.8", "phpunit/phpunit": "^9.5", "vimeo/psalm": "6.4.0", diff --git a/src/Instrumentation/Symfony/src/SymfonyInstrumentation.php b/src/Instrumentation/Symfony/src/SymfonyInstrumentation.php index 11e4ce2cf..16e36004b 100644 --- a/src/Instrumentation/Symfony/src/SymfonyInstrumentation.php +++ b/src/Instrumentation/Symfony/src/SymfonyInstrumentation.php @@ -94,18 +94,41 @@ public static function register(): void ?\Throwable $exception ): void { $scope = Context::storage()->scope(); - if (null === $scope || null === $exception) { + if (null === $scope) { + return; + } + + $type = $params[1] ?? HttpKernelInterface::MAIN_REQUEST; + $isSubRequest = ($type === HttpKernelInterface::SUB_REQUEST); + + // A MAIN_REQUEST that didn't throw is ended by the `terminate` hook. + if (!$isSubRequest && null === $exception) { return; } $span = Span::fromContext($scope->context()); $scope->detach(); - $span->recordException($exception); - $span->setStatus(StatusCode::STATUS_ERROR, $exception->getMessage()); + + if (null !== $exception) { + $span->recordException($exception); + $span->setStatus(StatusCode::STATUS_ERROR, $exception->getMessage()); + } + + if ($isSubRequest && null !== $response) { + $span->setAttribute(TraceAttributes::HTTP_RESPONSE_STATUS_CODE, $response->getStatusCode()); + } + + // We only reach this point for sub-requests or when an exception propagated + // out of handle(); neither is ever passed to `terminate()`, so the span must + // be ended here. Leaving a sub-request span open leaks its scope onto the + // context stack and prevents the main-request root span from being ended and + // exported (#1905). Only a MAIN_REQUEST that returned normally is left for + // `terminate()`, and that path already returned above. $span->end(); } ); + /** @psalm-suppress UnusedFunctionCall */ hook( HttpKernel::class, 'terminate', diff --git a/src/Instrumentation/Symfony/tests/Integration/SymfonyInstrumentationTest.php b/src/Instrumentation/Symfony/tests/Integration/SymfonyInstrumentationTest.php index 0ffc8e328..6d34906f7 100644 --- a/src/Instrumentation/Symfony/tests/Integration/SymfonyInstrumentationTest.php +++ b/src/Instrumentation/Symfony/tests/Integration/SymfonyInstrumentationTest.php @@ -6,6 +6,7 @@ use OpenTelemetry\API\Trace\SpanKind; use OpenTelemetry\API\Trace\StatusCode; +use OpenTelemetry\Contrib\Propagation\TraceResponse\TraceResponsePropagator; use OpenTelemetry\SemConv\TraceAttributes; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\EventDispatcher\EventDispatcherInterface; @@ -143,8 +144,9 @@ public function test_http_kernel_handle_subrequest(): void $request = new Request(); $request->attributes->set('_controller', 'ErrorController'); - $response = $kernel->handle($request, HttpKernelInterface::SUB_REQUEST); - $kernel->terminate($request, $response); + // Sub-requests are never passed to terminate() by Symfony; their span is ended + // by the handle post-hook, so it is exported as soon as handle() returns. + $kernel->handle($request, HttpKernelInterface::SUB_REQUEST); $this->assertCount(1, $this->storage); @@ -160,8 +162,7 @@ public function test_http_kernel_handle_subrequest_with_various_controller_types // String controller $request = new Request(); $request->attributes->set('_controller', 'SomeController::index'); - $response = $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); - $kernel->terminate($request, $response); + $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); $this->assertSame('GET SomeController::index', $this->storage[0]->getName()); $this->storage->exchangeArray([]); @@ -169,16 +170,14 @@ public function test_http_kernel_handle_subrequest_with_various_controller_types $controllerObj = new class() {}; $request = new Request(); $request->attributes->set('_controller', [$controllerObj, 'fooAction']); - $response = $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); - $kernel->terminate($request, $response); + $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); $this->assertSame('GET ' . get_class($controllerObj) . '::fooAction', $this->storage[0]->getName()); $this->storage->exchangeArray([]); // Array: [class, method] $request = new Request(); $request->attributes->set('_controller', ['SomeClass', 'barAction']); - $response = $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); - $kernel->terminate($request, $response); + $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); $this->assertSame('GET SomeClass::barAction', $this->storage[0]->getName()); $this->storage->exchangeArray([]); } @@ -194,15 +193,14 @@ public function test_http_kernel_handle_subrequest_with_null_and_object_controll $controllerObj2 = new class() {}; $request = new Request(); $request->attributes->set('_controller', $controllerObj2); - $response = $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); - $kernel->terminate($request, $response); + $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); $this->assertSame('GET sub-request', $this->storage[0]->getName()); + $this->storage->exchangeArray([]); // Null/other controller (should fallback to 'sub-request') $request = new Request(); $request->attributes->set('_controller', null); - $response = $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); - $kernel->terminate($request, $response); + $kernel->handle($request, \Symfony\Component\HttpKernel\HttpKernelInterface::SUB_REQUEST); $this->assertSame('GET sub-request', $this->storage[0]->getName()); } @@ -237,6 +235,64 @@ public function test_http_kernel_handle_5xx_response_is_error(): void $this->assertSame(500, $span->getAttributes()->get(TraceAttributes::HTTP_RESPONSE_STATUS_CODE)); } + public function test_http_kernel_sub_request_scope_does_not_leak_main_span(): void + { + // Simulates Symfony's internal error-controller dispatch: a MAIN_REQUEST handle() + // call whose exception is caught internally, followed by a nested SUB_REQUEST + // handle() call that renders the error response successfully (no exception). + $kernel = $this->getHttpKernel(new EventDispatcher(), fn () => new Response('error page')); + $this->assertCount(0, $this->storage); + + $mainRequest = new Request(); + $mainRequest->attributes->set('_route', 'main_route'); + $kernel->handle($mainRequest, HttpKernelInterface::MAIN_REQUEST, false); + + // The sub-request is dispatched while the main request span's scope is still on + // the context storage stack, exactly as Symfony's ErrorListener does it. + $subRequest = new Request(); + $subRequest->attributes->set('_controller', 'ErrorController'); + $subResponse = $kernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST); + + // The sub-request span must be ended and exported immediately: it never reaches + // `terminate()`, so leaving its scope attached would leak it and block the main + // request span from ever being reached again. + $this->assertCount(1, $this->storage); + $this->assertSame('GET ErrorController', $this->storage[0]->getName()); + $this->assertSame(SpanKind::KIND_INTERNAL, $this->storage[0]->getKind()); + + // `terminate()` must now see the main request's scope, not a leaked sub-request one. + $kernel->terminate($mainRequest, $subResponse); + + $this->assertCount(2, $this->storage); + $mainSpan = $this->storage[1]; + $this->assertSame('GET main_route', $mainSpan->getName()); + $this->assertSame(SpanKind::KIND_SERVER, $mainSpan->getKind()); + } + + public function test_http_kernel_records_exception_on_main_span_during_sub_request_error_handling(): void + { + $kernel = $this->getHttpKernel(new EventDispatcher(), fn () => new Response('error page')); + + $mainRequest = new Request(); + $kernel->handle($mainRequest, HttpKernelInterface::MAIN_REQUEST, false); + + // Symfony's HttpKernel::handleThrowable() records the exception on whatever span + // is currently active before it dispatches the internal error sub-request; at + // that point the only span on the stack is the main request's. + \OpenTelemetry\API\Trace\Span::getCurrent()->recordException(new \RuntimeException('boom')); + + $subRequest = new Request(); + $subRequest->attributes->set('_controller', 'ErrorController'); + $subResponse = $kernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST); + $kernel->terminate($mainRequest, $subResponse); + + $this->assertCount(2, $this->storage); + $mainSpan = $this->storage[1]; + $this->assertSame(SpanKind::KIND_SERVER, $mainSpan->getKind()); + $this->assertCount(1, $mainSpan->getEvents()); + $this->assertSame('exception', $mainSpan->getEvents()[0]->getName()); + } + private function getHttpKernel(EventDispatcherInterface $eventDispatcher, $controller = null, ?RequestStack $requestStack = null, array $arguments = []): HttpKernel { $controller ??= fn () => new Response('Hello');