From d7b34181c65402859f586ce246cc99e828413628 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 13 Apr 2022 09:45:37 +0200 Subject: [PATCH] fix for review --- phpstan-baseline.neon | 9 ++++ psalm-baseline.xml | 10 +++++ .../Compiler/HttpClientTracingPass.php | 4 +- .../AbstractTraceableHttpClient.php | 15 ++++--- .../HttpClient/AbstractTraceableResponse.php | 41 +------------------ .../HttpClient/TraceableHttpClientForV5.php | 2 +- .../HttpClient/TraceableHttpClientForV6.php | 2 +- .../HttpClient/TraceableResponseForV4.php | 5 +-- .../HttpClient/TraceableResponseForV5.php | 15 +++++-- .../HttpClient/TraceableResponseForV6.php | 13 ++++-- src/aliases.php | 2 +- .../HttpClient/TraceableHttpClientTest.php | 14 +++++-- 12 files changed, 71 insertions(+), 61 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index dd8025d4..ffc415ce 100644 --- 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 index 8b53fba0..15f9be12 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -68,6 +68,16 @@ setFetchMode + + + toStream + + + + + toStream + + TracingDriverForV2 diff --git a/src/DependencyInjection/Compiler/HttpClientTracingPass.php b/src/DependencyInjection/Compiler/HttpClientTracingPass.php index a17f6f31..1ebcdc4f 100644 --- 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 index 8450f871..22f921bc 100644 --- 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 index 079358af..31495ec3 100644 --- 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 index b7387db4..3fcbad29 100644 --- 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 index 371bf667..4116d476 100644 --- 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 index 9e347845..0cbaedad 100644 --- 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 index db8a99aa..0ca572a2 100644 --- 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 index bb1dc498..43dbbbf2 100644 --- 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 index 2d79599f..c413da98 100644 --- 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 index 431794cb..62e7f985 100644 --- 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