fix for review

getsentry · Apr 14, 2022 · d7b3418 · d7b3418
1 parent f000b6f
commit d7b3418
Show file tree

Hide file tree

Showing 12 changed files with 71 additions and 61 deletions.
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -310,3 +310,12 @@ parameters:
 			count: 1
 			path: tests/Tracing/Doctrine/DBAL/TracingStatementForV2Test.php
 
+		-
+			message: "#^Call to an undefined method Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface::toStream\\(\\)\\.$#"
+			count: 1
+			path: src/Tracing/HttpClient/TraceableResponseForV5.php
+
+		-
+			message: "#^Call to an undefined method Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface::toStream\\(\\)\\.$#"
+			count: 1
+			path: src/Tracing/HttpClient/TraceableResponseForV6.php
diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -68,6 +68,16 @@
       <code>setFetchMode</code>
     </UndefinedInterfaceMethod>
   </file>
+  <file src="src/Tracing/HttpClient/TraceableResponseForV5.php">
+    <UndefinedInterfaceMethod occurrences="1">
+      <code>toStream</code>
+    </UndefinedInterfaceMethod>
+  </file>
+  <file src="src/Tracing/HttpClient/TraceableResponseForV6.php">
+    <UndefinedInterfaceMethod occurrences="1">
+      <code>toStream</code>
+    </UndefinedInterfaceMethod>
+  </file>
   <file src="src/aliases.php">
     <MissingDependency occurrences="1">
       <code>TracingDriverForV2</code>

