From e62ccf808d24890d3bdbaa5614252f7697dd375a Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Sun, 19 Nov 2023 14:58:03 +0100 Subject: [PATCH] Fix the decoration of the HTTP client when tracing is enabled --- .../Compiler/HttpClientTracingPass.php | 51 ++++++++++++++++--- .../Compiler/HttpClientTracingPassTest.php | 44 ++++++++++++---- .../SentryExtensionTest.php | 2 + 3 files changed, 81 insertions(+), 16 deletions(-) diff --git a/src/DependencyInjection/Compiler/HttpClientTracingPass.php b/src/DependencyInjection/Compiler/HttpClientTracingPass.php index 2464532b..e61bd41e 100644 --- a/src/DependencyInjection/Compiler/HttpClientTracingPass.php +++ b/src/DependencyInjection/Compiler/HttpClientTracingPass.php @@ -12,6 +12,17 @@ final class HttpClientTracingPass implements CompilerPassInterface { + /** + * List of service IDs that can be registered in the container by the + * framework when decorating the mock client. The order is from the + * outermost decorator to the innermost. + */ + private const MOCK_HTTP_CLIENT_SERVICE_IDS = [ + 'http_client.retryable.inner.mock_client', + '.debug.http_client.inner.mock_client', + 'http_client.mock_client', + ]; + /** * {@inheritdoc} */ @@ -21,12 +32,40 @@ public function process(ContainerBuilder $container): void return; } - foreach ($container->findTaggedServiceIds('http_client.client') as $id => $tags) { - $container->register('.sentry.traceable.' . $id, TraceableHttpClient::class) - ->setDecoratedService($id) - ->setArgument(0, new Reference('.sentry.traceable.' . $id . '.inner')) - ->setArgument(1, new Reference(HubInterface::class)) - ->addTag('kernel.reset', ['method' => 'reset']); + $decoratedService = $this->getDecoratedService($container); + + $container->register(TraceableHttpClient::class, TraceableHttpClient::class) + ->setArgument(0, new Reference(TraceableHttpClient::class . '.inner')) + ->setArgument(1, new Reference(HubInterface::class)) + ->setDecoratedService($decoratedService[0], null, $decoratedService[1]); + } + + /** + * @return array{string, int} + */ + private function getDecoratedService(ContainerBuilder $container): array + { + // Starting from Symfony 6.3, the raw HTTP client that serves as adapter + // for the transport is registered as a separate service, so that the + // scoped clients can inject it before any decoration is applied on them. + // Since we need to access the full URL of the request, and such information + // is available after the `ScopingHttpClient` class did its job, we have + // to decorate such service. For more details, see https://github.com/symfony/symfony/pull/49513. + if ($container->hasDefinition('http_client.transport')) { + return ['http_client.transport', -15]; + } + + // On versions of Symfony prior to 6.3, when the mock client is in-use, + // each HTTP client is decorated by referencing explicitly the innermost + // service rather than by using the standard decoration feature. Hence, + // we have to look for the specific names of those services, and decorate + // them instead of the raw HTTP client. + foreach (self::MOCK_HTTP_CLIENT_SERVICE_IDS as $httpClientServiceId) { + if ($container->hasDefinition($httpClientServiceId)) { + return [$httpClientServiceId, 15]; + } } + + return ['http_client', 15]; } } diff --git a/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php index 9f3d12a7..34057831 100644 --- a/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php +++ b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php @@ -9,7 +9,6 @@ use Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClient; use Sentry\State\HubInterface; use Symfony\Component\DependencyInjection\ContainerBuilder; -use Symfony\Component\HttpClient\HttpClient; use Symfony\Contracts\HttpClient\HttpClientInterface; final class HttpClientTracingPassTest extends TestCase @@ -21,12 +20,38 @@ public static function setUpBeforeClass(): void } } - public function testProcess(): void + /** + * @dataProvider processDataProvider + */ + public function testProcess(string $httpClientServiceId): void { - $container = $this->createContainerBuilder(true, true); + $container = $this->createContainerBuilder(true, true, $httpClientServiceId); $container->compile(); - $this->assertSame(TraceableHttpClient::class, $container->findDefinition('http.client')->getClass()); + $this->assertSame(TraceableHttpClient::class, $container->getDefinition($httpClientServiceId)->getClass()); + } + + public function processDataProvider(): \Generator + { + yield 'The framework version is >=6.3' => [ + 'http_client.transport', + ]; + + yield 'The framework version is <6.3 and the mocked HTTP client is decorated by the retryable client' => [ + 'http_client.retryable.inner.mock_client', + ]; + + yield 'The framework version is <6.3 and the mocked HTTP client is decorated by the profiler' => [ + '.debug.http_client.inner.mock_client', + ]; + + yield 'The framework version is <6.3 and the mocked HTTP client is not decorated' => [ + 'http_client.mock_client', + ]; + + yield 'The framework version is <6.3 and the HTTP client is not mocked' => [ + 'http_client', + ]; } /** @@ -34,10 +59,10 @@ public function testProcess(): void */ public function testProcessDoesNothingIfConditionsForEnablingTracingAreMissing(bool $isTracingEnabled, bool $isHttpClientTracingEnabled): void { - $container = $this->createContainerBuilder($isTracingEnabled, $isHttpClientTracingEnabled); + $container = $this->createContainerBuilder($isTracingEnabled, $isHttpClientTracingEnabled, 'http_client.transport'); $container->compile(); - $this->assertSame(HttpClient::class, $container->getDefinition('http.client')->getClass()); + $this->assertSame(HttpClientInterface::class, $container->getDefinition('http_client.transport')->getClass()); } /** @@ -61,7 +86,7 @@ public function processDoesNothingIfConditionsForEnablingTracingAreMissingDataPr ]; } - private function createContainerBuilder(bool $isTracingEnabled, bool $isHttpClientTracingEnabled): ContainerBuilder + private function createContainerBuilder(bool $isTracingEnabled, bool $isHttpClientTracingEnabled, string $httpClientServiceId): ContainerBuilder { $container = new ContainerBuilder(); $container->addCompilerPass(new HttpClientTracingPass()); @@ -71,9 +96,8 @@ private function createContainerBuilder(bool $isTracingEnabled, bool $isHttpClie $container->register(HubInterface::class, HubInterface::class) ->setPublic(true); - $container->register('http.client', HttpClient::class) - ->setPublic(true) - ->addTag('http_client.client'); + $container->register($httpClientServiceId, HttpClientInterface::class) + ->setPublic(true); return $container; } diff --git a/tests/DependencyInjection/SentryExtensionTest.php b/tests/DependencyInjection/SentryExtensionTest.php index f2aac844..cfb2b0b6 100644 --- a/tests/DependencyInjection/SentryExtensionTest.php +++ b/tests/DependencyInjection/SentryExtensionTest.php @@ -40,6 +40,7 @@ use Symfony\Component\DependencyInjection\Reference; use Symfony\Component\ErrorHandler\Error\FatalError; use Symfony\Component\HttpClient\HttpClient; +use Symfony\Component\HttpClient\TraceableHttpClient; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent; use Symfony\Component\Messenger\Event\WorkerMessageHandledEvent; @@ -373,6 +374,7 @@ public function testInstrumentationIsDisabledWhenTracingIsDisabled(): void $this->assertFalse($container->hasDefinition(TracingDriverMiddleware::class)); $this->assertFalse($container->hasDefinition(ConnectionConfigurator::class)); $this->assertFalse($container->hasDefinition(TwigTracingExtension::class)); + $this->assertFalse($container->hasDefinition(TraceableHttpClient::class)); $this->assertFalse($container->getParameter('sentry.tracing.enabled')); $this->assertEmpty($container->getParameter('sentry.tracing.dbal.connections')); }