Fix the decoration of the HTTP client when tracing is enabled (#786)

Co-authored-by: Michi Hoffmann <[email protected]>

Author: ste93cry

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];
     }
 }

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,23 +20,49 @@ 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',
+        ];
     }
 
     /**
      * @dataProvider processDoesNothingIfConditionsForEnablingTracingAreMissingDataProvider
      */
     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;
     }

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'));
     }