diff --git a/src/DependencyInjection/Compiler/HttpClientTracingPass.php b/src/DependencyInjection/Compiler/HttpClientTracingPass.php
@@ -25,9 +25,9 @@ public function process(ContainerBuilder $container): void
         }
 
         foreach ($container->findTaggedServiceIds('http_client.client') as $id => $tags) {
-            $container->register('.sentry.' . $id, TraceableHttpClient::class)
+            $container->register('.sentry.traceable.' . $id, TraceableHttpClient::class)
                 ->setArguments([
-                    new Reference('.sentry.' . $id . '.inner'),
+                    new Reference('.sentry.traceable.' . $id . '.inner'),
                     new Reference(HubInterface::class),
                 ])
                 ->addTag('kernel.reset', ['method' => 'reset'])

diff --git a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php
@@ -44,15 +44,20 @@ public function __construct(HttpClientInterface $client, HubInterface $hub)
     public function request(string $method, string $url, array $options = []): ResponseInterface
     {
         $span = null;
-        $transaction = $this->hub->getTransaction();
-        if (null !== $transaction) {
+        $parent = $this->hub->getSpan();
+        if (null !== $parent) {
             $headers = $options['headers'] ?? [];
-            $headers['sentry-trace'] = $transaction->toTraceparent();
+            $headers['sentry-trace'] = $parent->toTraceparent();
             $options['headers'] = $headers;
 
             $context = new SpanContext();
-            $context->setOp($method . ' ' . $url);
-            $span = $transaction->startChild($context);
+            $context->setOp('http.request');
+            $context->setTags([
+                'method' => $method,
+                'url' => $url,
+            ]);
+
+            $span = $parent->startChild($context);
         }
 
         return new TraceableResponse($this->client, $this->client->request($method, $url, $options), $span);

diff --git a/src/Tracing/HttpClient/AbstractTraceableResponse.php b/src/Tracing/HttpClient/AbstractTraceableResponse.php
@@ -5,18 +5,11 @@
 namespace Sentry\SentryBundle\Tracing\HttpClient;
 
 use Sentry\Tracing\Span;
-use Sentry\Tracing\SpanContext;
 use Symfony\Component\HttpClient\Exception\ClientException;
 use Symfony\Component\HttpClient\Exception\RedirectionException;
 use Symfony\Component\HttpClient\Exception\ServerException;
-use Symfony\Component\HttpClient\Response\StreamableInterface;
-use Symfony\Component\HttpClient\Response\StreamWrapper;
 use Symfony\Component\HttpClient\TraceableHttpClient;
 use Symfony\Contracts\HttpClient\ChunkInterface;
-use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface;
-use Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface;
-use Symfony\Contracts\HttpClient\Exception\ServerExceptionInterface;
-use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Symfony\Contracts\HttpClient\ResponseInterface;
 
@@ -33,7 +26,7 @@ abstract class AbstractTraceableResponse implements ResponseInterface
     /**
      * @var HttpClientInterface
      */
-    private $client;
+    protected $client;
 
     /**
      * @var Span|null
@@ -110,30 +103,6 @@ public function cancel(): void
         $this->finish();
     }
 
-    /**
-     * Casts the response to a PHP stream resource.
-     *
-     * @return resource
-     *
-     * @throws TransportExceptionInterface   When a network error occurs
-     * @throws RedirectionExceptionInterface On a 3xx when $throw is true and the "max_redirects" option has been reached
-     * @throws ClientExceptionInterface      On a 4xx when $throw is true
-     * @throws ServerExceptionInterface      On a 5xx when $throw is true
-     */
-    public function toStream(bool $throw = true)
-    {
-        if ($throw) {
-            // Ensure headers arrived
-            $this->response->getHeaders(true);
-        }
-
-        if ($this->response instanceof StreamableInterface) {
-            return $this->response->toStream(false);
-        }
-
-        return StreamWrapper::createResource($this->response, $this->client);
-    }
-
     /**
      * @internal
      *
@@ -152,16 +121,10 @@ public static function stream(HttpClientInterface $client, iterable $responses,
 
             $traceableMap[$r->response] = $r;
             $wrappedResponses[] = $r->response;
-            if ($r->span) {
-                $spanContext = new SpanContext();
-                $spanContext->setOp('stream');
-
-                $r->span = $r->span->startChild($spanContext);
-            }
         }
 
         foreach ($client->stream($wrappedResponses, $timeout) as $r => $chunk) {
-            if ($traceableMap[$r]->span) {
+            if (null !== $traceableMap[$r]->span) {
                 $traceableMap[$r]->span->finish();
             }
 

diff --git a/src/Tracing/HttpClient/TraceableHttpClientForV5.php b/src/Tracing/HttpClient/TraceableHttpClientForV5.php
@@ -4,7 +4,7 @@
 
 namespace Sentry\SentryBundle\Tracing\HttpClient;
 
-class TraceableHttpClientForV5 extends AbstractTraceableHttpClient
+final class TraceableHttpClientForV5 extends AbstractTraceableHttpClient
 {
     /**
      * {@inheritdoc}

diff --git a/src/Tracing/HttpClient/TraceableHttpClientForV6.php b/src/Tracing/HttpClient/TraceableHttpClientForV6.php
@@ -4,7 +4,7 @@
 
 namespace Sentry\SentryBundle\Tracing\HttpClient;
 
-class TraceableHttpClientForV6 extends AbstractTraceableHttpClient
+final class TraceableHttpClientForV6 extends AbstractTraceableHttpClient
 {
     /**
      * {@inheritdoc}

diff --git a/src/Tracing/HttpClient/TraceableResponseForV4.php b/src/Tracing/HttpClient/TraceableResponseForV4.php
@@ -7,11 +7,10 @@
 /**
  * @internal
  */
-class TraceableResponseForV4 extends AbstractTraceableResponse
+final class TraceableResponseForV4 extends AbstractTraceableResponse
 {
     /**
-     * @return mixed An array of all available info, or one of them when $type is
-     *               provided, or null when an unsupported type is requested
+     * {@inheritdoc}
      */
     public function getInfo(string $type = null)
     {

diff --git a/src/Tracing/HttpClient/TraceableResponseForV5.php b/src/Tracing/HttpClient/TraceableResponseForV5.php
@@ -9,14 +9,23 @@
 /**
  * @internal
  */
-class TraceableResponseForV5 extends AbstractTraceableResponse implements StreamableInterface
+final class TraceableResponseForV5 extends AbstractTraceableResponse implements StreamableInterface
 {
     /**
-     * @return mixed An array of all available info, or one of them when $type is
-     *               provided, or null when an unsupported type is requested
+     * {@inheritdoc}
+     *
+     * @return mixed
      */
     public function getInfo(string $type = null)
     {
         return $this->response->getInfo($type);
     }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function toStream(bool $throw = true)
+    {
+        return $this->response->toStream($throw);
+    }
 }
diff --git a/src/Tracing/HttpClient/TraceableResponseForV6.php b/src/Tracing/HttpClient/TraceableResponseForV6.php
@@ -9,14 +9,21 @@
 /**
  * @internal
  */
-class TraceableResponseForV6 extends AbstractTraceableResponse implements StreamableInterface
+final class TraceableResponseForV6 extends AbstractTraceableResponse implements StreamableInterface
 {
     /**
-     * @return mixed An array of all available info, or one of them when $type is
-     *               provided, or null when an unsupported type is requested
+     * {@inheritdoc}
      */
     public function getInfo(string $type = null): mixed
     {
         return $this->response->getInfo($type);
     }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function toStream(bool $throw = true)
+    {
+        return $this->response->toStream($throw);
+    }
 }
diff --git a/src/aliases.php b/src/aliases.php
@@ -136,7 +136,7 @@ class_alias(TracingDriverForV2::class, TracingDriver::class);
     if (!interface_exists(StreamableInterface::class)) {
         class_alias(TraceableResponseForV4::class, TraceableResponse::class);
         class_alias(TraceableHttpClientForV5::class, TraceableHttpClient::class);
-    } elseif (\PHP_VERSION_ID >= 80000) {
+    } elseif (version_compare(\PHP_VERSION, '8.0', '>=')) {
         class_alias(TraceableResponseForV6::class, TraceableResponse::class);
         class_alias(TraceableHttpClientForV6::class, TraceableHttpClient::class);
     } else {

diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php
@@ -52,7 +52,7 @@ public function testRequest(): void
         $transaction->initSpanRecorder();
 
         $this->hub->expects($this->once())
-            ->method('getTransaction')
+            ->method('getSpan')
             ->willReturn($transaction);
 
         $response = $this->createMock(ResponseInterface::class);
@@ -73,14 +73,22 @@ public function testRequest(): void
 
         $this->assertCount(2, $spans);
         $this->assertNull($spans[1]->getEndTimestamp());
-        $this->assertSame('POST http://www.example.org/test-page', $spans[1]->getOp());
+        $this->assertSame('http.request', $spans[1]->getOp());
+        $this->assertSame([
+            'method' => 'POST',
+            'url' => 'http://www.example.org/test-page',
+        ], $spans[1]->getTags());
 
         $response->getContent(false);
 
         $spans = $transaction->getSpanRecorder()->getSpans();
         $this->assertCount(2, $spans);
         $this->assertNotNull($spans[1]->getEndTimestamp());
-        $this->assertSame('POST http://www.example.org/test-page', $spans[1]->getOp());
+        $this->assertSame('http.request', $spans[1]->getOp());
+        $this->assertSame([
+            'method' => 'POST',
+            'url' => 'http://www.example.org/test-page',
+        ], $spans[1]->getTags());
     }
 
     private static function isHttpClientPackageInstalled(): bool