From bc0592bd3bc78ea20c2f86d7a0e608834693ba6a Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Tue, 22 Mar 2022 23:45:50 +0100 Subject: [PATCH 01/31] http client tracing --- composer.json | 1 + phpstan-baseline.neon | 20 ++ phpstan.neon | 1 + psalm.xml | 1 + .../Compiler/HttpClientTracingPass.php | 37 +++ src/DependencyInjection/Configuration.php | 4 + src/DependencyInjection/SentryExtension.php | 17 ++ src/SentryBundle.php | 2 + .../AbstractTraceableHttpClient.php | 87 +++++++ .../HttpClient/AbstractTraceableResponse.php | 225 ++++++++++++++++++ .../HttpClient/TraceableHttpClientForV5.php | 19 ++ .../HttpClient/TraceableHttpClientForV6.php | 19 ++ .../HttpClient/TraceableResponseForV4.php | 20 ++ .../HttpClient/TraceableResponseForV5.php | 22 ++ .../HttpClient/TraceableResponseForV6.php | 22 ++ src/aliases.php | 34 ++- .../DependencyInjection/ConfigurationTest.php | 4 + 17 files changed, 530 insertions(+), 5 deletions(-) create mode 100644 src/DependencyInjection/Compiler/HttpClientTracingPass.php create mode 100644 src/Tracing/HttpClient/AbstractTraceableHttpClient.php create mode 100644 src/Tracing/HttpClient/AbstractTraceableResponse.php create mode 100644 src/Tracing/HttpClient/TraceableHttpClientForV5.php create mode 100644 src/Tracing/HttpClient/TraceableHttpClientForV6.php create mode 100644 src/Tracing/HttpClient/TraceableResponseForV4.php create mode 100644 src/Tracing/HttpClient/TraceableResponseForV5.php create mode 100644 src/Tracing/HttpClient/TraceableResponseForV6.php diff --git a/composer.json b/composer.json index fd124a43..537f5239 100644 --- a/composer.json +++ b/composer.json @@ -54,6 +54,7 @@ "symfony/cache": "^4.4.20||^5.0.11||^6.0", "symfony/dom-crawler": "^4.4.20||^5.0.11||^6.0", "symfony/framework-bundle": "^4.4.20||^5.0.11||^6.0", + "symfony/http-client": "^4.4.20||^5.0.11||^6.0", "symfony/messenger": "^4.4.20||^5.0.11||^6.0", "symfony/monolog-bundle": "^3.4", "symfony/phpunit-bridge": "^5.2.6||^6.0", diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index cda2318b..d6174ba1 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -265,6 +265,26 @@ parameters: count: 1 path: tests/DependencyInjection/SentryExtensionTest.php + - + message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableHttpClient::request\\(\\) has parameter \\$options with no value type specified in iterable type array.$#" + count: 1 + path: src/Tracing/HttpClient/AbstractTraceableHttpClient.php + + - + message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableResponse::toArray\\(\\) return type has no value type specified in iterable type array.$#" + count: 1 + path: src/Tracing/HttpClient/AbstractTraceableResponse.php + + - + message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableResponse::stream\\(\\) has parameter \\$responses with no value type specified in iterable type iterable.$#" + count: 1 + path: src/Tracing/HttpClient/AbstractTraceableResponse.php + + - + message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\TraceableHttpClientForV6::withOptions\\(\\) has parameter \\$options with no value type specified in iterable type array.$#" + count: 1 + path: src/Tracing/HttpClient/TraceableHttpClientForV6.php + - message: "#^Class Symfony\\\\Component\\\\Debug\\\\Exception\\\\FatalErrorException not found\\.$#" count: 1 diff --git a/phpstan.neon b/phpstan.neon index df2e90c3..4ee88fdc 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -12,6 +12,7 @@ parameters: - src/aliases.php - src/Tracing/Doctrine/DBAL/TracingStatementForV2.php - src/Tracing/Doctrine/DBAL/TracingDriverForV2.php + - src/Tracing/HttpClient/TraceableHttpClientForV5.php - tests/End2End/App - tests/Tracing/Doctrine/DBAL/TracingDriverForV2Test.php - tests/EventListener/Fixtures/UserWithoutIdentifierStub.php diff --git a/psalm.xml b/psalm.xml index 39fe8712..abb144f4 100644 --- a/psalm.xml +++ b/psalm.xml @@ -11,6 +11,7 @@ + diff --git a/src/DependencyInjection/Compiler/HttpClientTracingPass.php b/src/DependencyInjection/Compiler/HttpClientTracingPass.php new file mode 100644 index 00000000..a17f6f31 --- /dev/null +++ b/src/DependencyInjection/Compiler/HttpClientTracingPass.php @@ -0,0 +1,37 @@ +getParameter('sentry.tracing.enabled') + || !$container->getParameter('sentry.tracing.http_client.enabled') + ) { + return; + } + + foreach ($container->findTaggedServiceIds('http_client.client') as $id => $tags) { + $container->register('.sentry.' . $id, TraceableHttpClient::class) + ->setArguments([ + new Reference('.sentry.' . $id . '.inner'), + new Reference(HubInterface::class), + ]) + ->addTag('kernel.reset', ['method' => 'reset']) + ->setDecoratedService($id); + } + } +} diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 6e5a5902..05d06550 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -14,6 +14,7 @@ use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition; use Symfony\Component\Config\Definition\Builder\TreeBuilder; use Symfony\Component\Config\Definition\ConfigurationInterface; +use Symfony\Component\HttpClient\HttpClient; use Symfony\Component\Messenger\MessageBusInterface; final class Configuration implements ConfigurationInterface @@ -184,6 +185,9 @@ private function addDistributedTracingSection(ArrayNodeDefinition $rootNode): vo ->arrayNode('cache') ->{class_exists(CacheItem::class) ? 'canBeDisabled' : 'canBeEnabled'}() ->end() + ->arrayNode('http_client') + ->{class_exists(HttpClient::class) ? 'canBeDisabled' : 'canBeEnabled'}() + ->end() ->arrayNode('console') ->addDefaultsIfNotSet() ->fixXmlConfig('excluded_command') diff --git a/src/DependencyInjection/SentryExtension.php b/src/DependencyInjection/SentryExtension.php index 1b2bc394..e60e7a84 100644 --- a/src/DependencyInjection/SentryExtension.php +++ b/src/DependencyInjection/SentryExtension.php @@ -37,6 +37,7 @@ use Symfony\Component\DependencyInjection\Loader; use Symfony\Component\DependencyInjection\Reference; use Symfony\Component\ErrorHandler\Error\FatalError; +use Symfony\Component\HttpClient\HttpClient; use Symfony\Component\HttpKernel\DependencyInjection\ConfigurableExtension; final class SentryExtension extends ConfigurableExtension @@ -74,6 +75,7 @@ protected function loadInternal(array $mergedConfig, ContainerBuilder $container $this->registerDbalTracingConfiguration($container, $mergedConfig['tracing']); $this->registerTwigTracingConfiguration($container, $mergedConfig['tracing']); $this->registerCacheTracingConfiguration($container, $mergedConfig['tracing']); + $this->registerHttpClientTracingConfiguration($container, $mergedConfig['tracing']); } /** @@ -247,6 +249,21 @@ private function registerCacheTracingConfiguration(ContainerBuilder $container, $container->setParameter('sentry.tracing.cache.enabled', $isConfigEnabled); } + /** + * @param array $config + */ + private function registerHttpClientTracingConfiguration(ContainerBuilder $container, array $config): void + { + $isConfigEnabled = $this->isConfigEnabled($container, $config) + && $this->isConfigEnabled($container, $config['http_client']); + + if ($isConfigEnabled && !class_exists(HttpClient::class)) { + throw new LogicException('Http client tracing support cannot be enabled because the symfony/http-client Composer package is not installed.'); + } + + $container->setParameter('sentry.tracing.http_client.enabled', $isConfigEnabled); + } + /** * @param string[] $integrations * @param array $config diff --git a/src/SentryBundle.php b/src/SentryBundle.php index 4098f8b3..130999eb 100644 --- a/src/SentryBundle.php +++ b/src/SentryBundle.php @@ -6,6 +6,7 @@ use Sentry\SentryBundle\DependencyInjection\Compiler\CacheTracingPass; use Sentry\SentryBundle\DependencyInjection\Compiler\DbalTracingPass; +use Sentry\SentryBundle\DependencyInjection\Compiler\HttpClientTracingPass; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\HttpKernel\Bundle\Bundle; @@ -19,5 +20,6 @@ public function build(ContainerBuilder $container): void $container->addCompilerPass(new DbalTracingPass()); $container->addCompilerPass(new CacheTracingPass()); + $container->addCompilerPass(new HttpClientTracingPass()); } } diff --git a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php new file mode 100644 index 00000000..f658b842 --- /dev/null +++ b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php @@ -0,0 +1,87 @@ +client = $client; + $this->hub = $hub; + } + + /** + * {@inheritdoc} + */ + public function request(string $method, string $url, array $options = []): ResponseInterface + { + $span = null; + $transaction = $this->hub->getTransaction(); + if (null !== $transaction) { + $context = new SpanContext(); + $context->setOp($method . ' ' . $url); + $span = $transaction->startChild($context); + } + + return new TraceableResponse($this->client, $this->client->request($method, $url, $options), $span); + } + + /** + * {@inheritdoc} + */ + public function stream($responses, float $timeout = null): ResponseStreamInterface + { + if ($responses instanceof AbstractTraceableResponse) { + $responses = [$responses]; + } elseif (!is_iterable($responses)) { + throw new \TypeError(sprintf('"%s()" expects parameter 1 to be an iterable of TraceableResponse objects, "%s" given.', __METHOD__, get_debug_type($responses))); + } + + return new ResponseStream(AbstractTraceableResponse::stream($this->client, $responses, $timeout)); + } + + public function reset(): void + { + if ($this->client instanceof ResetInterface) { + $this->client->reset(); + } + } + + /** + * {@inheritdoc} + */ + public function setLogger(LoggerInterface $logger): void + { + if ($this->client instanceof LoggerAwareInterface) { + $this->client->setLogger($logger); + } + } +} diff --git a/src/Tracing/HttpClient/AbstractTraceableResponse.php b/src/Tracing/HttpClient/AbstractTraceableResponse.php new file mode 100644 index 00000000..996ec4b0 --- /dev/null +++ b/src/Tracing/HttpClient/AbstractTraceableResponse.php @@ -0,0 +1,225 @@ +client = $client; + $this->response = $response; + $this->span = $span; + } + + public function __sleep(): array + { + throw new \BadMethodCallException('Cannot serialize ' . __CLASS__); + } + + public function __wakeup() + { + throw new \BadMethodCallException('Cannot unserialize ' . __CLASS__); + } + + public function __destruct() + { + try { + if (method_exists($this->response, '__destruct')) { + $this->response->__destruct(); + } + } finally { + $this->finish(); + } + } + + public function getStatusCode(): int + { + return $this->traceFunction('status_code', function () { + return $this->response->getStatusCode(); + }); + } + + public function getHeaders(bool $throw = true): array + { + return $this->traceFunction('headers', function () use ($throw) { + return $this->response->getHeaders($throw); + }); + } + + public function getContent(bool $throw = true): string + { + try { + return $this->response->getContent($throw); + } finally { + $this->finish(); + + if ($throw) { + $this->checkStatusCode($this->response->getStatusCode()); + } + } + } + + public function toArray(bool $throw = true): array + { + try { + return $this->response->toArray($throw); + } finally { + $this->finish(); + + if ($throw) { + $this->checkStatusCode($this->response->getStatusCode()); + } + } + } + + public function cancel(): void + { + $this->response->cancel(); + $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 + * + * @return \Generator + */ + public static function stream(HttpClientInterface $client, iterable $responses, ?float $timeout): \Generator + { + $wrappedResponses = []; + /** @var \SplObjectStorage $traceableMap */ + $traceableMap = new \SplObjectStorage(); + + foreach ($responses as $r) { + if (!$r instanceof self) { + throw new \TypeError(sprintf('"%s::stream()" expects parameter 1 to be an iterable of TraceableResponse objects, "%s" given.', TraceableHttpClient::class, get_debug_type($r))); + } + + $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) { + $traceableMap[$r]->span->finish(); + } + + yield $traceableMap[$r] => $chunk; + } + } + + private function checkStatusCode(int $code): void + { + if (500 <= $code) { + throw new ServerException($this); + } + + if (400 <= $code) { + throw new ClientException($this); + } + + if (300 <= $code) { + throw new RedirectionException($this); + } + } + + private function finish(): void + { + if (null !== $this->span) { + $this->span->finish(); + $this->span = null; + } + } + + /** + * @phpstan-template TResult + * + * @phpstan-param Closure(): TResult $callback + * + * @phpstan-return TResult + */ + private function traceFunction(string $spanOperation, Closure $callback) + { + $span = $this->span; + if (null !== $span) { + $spanContext = new SpanContext(); + $spanContext->setOp($spanOperation); + + $span = $span->startChild($spanContext); + } + + try { + return $callback(); + } finally { + if (null !== $span) { + $span->finish(); + } + } + } +} diff --git a/src/Tracing/HttpClient/TraceableHttpClientForV5.php b/src/Tracing/HttpClient/TraceableHttpClientForV5.php new file mode 100644 index 00000000..b7387db4 --- /dev/null +++ b/src/Tracing/HttpClient/TraceableHttpClientForV5.php @@ -0,0 +1,19 @@ +client = $this->client->withOptions($options); + + return $clone; + } +} diff --git a/src/Tracing/HttpClient/TraceableHttpClientForV6.php b/src/Tracing/HttpClient/TraceableHttpClientForV6.php new file mode 100644 index 00000000..371bf667 --- /dev/null +++ b/src/Tracing/HttpClient/TraceableHttpClientForV6.php @@ -0,0 +1,19 @@ +client = $this->client->withOptions($options); + + return $clone; + } +} diff --git a/src/Tracing/HttpClient/TraceableResponseForV4.php b/src/Tracing/HttpClient/TraceableResponseForV4.php new file mode 100644 index 00000000..9e347845 --- /dev/null +++ b/src/Tracing/HttpClient/TraceableResponseForV4.php @@ -0,0 +1,20 @@ +response->getInfo($type); + } +} diff --git a/src/Tracing/HttpClient/TraceableResponseForV5.php b/src/Tracing/HttpClient/TraceableResponseForV5.php new file mode 100644 index 00000000..db8a99aa --- /dev/null +++ b/src/Tracing/HttpClient/TraceableResponseForV5.php @@ -0,0 +1,22 @@ +response->getInfo($type); + } +} diff --git a/src/Tracing/HttpClient/TraceableResponseForV6.php b/src/Tracing/HttpClient/TraceableResponseForV6.php new file mode 100644 index 00000000..bb1dc498 --- /dev/null +++ b/src/Tracing/HttpClient/TraceableResponseForV6.php @@ -0,0 +1,22 @@ +response->getInfo($type); + } +} diff --git a/src/aliases.php b/src/aliases.php index 5d0894aa..cb6910b8 100644 --- a/src/aliases.php +++ b/src/aliases.php @@ -11,12 +11,23 @@ use Sentry\SentryBundle\Tracing\Cache\TraceableTagAwareCacheAdapter; use Sentry\SentryBundle\Tracing\Cache\TraceableTagAwareCacheAdapterForV2; use Sentry\SentryBundle\Tracing\Cache\TraceableTagAwareCacheAdapterForV3; +use Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriver; use Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverForV2; use Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverForV3; +use Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingStatement; use Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingStatementForV2; use Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingStatementForV3; +use Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClient; +use Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClientForV5; +use Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClientForV6; +use Sentry\SentryBundle\Tracing\HttpClient\TraceableResponse; +use Sentry\SentryBundle\Tracing\HttpClient\TraceableResponseForV4; +use Sentry\SentryBundle\Tracing\HttpClient\TraceableResponseForV5; +use Sentry\SentryBundle\Tracing\HttpClient\TraceableResponseForV6; use Symfony\Component\Cache\Adapter\AdapterInterface; use Symfony\Component\Cache\DoctrineProvider; +use Symfony\Component\HttpClient\Response\StreamableInterface; +use Symfony\Contracts\HttpClient\ResponseInterface; if (interface_exists(AdapterInterface::class)) { if (!class_exists(DoctrineProvider::class, false) && version_compare(\PHP_VERSION, '8.0.0', '>=')) { @@ -38,12 +49,25 @@ class_alias(TraceableTagAwareCacheAdapterForV2::class, TraceableTagAwareCacheAda } } -if (!class_exists('Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingStatement')) { +if (!class_exists(TracingStatement::class)) { if (class_exists(Result::class)) { - class_alias(TracingStatementForV3::class, 'Sentry\\SentryBundle\\Tracing\\Doctrine\\DBAL\\TracingStatement'); - class_alias(TracingDriverForV3::class, 'Sentry\\SentryBundle\\Tracing\\Doctrine\\DBAL\\TracingDriver'); + class_alias(TracingStatementForV3::class, TracingStatement::class); + class_alias(TracingDriverForV3::class, TracingDriver::class); } elseif (interface_exists(Result::class)) { - class_alias(TracingStatementForV2::class, 'Sentry\\SentryBundle\\Tracing\\Doctrine\\DBAL\\TracingStatement'); - class_alias(TracingDriverForV2::class, 'Sentry\\SentryBundle\\Tracing\\Doctrine\\DBAL\\TracingDriver'); + class_alias(TracingStatementForV2::class, TracingStatement::class); + class_alias(TracingDriverForV2::class, TracingDriver::class); + } +} + +if (!class_exists(TraceableResponse::class) && interface_exists(ResponseInterface::class)) { + if (!interface_exists(StreamableInterface::class)) { + class_alias(TraceableResponseForV4::class, TraceableResponse::class); + class_alias(TraceableHttpClientForV5::class, TraceableHttpClient::class); + } elseif (\PHP_VERSION_ID >= 80000) { + class_alias(TraceableResponseForV6::class, TraceableResponse::class); + class_alias(TraceableHttpClientForV6::class, TraceableHttpClient::class); + } else { + class_alias(TraceableResponseForV5::class, TraceableResponse::class); + class_alias(TraceableHttpClientForV5::class, TraceableHttpClient::class); } } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 5f73cfde..9b853130 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -12,6 +12,7 @@ use Symfony\Component\Cache\CacheItem; use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException; use Symfony\Component\Config\Definition\Processor; +use Symfony\Component\HttpClient\HttpClient; use Symfony\Component\Messenger\MessageBusInterface; final class ConfigurationTest extends TestCase @@ -52,6 +53,9 @@ public function testProcessConfigurationWithDefaultConfiguration(): void 'cache' => [ 'enabled' => class_exists(CacheItem::class), ], + 'http_client' => [ + 'enabled' => class_exists(HttpClient::class), + ], 'console' => [ 'excluded_commands' => ['messenger:consume'], ], From c680429193afcf818c2074806996eb110561eee9 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 23 Mar 2022 20:43:01 +0100 Subject: [PATCH 02/31] sentry sentry-trace header in tracing http client --- .../AbstractTraceableHttpClient.php | 4 +++ .../HttpClient/AbstractTraceableResponse.php | 35 ++----------------- 2 files changed, 6 insertions(+), 33 deletions(-) diff --git a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php index f658b842..8450f871 100644 --- a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php +++ b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php @@ -46,6 +46,10 @@ public function request(string $method, string $url, array $options = []): Respo $span = null; $transaction = $this->hub->getTransaction(); if (null !== $transaction) { + $headers = $options['headers'] ?? []; + $headers['sentry-trace'] = $transaction->toTraceparent(); + $options['headers'] = $headers; + $context = new SpanContext(); $context->setOp($method . ' ' . $url); $span = $transaction->startChild($context); diff --git a/src/Tracing/HttpClient/AbstractTraceableResponse.php b/src/Tracing/HttpClient/AbstractTraceableResponse.php index 996ec4b0..079358af 100644 --- a/src/Tracing/HttpClient/AbstractTraceableResponse.php +++ b/src/Tracing/HttpClient/AbstractTraceableResponse.php @@ -4,7 +4,6 @@ namespace Sentry\SentryBundle\Tracing\HttpClient; -use Closure; use Sentry\Tracing\Span; use Sentry\Tracing\SpanContext; use Symfony\Component\HttpClient\Exception\ClientException; @@ -71,16 +70,12 @@ public function __destruct() public function getStatusCode(): int { - return $this->traceFunction('status_code', function () { - return $this->response->getStatusCode(); - }); + return $this->response->getStatusCode(); } public function getHeaders(bool $throw = true): array { - return $this->traceFunction('headers', function () use ($throw) { - return $this->response->getHeaders($throw); - }); + return $this->response->getHeaders($throw); } public function getContent(bool $throw = true): string @@ -196,30 +191,4 @@ private function finish(): void $this->span = null; } } - - /** - * @phpstan-template TResult - * - * @phpstan-param Closure(): TResult $callback - * - * @phpstan-return TResult - */ - private function traceFunction(string $spanOperation, Closure $callback) - { - $span = $this->span; - if (null !== $span) { - $spanContext = new SpanContext(); - $spanContext->setOp($spanOperation); - - $span = $span->startChild($spanContext); - } - - try { - return $callback(); - } finally { - if (null !== $span) { - $span->finish(); - } - } - } } From 166c4782e0a8825cf7ece249989eca73e5674a0a Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 23 Mar 2022 21:39:25 +0100 Subject: [PATCH 03/31] add test on http client request --- .../HttpClient/TraceableHttpClientTest.php | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 tests/Tracing/HttpClient/TraceableHttpClientTest.php diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php new file mode 100644 index 00000000..431794cb --- /dev/null +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -0,0 +1,90 @@ +hub = $this->createMock(HubInterface::class); + $this->client = $this->createMock(HttpClientInterface::class); + $this->httpClient = new TraceableHttpClient($this->client, $this->hub); + } + + public function testRequest(): void + { + $transaction = new Transaction(new TransactionContext()); + $transaction->initSpanRecorder(); + + $this->hub->expects($this->once()) + ->method('getTransaction') + ->willReturn($transaction); + + $response = $this->createMock(ResponseInterface::class); + $this->client->expects($this->once()) + ->method('request') + ->with('POST', 'http://www.example.org/test-page', new Callback(function ($value) use ($transaction) { + $this->assertArrayHasKey('headers', $value); + + return ['sentry-trace' => $transaction->toTraceparent()] === $value['headers']; + })) + ->willReturn($response); + + $response = $this->httpClient->request('POST', 'http://www.example.org/test-page', []); + + $this->assertInstanceOf(AbstractTraceableResponse::class, $response); + $this->assertNotNull($transaction->getSpanRecorder()); + $spans = $transaction->getSpanRecorder()->getSpans(); + + $this->assertCount(2, $spans); + $this->assertNull($spans[1]->getEndTimestamp()); + $this->assertSame('POST http://www.example.org/test-page', $spans[1]->getOp()); + + $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()); + } + + private static function isHttpClientPackageInstalled(): bool + { + return interface_exists(HttpClientInterface::class); + } +} From 5ce0a9d3c63fd60ac7f8213b89001490dceb9014 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 23 Mar 2022 21:44:42 +0100 Subject: [PATCH 04/31] use newer phpunit on php 8 --- .github/workflows/tests.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 0083401f..4abac4b7 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -63,6 +63,10 @@ jobs: - name: Setup Problem Matchers for PHPUnit run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json" + - name: Update PHPUnit + run: composer require --dev phpunit/phpunit ^9.3.9 --no-update + if: matrix.php == '8.0' && matrix.dependencies == 'lowest' + - name: Install dependencies uses: ramsey/composer-install@v1 with: From 9d42874ccada08a0249774318423f2e9c14456ec Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 23 Mar 2022 21:56:52 +0100 Subject: [PATCH 05/31] require php 8.1 in psalm --- .github/workflows/static-analysis.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/static-analysis.yaml b/.github/workflows/static-analysis.yaml index 69e391a2..ffb27e13 100644 --- a/.github/workflows/static-analysis.yaml +++ b/.github/workflows/static-analysis.yaml @@ -61,4 +61,4 @@ jobs: run: composer update --no-progress --no-interaction --prefer-dist - name: Run script - run: composer psalm -- --php-version=8.0 + run: composer psalm -- --php-version=8.1 From 0ee2c44695dcfbc4e82b3201a1b4734baf921cdc Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Tue, 12 Apr 2022 17:34:02 +0200 Subject: [PATCH 06/31] Revert "require php 8.1 in psalm" This reverts commit 022b913328a3f0dc993a1d07eeda4e4eb45791b7. --- .github/workflows/static-analysis.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/static-analysis.yaml b/.github/workflows/static-analysis.yaml index ffb27e13..69e391a2 100644 --- a/.github/workflows/static-analysis.yaml +++ b/.github/workflows/static-analysis.yaml @@ -61,4 +61,4 @@ jobs: run: composer update --no-progress --no-interaction --prefer-dist - name: Run script - run: composer psalm -- --php-version=8.1 + run: composer psalm -- --php-version=8.0 From 064881449ed9e118a968078863295255264ea95a Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 13 Apr 2022 09:45:37 +0200 Subject: [PATCH 07/31] 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 d6174ba1..0f1640b5 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -430,3 +430,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 5ef7a1e3..b40d7e6f 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -50,6 +50,16 @@ $params + + + 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 cb6910b8..3f81d525 100644 --- a/src/aliases.php +++ b/src/aliases.php @@ -63,7 +63,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 From 73132c2176e4d59f5b0c3cdd2fd9a2fa1cd19980 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Thu, 14 Apr 2022 15:50:29 +0200 Subject: [PATCH 08/31] add a couple of tests and changelog line --- CHANGELOG.md | 2 + psalm-baseline.xml | 19 +++++ .../Compiler/HttpClientTracingPassTest.php | 73 +++++++++++++++++++ .../HttpClient/TraceableHttpClientTest.php | 29 +++++++- 4 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e77863f3..0d83edd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Add support for tracing of http client requests (#606) + ## 4.3.0 (2022-05-30) - Fix compatibility issue with Symfony >= 6.1.0 (#635) - Add `TracingDriverConnectionInterface::getNativeConnection()` method to get the original driver connection (#597) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index b40d7e6f..32016223 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -73,4 +73,23 @@ PostResponseEvent + + + $container->getParameter('sentry.tracing.cache.enabled') + + + + + $container->getParameter('sentry.tracing.enabled') + $container->getParameter('sentry.tracing.http_client.enabled') + + + + + $container->getParameter('sentry.tracing.enabled') + $container->getParameter('sentry.tracing.dbal.enabled') + $container->getParameter('sentry.tracing.dbal.connections') + $container->getParameter('doctrine.connections') + + diff --git a/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php new file mode 100644 index 00000000..16861636 --- /dev/null +++ b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php @@ -0,0 +1,73 @@ + $params + * + * @dataProvider provideDisableContainerParameters + */ + public function testShouldNotDecorateHttpClientServicesIfDisabled(array $params): void + { + $container = new ContainerBuilder(new ParameterBag($params)); + $container->register('http.client', HttpClient::class) + ->setPublic(true) + ->addTag('http_client.client'); + + $container->addCompilerPass(new HttpClientTracingPass()); + $container->compile(); + + self::assertEquals(HttpClient::class, $container->getDefinition('http.client')->getClass()); + } + + /** + * @return iterable + */ + public function provideDisableContainerParameters(): iterable + { + yield [['sentry.tracing.enabled' => true, 'sentry.tracing.http_client.enabled' => false]]; + yield [['sentry.tracing.enabled' => false, 'sentry.tracing.http_client.enabled' => false]]; + yield [['sentry.tracing.enabled' => false, 'sentry.tracing.http_client.enabled' => true]]; + } + + public function testShouldDecorateHttpClients(): void + { + $container = new ContainerBuilder(new ParameterBag(['sentry.tracing.enabled' => true, 'sentry.tracing.http_client.enabled' => true])); + $container->register(HubInterface::class) + ->setFactory([HubAdapter::class, 'getInstance']); + $container->register('http.client', HttpClient::class) + ->setPublic(true) + ->addTag('http_client.client'); + + $container->addCompilerPass(new HttpClientTracingPass()); + $container->compile(); + + self::assertEquals(TraceableHttpClient::class, $container->findDefinition('http.client')->getClass()); + } + + private static function isHttpClientPackageInstalled(): bool + { + return interface_exists(HttpClientInterface::class); + } +} diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 62e7f985..5ae72757 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -7,6 +7,8 @@ use PHPUnit\Framework\Constraint\Callback; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerAwareInterface; +use Psr\Log\NullLogger; use Sentry\SentryBundle\Tracing\HttpClient\AbstractTraceableResponse; use Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClient; use Sentry\State\HubInterface; @@ -14,6 +16,7 @@ use Sentry\Tracing\TransactionContext; use Symfony\Contracts\HttpClient\HttpClientInterface; use Symfony\Contracts\HttpClient\ResponseInterface; +use Symfony\Contracts\Service\ResetInterface; final class TraceableHttpClientTest extends TestCase { @@ -23,7 +26,7 @@ final class TraceableHttpClientTest extends TestCase private $hub; /** - * @var MockObject&HttpClientInterface + * @var MockObject&HttpClientInterface&LoggerAwareInterface&ResetInterface */ private $client; @@ -42,7 +45,7 @@ public static function setUpBeforeClass(): void protected function setUp(): void { $this->hub = $this->createMock(HubInterface::class); - $this->client = $this->createMock(HttpClientInterface::class); + $this->client = $this->createMock(TestableHttpClientInterface::class); $this->httpClient = new TraceableHttpClient($this->client, $this->hub); } @@ -91,8 +94,30 @@ public function testRequest(): void ], $spans[1]->getTags()); } + public function testSetLoggerShouldBeForwardedToDecoratedInstance(): void + { + $logger = new NullLogger(); + $this->client->expects($this->once()) + ->method('setLogger') + ->with($logger); + + $this->httpClient->setLogger($logger); + } + + public function testResetCallShouldBeForwardedToDecoratedInstance(): void + { + $this->client->expects($this->once()) + ->method('reset'); + + $this->httpClient->reset(); + } + private static function isHttpClientPackageInstalled(): bool { return interface_exists(HttpClientInterface::class); } } + +interface TestableHttpClientInterface extends HttpClientInterface, LoggerAwareInterface, ResetInterface +{ +} From 095abc3bfa9446e99334e0c50da8711fc8a57d76 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Fri, 15 Apr 2022 13:47:40 +0200 Subject: [PATCH 09/31] use http.url and http.method tags, instead of url and method --- src/Tracing/HttpClient/AbstractTraceableHttpClient.php | 4 ++-- tests/Tracing/HttpClient/TraceableHttpClientTest.php | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php index 22f921bc..28edd8ff 100644 --- a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php +++ b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php @@ -53,8 +53,8 @@ public function request(string $method, string $url, array $options = []): Respo $context = new SpanContext(); $context->setOp('http.request'); $context->setTags([ - 'method' => $method, - 'url' => $url, + 'http.method' => $method, + 'http.url' => $url, ]); $span = $parent->startChild($context); diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 5ae72757..9870d5ed 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -78,8 +78,8 @@ public function testRequest(): void $this->assertNull($spans[1]->getEndTimestamp()); $this->assertSame('http.request', $spans[1]->getOp()); $this->assertSame([ - 'method' => 'POST', - 'url' => 'http://www.example.org/test-page', + 'http.method' => 'POST', + 'http.url' => 'http://www.example.org/test-page', ], $spans[1]->getTags()); $response->getContent(false); @@ -89,8 +89,8 @@ public function testRequest(): void $this->assertNotNull($spans[1]->getEndTimestamp()); $this->assertSame('http.request', $spans[1]->getOp()); $this->assertSame([ - 'method' => 'POST', - 'url' => 'http://www.example.org/test-page', + 'http.method' => 'POST', + 'http.url' => 'http://www.example.org/test-page', ], $spans[1]->getTags()); } From d4c749f1409ba7f505d859f730a91cde459fe9d8 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Mon, 2 May 2022 16:06:19 +0200 Subject: [PATCH 10/31] http.request -> http.client --- src/Tracing/HttpClient/AbstractTraceableHttpClient.php | 2 +- tests/Tracing/HttpClient/TraceableHttpClientTest.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php index 28edd8ff..ff57b253 100644 --- a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php +++ b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php @@ -51,7 +51,7 @@ public function request(string $method, string $url, array $options = []): Respo $options['headers'] = $headers; $context = new SpanContext(); - $context->setOp('http.request'); + $context->setOp('http.client'); $context->setTags([ 'http.method' => $method, 'http.url' => $url, diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 9870d5ed..24a2cb79 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -76,7 +76,7 @@ public function testRequest(): void $this->assertCount(2, $spans); $this->assertNull($spans[1]->getEndTimestamp()); - $this->assertSame('http.request', $spans[1]->getOp()); + $this->assertSame('http.client', $spans[1]->getOp()); $this->assertSame([ 'http.method' => 'POST', 'http.url' => 'http://www.example.org/test-page', @@ -87,7 +87,7 @@ public function testRequest(): void $spans = $transaction->getSpanRecorder()->getSpans(); $this->assertCount(2, $spans); $this->assertNotNull($spans[1]->getEndTimestamp()); - $this->assertSame('http.request', $spans[1]->getOp()); + $this->assertSame('http.client', $spans[1]->getOp()); $this->assertSame([ 'http.method' => 'POST', 'http.url' => 'http://www.example.org/test-page', From 12490c4dbf4d4f1233d90a5e2b9d4c404c2a2511 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 22 Jun 2022 10:11:19 +0200 Subject: [PATCH 11/31] add tests for traceable response classes --- .../HttpClient/AbstractTraceableResponse.php | 26 --- .../HttpClient/TraceableResponseTest.php | 158 ++++++++++++++++++ 2 files changed, 158 insertions(+), 26 deletions(-) create mode 100644 tests/Tracing/HttpClient/TraceableResponseTest.php diff --git a/src/Tracing/HttpClient/AbstractTraceableResponse.php b/src/Tracing/HttpClient/AbstractTraceableResponse.php index 31495ec3..b2ad3819 100644 --- a/src/Tracing/HttpClient/AbstractTraceableResponse.php +++ b/src/Tracing/HttpClient/AbstractTraceableResponse.php @@ -5,9 +5,6 @@ namespace Sentry\SentryBundle\Tracing\HttpClient; use Sentry\Tracing\Span; -use Symfony\Component\HttpClient\Exception\ClientException; -use Symfony\Component\HttpClient\Exception\RedirectionException; -use Symfony\Component\HttpClient\Exception\ServerException; use Symfony\Component\HttpClient\TraceableHttpClient; use Symfony\Contracts\HttpClient\ChunkInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; @@ -77,10 +74,6 @@ public function getContent(bool $throw = true): string return $this->response->getContent($throw); } finally { $this->finish(); - - if ($throw) { - $this->checkStatusCode($this->response->getStatusCode()); - } } } @@ -90,10 +83,6 @@ public function toArray(bool $throw = true): array return $this->response->toArray($throw); } finally { $this->finish(); - - if ($throw) { - $this->checkStatusCode($this->response->getStatusCode()); - } } } @@ -132,21 +121,6 @@ public static function stream(HttpClientInterface $client, iterable $responses, } } - private function checkStatusCode(int $code): void - { - if (500 <= $code) { - throw new ServerException($this); - } - - if (400 <= $code) { - throw new ClientException($this); - } - - if (300 <= $code) { - throw new RedirectionException($this); - } - } - private function finish(): void { if (null !== $this->span) { diff --git a/tests/Tracing/HttpClient/TraceableResponseTest.php b/tests/Tracing/HttpClient/TraceableResponseTest.php new file mode 100644 index 00000000..f8292abd --- /dev/null +++ b/tests/Tracing/HttpClient/TraceableResponseTest.php @@ -0,0 +1,158 @@ +mockedResponse = $this->createMock(ResponseInterface::class); + $this->client = $this->createMock(HttpClientInterface::class); + $this->hub = $this->createMock(HubInterface::class); + $this->response = new TraceableResponse($this->client, $this->mockedResponse, null); + } + + public function testCannotBeSerialized(): void + { + $this->expectException(\BadMethodCallException::class); + serialize($this->response); + } + + public function testCannotBeDeserialized(): void + { + $this->expectException(\BadMethodCallException::class); + unserialize(sprintf('O:%u:"%s":0:{}', strlen(TraceableResponse::class), TraceableResponse::class)); + } + + public function testDestructor(): void + { + $transaction = new Transaction(new TransactionContext(), $this->hub); + $context = new SpanContext(); + $span = $transaction->startChild($context); + + $this->mockedResponse = $this->getMockBuilder(ResponseInterface::class) + ->addMethods(['__destruct']) + ->getMockForAbstractClass(); + + $this->mockedResponse + ->expects($this->once()) + ->method('__destruct'); + + $this->response = new TraceableResponse($this->client, $this->mockedResponse, $span); + + // Call gc to invoke destructors at the right time. + unset($this->response); + + gc_mem_caches(); + gc_collect_cycles(); + + $this->assertNotNull($span->getEndTimestamp()); + } + + public function testGetHeaders(): void + { + $this->mockedResponse + ->expects($this->once()) + ->method('getHeaders') + ->with(true); + + $this->response->getHeaders(true); + } + + public function testGetStatusCode(): void + { + $this->mockedResponse + ->expects($this->once()) + ->method('getStatusCode'); + + $this->response->getStatusCode(); + } + + public function testGetContent(): void + { + $this->mockedResponse + ->expects($this->once()) + ->method('getContent') + ->with(false); + + $this->response->getContent(false); + } + + public function testToArray(): void + { + $this->mockedResponse + ->expects($this->once()) + ->method('toArray') + ->with(false); + + $this->response->toArray(false); + } + + public function testCancel(): void + { + $this->mockedResponse + ->expects($this->once()) + ->method('cancel'); + + $this->response->cancel(); + } + + public function testGetInfo(): void + { + $this->mockedResponse + ->expects($this->once()) + ->method('getInfo') + ->with('type'); + + $this->response->getInfo('type'); + } + + public function testToStream(): void + { + if (! method_exists($this->response, 'toStream')) { + self::markTestSkipped('Response toStream method is not existent in this version of http-client'); + } + + $this->mockedResponse = $this->getMockBuilder(ResponseInterface::class) + ->addMethods(['toStream']) + ->getMockForAbstractClass(); + + $this->mockedResponse + ->expects($this->once()) + ->method('toStream') + ->with(false); + + $this->response = new TraceableResponse($this->client, $this->mockedResponse, null); + $this->response->toStream(false); + } +} From 6760d1b2080256fc9f80b521d6dcb452e02505fb Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 22 Jun 2022 10:20:20 +0200 Subject: [PATCH 12/31] style fixes --- phpstan-baseline.neon | 7 ++++++- tests/Tracing/HttpClient/TraceableResponseTest.php | 6 ++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 0f1640b5..df52dd92 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -122,7 +122,7 @@ parameters: - message: "#^Parameter \\#2 \\$config of method Symfony\\\\Component\\\\DependencyInjection\\\\Extension\\\\Extension\\:\\:isConfigEnabled\\(\\) expects array, mixed given\\.$#" - count: 3 + count: 4 path: src/DependencyInjection/SentryExtension.php - @@ -135,6 +135,11 @@ parameters: count: 1 path: src/DependencyInjection/SentryExtension.php + - + message: "#^Parameter \\#2 \\$config of method Sentry\\\\SentryBundle\\\\DependencyInjection\\\\SentryExtension\\:\\:registerHttpClientTracingConfiguration\\(\\) expects array, mixed given\\.$#" + count: 1 + path: src/DependencyInjection/SentryExtension.php + - message: "#^Parameter \\#2 \\$value of method Symfony\\\\Component\\\\DependencyInjection\\\\Container\\:\\:setParameter\\(\\) expects array\\|bool\\|float\\|int\\|string\\|UnitEnum\\|null, mixed given\\.$#" count: 2 diff --git a/tests/Tracing/HttpClient/TraceableResponseTest.php b/tests/Tracing/HttpClient/TraceableResponseTest.php index f8292abd..6dcea4d6 100644 --- a/tests/Tracing/HttpClient/TraceableResponseTest.php +++ b/tests/Tracing/HttpClient/TraceableResponseTest.php @@ -1,5 +1,7 @@ expectException(\BadMethodCallException::class); - unserialize(sprintf('O:%u:"%s":0:{}', strlen(TraceableResponse::class), TraceableResponse::class)); + unserialize(sprintf('O:%u:"%s":0:{}', \strlen(TraceableResponse::class), TraceableResponse::class)); } public function testDestructor(): void @@ -139,7 +141,7 @@ public function testGetInfo(): void public function testToStream(): void { - if (! method_exists($this->response, 'toStream')) { + if (!method_exists($this->response, 'toStream')) { self::markTestSkipped('Response toStream method is not existent in this version of http-client'); } From 79695c3cdc10b681411dd7057af58ceab4eafe8e Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 22 Jun 2022 10:31:19 +0200 Subject: [PATCH 13/31] create fixtures for destructible and streamable responses --- .../Fixtures/DestructibleResponseInterface.php | 12 ++++++++++++ .../Fixtures/StreamableResponseInterface.php | 12 ++++++++++++ tests/Tracing/HttpClient/TraceableResponseTest.php | 12 ++++-------- 3 files changed, 28 insertions(+), 8 deletions(-) create mode 100644 tests/Tracing/HttpClient/Fixtures/DestructibleResponseInterface.php create mode 100644 tests/Tracing/HttpClient/Fixtures/StreamableResponseInterface.php diff --git a/tests/Tracing/HttpClient/Fixtures/DestructibleResponseInterface.php b/tests/Tracing/HttpClient/Fixtures/DestructibleResponseInterface.php new file mode 100644 index 00000000..9c6d566a --- /dev/null +++ b/tests/Tracing/HttpClient/Fixtures/DestructibleResponseInterface.php @@ -0,0 +1,12 @@ +startChild($context); - $this->mockedResponse = $this->getMockBuilder(ResponseInterface::class) - ->addMethods(['__destruct']) - ->getMockForAbstractClass(); - + $this->mockedResponse = $this->createMock(DestructibleResponseInterface::class); $this->mockedResponse ->expects($this->once()) ->method('__destruct'); @@ -145,10 +144,7 @@ public function testToStream(): void self::markTestSkipped('Response toStream method is not existent in this version of http-client'); } - $this->mockedResponse = $this->getMockBuilder(ResponseInterface::class) - ->addMethods(['toStream']) - ->getMockForAbstractClass(); - + $this->mockedResponse = $this->createMock(StreamableResponseInterface::class); $this->mockedResponse ->expects($this->once()) ->method('toStream') From c4687c171d18f663c8c4eca6c0d9a59742f5ce41 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 22 Jun 2022 10:39:24 +0200 Subject: [PATCH 14/31] use ->assert instead of self::assert in phpunit tests --- .../Compiler/HttpClientTracingPassTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php index 16861636..37c1f97a 100644 --- a/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php +++ b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php @@ -38,7 +38,7 @@ public function testShouldNotDecorateHttpClientServicesIfDisabled(array $params) $container->addCompilerPass(new HttpClientTracingPass()); $container->compile(); - self::assertEquals(HttpClient::class, $container->getDefinition('http.client')->getClass()); + $this->assertEquals(HttpClient::class, $container->getDefinition('http.client')->getClass()); } /** @@ -63,7 +63,7 @@ public function testShouldDecorateHttpClients(): void $container->addCompilerPass(new HttpClientTracingPass()); $container->compile(); - self::assertEquals(TraceableHttpClient::class, $container->findDefinition('http.client')->getClass()); + $this->assertEquals(TraceableHttpClient::class, $container->findDefinition('http.client')->getClass()); } private static function isHttpClientPackageInstalled(): bool From 38faf38ef5e44fc6ce85dc8f054699fc1db9c1e2 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 22 Jun 2022 10:40:44 +0200 Subject: [PATCH 15/31] mark test classes as final --- .../DependencyInjection/Compiler/HttpClientTracingPassTest.php | 2 +- tests/Tracing/HttpClient/TraceableResponseTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php index 37c1f97a..4a45ab0e 100644 --- a/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php +++ b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php @@ -14,7 +14,7 @@ use Symfony\Component\HttpClient\HttpClient; use Symfony\Contracts\HttpClient\HttpClientInterface; -class HttpClientTracingPassTest extends TestCase +final class HttpClientTracingPassTest extends TestCase { public static function setUpBeforeClass(): void { diff --git a/tests/Tracing/HttpClient/TraceableResponseTest.php b/tests/Tracing/HttpClient/TraceableResponseTest.php index c465d732..e6ac07f8 100644 --- a/tests/Tracing/HttpClient/TraceableResponseTest.php +++ b/tests/Tracing/HttpClient/TraceableResponseTest.php @@ -16,7 +16,7 @@ use Symfony\Contracts\HttpClient\HttpClientInterface; use Symfony\Contracts\HttpClient\ResponseInterface; -class TraceableResponseTest extends TestCase +final class TraceableResponseTest extends TestCase { /** * @var MockObject&ResponseInterface From 0e2e9797cea05670b4366014f3836e33ccc13b07 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 22 Jun 2022 10:42:03 +0200 Subject: [PATCH 16/31] rename variable --- src/Tracing/HttpClient/AbstractTraceableResponse.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Tracing/HttpClient/AbstractTraceableResponse.php b/src/Tracing/HttpClient/AbstractTraceableResponse.php index b2ad3819..084f2b8b 100644 --- a/src/Tracing/HttpClient/AbstractTraceableResponse.php +++ b/src/Tracing/HttpClient/AbstractTraceableResponse.php @@ -103,13 +103,13 @@ public static function stream(HttpClientInterface $client, iterable $responses, /** @var \SplObjectStorage $traceableMap */ $traceableMap = new \SplObjectStorage(); - foreach ($responses as $r) { - if (!$r instanceof self) { - throw new \TypeError(sprintf('"%s::stream()" expects parameter 1 to be an iterable of TraceableResponse objects, "%s" given.', TraceableHttpClient::class, get_debug_type($r))); + foreach ($responses as $response) { + if (!$response instanceof self) { + throw new \TypeError(sprintf('"%s::stream()" expects parameter 1 to be an iterable of TraceableResponse objects, "%s" given.', TraceableHttpClient::class, get_debug_type($response))); } - $traceableMap[$r->response] = $r; - $wrappedResponses[] = $r->response; + $traceableMap[$response->response] = $response; + $wrappedResponses[] = $response->response; } foreach ($client->stream($wrappedResponses, $timeout) as $r => $chunk) { From b00333f1a22c65aca6d3eac82e947bdc90d4d035 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 22 Jun 2022 10:47:44 +0200 Subject: [PATCH 17/31] test http-client tracing enabled/disabled --- .github/workflows/tests.yaml | 2 +- src/Resources/config/schema/sentry-1.0.xsd | 5 +++++ .../Fixtures/php/http_client_tracing_enabled.php | 14 ++++++++++++++ .../Fixtures/xml/http_client_tracing_enabled.xml | 14 ++++++++++++++ .../Fixtures/yml/http_client_tracing_enabled.yml | 4 ++++ tests/DependencyInjection/SentryExtensionTest.php | 13 +++++++++++++ 6 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 tests/DependencyInjection/Fixtures/php/http_client_tracing_enabled.php create mode 100644 tests/DependencyInjection/Fixtures/xml/http_client_tracing_enabled.xml create mode 100644 tests/DependencyInjection/Fixtures/yml/http_client_tracing_enabled.yml diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 4abac4b7..0cbd7f5c 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -116,7 +116,7 @@ jobs: run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json" - name: Remove optional packages - run: composer remove doctrine/dbal doctrine/doctrine-bundle symfony/messenger symfony/twig-bundle symfony/cache --dev --no-update + run: composer remove doctrine/dbal doctrine/doctrine-bundle symfony/messenger symfony/twig-bundle symfony/cache symfony/http-client --dev --no-update - name: Install dependencies uses: ramsey/composer-install@v1 diff --git a/src/Resources/config/schema/sentry-1.0.xsd b/src/Resources/config/schema/sentry-1.0.xsd index 79a6aac4..9240c5c6 100644 --- a/src/Resources/config/schema/sentry-1.0.xsd +++ b/src/Resources/config/schema/sentry-1.0.xsd @@ -91,6 +91,7 @@ + @@ -117,4 +118,8 @@ + + + + diff --git a/tests/DependencyInjection/Fixtures/php/http_client_tracing_enabled.php b/tests/DependencyInjection/Fixtures/php/http_client_tracing_enabled.php new file mode 100644 index 00000000..639e2f9a --- /dev/null +++ b/tests/DependencyInjection/Fixtures/php/http_client_tracing_enabled.php @@ -0,0 +1,14 @@ +loadFromExtension('sentry', [ + 'tracing' => [ + 'http_client' => [ + 'enabled' => true, + ], + ], +]); diff --git a/tests/DependencyInjection/Fixtures/xml/http_client_tracing_enabled.xml b/tests/DependencyInjection/Fixtures/xml/http_client_tracing_enabled.xml new file mode 100644 index 00000000..cb0621ab --- /dev/null +++ b/tests/DependencyInjection/Fixtures/xml/http_client_tracing_enabled.xml @@ -0,0 +1,14 @@ + + + + + + + + + + diff --git a/tests/DependencyInjection/Fixtures/yml/http_client_tracing_enabled.yml b/tests/DependencyInjection/Fixtures/yml/http_client_tracing_enabled.yml new file mode 100644 index 00000000..50d56ede --- /dev/null +++ b/tests/DependencyInjection/Fixtures/yml/http_client_tracing_enabled.yml @@ -0,0 +1,4 @@ +sentry: + tracing: + http_client: + enabled: true diff --git a/tests/DependencyInjection/SentryExtensionTest.php b/tests/DependencyInjection/SentryExtensionTest.php index eaafea6d..e907e512 100644 --- a/tests/DependencyInjection/SentryExtensionTest.php +++ b/tests/DependencyInjection/SentryExtensionTest.php @@ -35,6 +35,7 @@ use Symfony\Component\DependencyInjection\ParameterBag\EnvPlaceholderParameterBag; use Symfony\Component\DependencyInjection\Reference; use Symfony\Component\ErrorHandler\Error\FatalError; +use Symfony\Component\HttpClient\HttpClient; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent; use Symfony\Component\Messenger\Event\WorkerMessageHandledEvent; @@ -380,6 +381,18 @@ public function testTwigTracingExtensionIsConfiguredWhenTwigTracingIsEnabled(): $this->assertTrue($container->hasDefinition(TwigTracingExtension::class)); } + public function testHttpClientTracingExtensionIsConfiguredWhenHttpClientTracingIsEnabled(): void + { + if (!class_exists(HttpClient::class)) { + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('Http client tracing support cannot be enabled because the symfony/http-client Composer package is not installed.'); + } + + $container = $this->createContainerFromFixture('http_client_tracing_enabled'); + + $this->assertTrue($container->getParameter('sentry.tracing.http_client.enabled')); + } + public function testTwigTracingExtensionIsRemovedWhenTwigTracingIsDisabled(): void { $container = $this->createContainerFromFixture('full'); From 4a28c72a4dc004c1ae820f2093e3aa7990944713 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Wed, 22 Jun 2022 10:57:12 +0200 Subject: [PATCH 18/31] add span description --- src/Tracing/HttpClient/AbstractTraceableHttpClient.php | 1 + tests/Tracing/HttpClient/TraceableHttpClientTest.php | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php index ff57b253..0d9840c5 100644 --- a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php +++ b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php @@ -52,6 +52,7 @@ public function request(string $method, string $url, array $options = []): Respo $context = new SpanContext(); $context->setOp('http.client'); + $context->setDescription('HTTP ' . $method); $context->setTags([ 'http.method' => $method, 'http.url' => $url, diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 24a2cb79..114e2793 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -77,6 +77,7 @@ public function testRequest(): void $this->assertCount(2, $spans); $this->assertNull($spans[1]->getEndTimestamp()); $this->assertSame('http.client', $spans[1]->getOp()); + $this->assertSame('HTTP POST', $spans[1]->getDescription()); $this->assertSame([ 'http.method' => 'POST', 'http.url' => 'http://www.example.org/test-page', @@ -88,6 +89,7 @@ public function testRequest(): void $this->assertCount(2, $spans); $this->assertNotNull($spans[1]->getEndTimestamp()); $this->assertSame('http.client', $spans[1]->getOp()); + $this->assertSame('HTTP POST', $spans[1]->getDescription()); $this->assertSame([ 'http.method' => 'POST', 'http.url' => 'http://www.example.org/test-page', From 0d63111aa424005b48aa4c7ae120a443283aa23b Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Thu, 23 Jun 2022 10:02:38 +0200 Subject: [PATCH 19/31] update CHANGELOG.md Co-authored-by: Alessandro Lai --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d83edd3..d56470cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- Add support for tracing of http client requests (#606) +- Add support for tracing of the Symfony HTTP client requests (#606) ## 4.3.0 (2022-05-30) - Fix compatibility issue with Symfony >= 6.1.0 (#635) From fd9f433801b5ad55c12e1c9f4d411b4b0fa7b40f Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Mon, 4 Jul 2022 18:49:45 +0200 Subject: [PATCH 20/31] Add a test for the `TraceableHttpClient::withOptions()` method --- .../HttpClient/TraceableHttpClientTest.php | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 114e2793..83320dd0 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -14,6 +14,8 @@ use Sentry\State\HubInterface; use Sentry\Tracing\Transaction; use Sentry\Tracing\TransactionContext; +use Symfony\Component\HttpClient\MockHttpClient; +use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Contracts\HttpClient\HttpClientInterface; use Symfony\Contracts\HttpClient\ResponseInterface; use Symfony\Contracts\Service\ResetInterface; @@ -114,6 +116,41 @@ public function testResetCallShouldBeForwardedToDecoratedInstance(): void $this->httpClient->reset(); } + public function testWithOptions(): void + { + $transaction = new Transaction(new TransactionContext()); + $transaction->initSpanRecorder(); + + $this->hub->expects($this->exactly(2)) + ->method('getSpan') + ->willReturn($transaction); + + $responses = [ + new MockResponse(), + new MockResponse(), + ]; + + $decoratedHttpClient = new MockHttpClient($responses, 'https://www.example.com'); + $httpClient1 = new TraceableHttpClient($decoratedHttpClient, $this->hub); + $httpClient2 = $httpClient1->withOptions(['base_uri' => 'https://www.example.org']); + + $this->assertNotSame($httpClient1, $httpClient2); + + $response = $httpClient1->request('GET', 'test-page'); + + $this->assertInstanceOf(AbstractTraceableResponse::class, $response); + $this->assertSame(200, $response->getStatusCode()); + $this->assertSame('GET', $response->getInfo('http_method')); + $this->assertSame('https://www.example.com/test-page', $response->getInfo('url')); + + $response = $httpClient2->request('GET', 'test-page'); + + $this->assertInstanceOf(AbstractTraceableResponse::class, $response); + $this->assertSame(200, $response->getStatusCode()); + $this->assertSame('GET', $response->getInfo('http_method')); + $this->assertSame('https://www.example.org/test-page', $response->getInfo('url')); + } + private static function isHttpClientPackageInstalled(): bool { return interface_exists(HttpClientInterface::class); From 39eaa7fe8666cbdece9f4a25a528a170d6d65ac9 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Tue, 5 Jul 2022 12:23:47 +0200 Subject: [PATCH 21/31] fix withOptions test for v4 --- src/Tracing/HttpClient/TraceableHttpClientForV4.php | 9 +++++++++ src/aliases.php | 3 ++- tests/DependencyInjection/Fixtures/php/full.php | 3 +++ tests/DependencyInjection/Fixtures/xml/full.xml | 1 + tests/DependencyInjection/Fixtures/yml/full.yml | 2 ++ tests/Tracing/HttpClient/TraceableHttpClientTest.php | 4 ++++ 6 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 src/Tracing/HttpClient/TraceableHttpClientForV4.php diff --git a/src/Tracing/HttpClient/TraceableHttpClientForV4.php b/src/Tracing/HttpClient/TraceableHttpClientForV4.php new file mode 100644 index 00000000..d9942993 --- /dev/null +++ b/src/Tracing/HttpClient/TraceableHttpClientForV4.php @@ -0,0 +1,9 @@ +=')) { class_alias(TraceableResponseForV6::class, TraceableResponse::class); class_alias(TraceableHttpClientForV6::class, TraceableHttpClient::class); diff --git a/tests/DependencyInjection/Fixtures/php/full.php b/tests/DependencyInjection/Fixtures/php/full.php index ae7f861e..82c71ef8 100644 --- a/tests/DependencyInjection/Fixtures/php/full.php +++ b/tests/DependencyInjection/Fixtures/php/full.php @@ -51,6 +51,9 @@ 'enabled' => false, 'connections' => ['default'], ], + 'http_client' => [ + 'enabled' => false, + ], 'twig' => [ 'enabled' => false, ], diff --git a/tests/DependencyInjection/Fixtures/xml/full.xml b/tests/DependencyInjection/Fixtures/xml/full.xml index cab3d83a..e9240f89 100644 --- a/tests/DependencyInjection/Fixtures/xml/full.xml +++ b/tests/DependencyInjection/Fixtures/xml/full.xml @@ -49,6 +49,7 @@ + app:command diff --git a/tests/DependencyInjection/Fixtures/yml/full.yml b/tests/DependencyInjection/Fixtures/yml/full.yml index 19981f21..e3c86290 100644 --- a/tests/DependencyInjection/Fixtures/yml/full.yml +++ b/tests/DependencyInjection/Fixtures/yml/full.yml @@ -50,6 +50,8 @@ sentry: enabled: false cache: enabled: false + http_client: + enabled: false console: excluded_commands: - app:command diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 83320dd0..1c48069b 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -118,6 +118,10 @@ public function testResetCallShouldBeForwardedToDecoratedInstance(): void public function testWithOptions(): void { + if (!method_exists(MockHttpClient::class, 'withOptions')) { + self::markTestSkipped(); + } + $transaction = new Transaction(new TransactionContext()); $transaction->initSpanRecorder(); From 993c49437556d1b7b69eb3052f67068a01546121 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Tue, 5 Jul 2022 12:54:14 +0200 Subject: [PATCH 22/31] add test for http-client stream method --- phpstan.neon | 1 + psalm.xml | 1 + .../HttpClient/TraceableHttpClientTest.php | 29 +++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/phpstan.neon b/phpstan.neon index 4ee88fdc..b3ba7ee8 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -12,6 +12,7 @@ parameters: - src/aliases.php - src/Tracing/Doctrine/DBAL/TracingStatementForV2.php - src/Tracing/Doctrine/DBAL/TracingDriverForV2.php + - src/Tracing/HttpClient/TraceableHttpClientForV4.php - src/Tracing/HttpClient/TraceableHttpClientForV5.php - tests/End2End/App - tests/Tracing/Doctrine/DBAL/TracingDriverForV2Test.php diff --git a/psalm.xml b/psalm.xml index abb144f4..89240539 100644 --- a/psalm.xml +++ b/psalm.xml @@ -11,6 +11,7 @@ + diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 1c48069b..4f503858 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -11,6 +11,7 @@ use Psr\Log\NullLogger; use Sentry\SentryBundle\Tracing\HttpClient\AbstractTraceableResponse; use Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClient; +use Sentry\SentryBundle\Tracing\HttpClient\TraceableResponse; use Sentry\State\HubInterface; use Sentry\Tracing\Transaction; use Sentry\Tracing\TransactionContext; @@ -155,6 +156,34 @@ public function testWithOptions(): void $this->assertSame('https://www.example.org/test-page', $response->getInfo('url')); } + public function testStream(): void + { + if (!method_exists(MockHttpClient::class, 'stream')) { + self::markTestSkipped(); + } + + $transaction = new Transaction(new TransactionContext()); + $transaction->initSpanRecorder(); + + $mockResponse = MockResponse::fromRequest('GET', 'https://www.example.com', [], new MockResponse('test_body', ['http_code' => 201])); + $decoratedHttpClient = new MockHttpClient([$mockResponse], 'https://www.example.com'); + $httpClient = new TraceableHttpClient($decoratedHttpClient, $this->hub); + $response = new TraceableResponse($decoratedHttpClient, $mockResponse, $transaction); + + $stream = $httpClient->stream([$response]); + $chunks = iterator_to_array($stream, false); + + $this->assertCount(3, $chunks); + + $this->assertNotNull($transaction->getSpanRecorder()); + $spans = $transaction->getSpanRecorder()->getSpans(); + $this->assertCount(1, $spans); + $this->assertNotNull($spans[0]->getEndTimestamp()); + + $this->assertEquals('test_body', $response->getContent(false)); + $this->assertEquals(201, $response->getStatusCode()); + } + private static function isHttpClientPackageInstalled(): bool { return interface_exists(HttpClientInterface::class); From f2f5adc24a107e79b8951e87670a763ec5a80b84 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Wed, 6 Jul 2022 23:46:26 +0200 Subject: [PATCH 23/31] Mark the `TraceableHttpClientFor*` classes as `@internal` --- src/Tracing/HttpClient/TraceableHttpClientForV4.php | 3 +++ src/Tracing/HttpClient/TraceableHttpClientForV5.php | 3 +++ src/Tracing/HttpClient/TraceableHttpClientForV6.php | 3 +++ 3 files changed, 9 insertions(+) diff --git a/src/Tracing/HttpClient/TraceableHttpClientForV4.php b/src/Tracing/HttpClient/TraceableHttpClientForV4.php index d9942993..63e31dd9 100644 --- a/src/Tracing/HttpClient/TraceableHttpClientForV4.php +++ b/src/Tracing/HttpClient/TraceableHttpClientForV4.php @@ -4,6 +4,9 @@ namespace Sentry\SentryBundle\Tracing\HttpClient; +/** + * @internal + */ final class TraceableHttpClientForV4 extends AbstractTraceableHttpClient { } diff --git a/src/Tracing/HttpClient/TraceableHttpClientForV5.php b/src/Tracing/HttpClient/TraceableHttpClientForV5.php index 3fcbad29..60afb497 100644 --- a/src/Tracing/HttpClient/TraceableHttpClientForV5.php +++ b/src/Tracing/HttpClient/TraceableHttpClientForV5.php @@ -4,6 +4,9 @@ namespace Sentry\SentryBundle\Tracing\HttpClient; +/** + * @internal + */ final class TraceableHttpClientForV5 extends AbstractTraceableHttpClient { /** diff --git a/src/Tracing/HttpClient/TraceableHttpClientForV6.php b/src/Tracing/HttpClient/TraceableHttpClientForV6.php index 4116d476..ab9e0c8e 100644 --- a/src/Tracing/HttpClient/TraceableHttpClientForV6.php +++ b/src/Tracing/HttpClient/TraceableHttpClientForV6.php @@ -4,6 +4,9 @@ namespace Sentry\SentryBundle\Tracing\HttpClient; +/** + * @internal + */ final class TraceableHttpClientForV6 extends AbstractTraceableHttpClient { /** From e732d59689ee0ba9526794d9fc555c11364b56d2 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 7 Jul 2022 01:30:15 +0200 Subject: [PATCH 24/31] Minor methods and variables names changes --- .../HttpClient/TraceableHttpClientTest.php | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 4f503858..6d656c13 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -31,7 +31,7 @@ final class TraceableHttpClientTest extends TestCase /** * @var MockObject&HttpClientInterface&LoggerAwareInterface&ResetInterface */ - private $client; + private $decoratedHttpClient; /** * @var TraceableHttpClient @@ -48,8 +48,8 @@ public static function setUpBeforeClass(): void protected function setUp(): void { $this->hub = $this->createMock(HubInterface::class); - $this->client = $this->createMock(TestableHttpClientInterface::class); - $this->httpClient = new TraceableHttpClient($this->client, $this->hub); + $this->decoratedHttpClient = $this->createMock(TestableHttpClientInterface::class); + $this->httpClient = new TraceableHttpClient($this->decoratedHttpClient, $this->hub); } public function testRequest(): void @@ -62,7 +62,7 @@ public function testRequest(): void ->willReturn($transaction); $response = $this->createMock(ResponseInterface::class); - $this->client->expects($this->once()) + $this->decoratedHttpClient->expects($this->once()) ->method('request') ->with('POST', 'http://www.example.org/test-page', new Callback(function ($value) use ($transaction) { $this->assertArrayHasKey('headers', $value); @@ -102,16 +102,17 @@ public function testRequest(): void public function testSetLoggerShouldBeForwardedToDecoratedInstance(): void { $logger = new NullLogger(); - $this->client->expects($this->once()) + + $this->decoratedHttpClient->expects($this->once()) ->method('setLogger') ->with($logger); $this->httpClient->setLogger($logger); } - public function testResetCallShouldBeForwardedToDecoratedInstance(): void + public function testReset(): void { - $this->client->expects($this->once()) + $this->decoratedHttpClient->expects($this->once()) ->method('reset'); $this->httpClient->reset(); From c6d5f2b1dac7eeab64d662c263a71777bb5512b2 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 7 Jul 2022 01:29:25 +0200 Subject: [PATCH 25/31] Improve the test for the `TraceableHttpClient::stream()` method --- .../AbstractTraceableHttpClient.php | 5 +- .../HttpClient/AbstractTraceableResponse.php | 25 +++---- .../HttpClient/TraceableHttpClientTest.php | 71 +++++++++++-------- 3 files changed, 57 insertions(+), 44 deletions(-) diff --git a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php index 0d9840c5..9dd104ef 100644 --- a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php +++ b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php @@ -30,7 +30,7 @@ abstract class AbstractTraceableHttpClient implements HttpClientInterface, Reset /** * @var HubInterface */ - private $hub; + protected $hub; public function __construct(HttpClientInterface $client, HubInterface $hub) { @@ -45,6 +45,7 @@ public function request(string $method, string $url, array $options = []): Respo { $span = null; $parent = $this->hub->getSpan(); + if (null !== $parent) { $headers = $options['headers'] ?? []; $headers['sentry-trace'] = $parent->toTraceparent(); @@ -71,8 +72,6 @@ public function stream($responses, float $timeout = null): ResponseStreamInterfa { if ($responses instanceof AbstractTraceableResponse) { $responses = [$responses]; - } elseif (!is_iterable($responses)) { - throw new \TypeError(sprintf('"%s()" expects parameter 1 to be an iterable of TraceableResponse objects, "%s" given.', __METHOD__, get_debug_type($responses))); } return new ResponseStream(AbstractTraceableResponse::stream($this->client, $responses, $timeout)); diff --git a/src/Tracing/HttpClient/AbstractTraceableResponse.php b/src/Tracing/HttpClient/AbstractTraceableResponse.php index 084f2b8b..bc8081c3 100644 --- a/src/Tracing/HttpClient/AbstractTraceableResponse.php +++ b/src/Tracing/HttpClient/AbstractTraceableResponse.php @@ -28,7 +28,7 @@ abstract class AbstractTraceableResponse implements ResponseInterface /** * @var Span|null */ - private $span; + protected $span; public function __construct(HttpClientInterface $client, ResponseInterface $response, ?Span $span) { @@ -54,7 +54,7 @@ public function __destruct() $this->response->__destruct(); } } finally { - $this->finish(); + $this->finishSpan(); } } @@ -73,7 +73,7 @@ public function getContent(bool $throw = true): string try { return $this->response->getContent($throw); } finally { - $this->finish(); + $this->finishSpan(); } } @@ -82,26 +82,28 @@ public function toArray(bool $throw = true): array try { return $this->response->toArray($throw); } finally { - $this->finish(); + $this->finishSpan(); } } public function cancel(): void { $this->response->cancel(); - $this->finish(); + $this->finishSpan(); } /** * @internal * + * @param iterable $responses + * * @return \Generator */ public static function stream(HttpClientInterface $client, iterable $responses, ?float $timeout): \Generator { - $wrappedResponses = []; /** @var \SplObjectStorage $traceableMap */ $traceableMap = new \SplObjectStorage(); + $wrappedResponses = []; foreach ($responses as $response) { if (!$response instanceof self) { @@ -112,16 +114,15 @@ public static function stream(HttpClientInterface $client, iterable $responses, $wrappedResponses[] = $response->response; } - foreach ($client->stream($wrappedResponses, $timeout) as $r => $chunk) { - if (null !== $traceableMap[$r]->span) { - $traceableMap[$r]->span->finish(); - } + foreach ($client->stream($wrappedResponses, $timeout) as $response => $chunk) { + $traceableResponse = $traceableMap[$response]; + $traceableResponse->finishSpan(); - yield $traceableMap[$r] => $chunk; + yield $traceableResponse => $chunk; } } - private function finish(): void + private function finishSpan(): void { if (null !== $this->span) { $this->span->finish(); diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 6d656c13..ed165254 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -11,7 +11,6 @@ use Psr\Log\NullLogger; use Sentry\SentryBundle\Tracing\HttpClient\AbstractTraceableResponse; use Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClient; -use Sentry\SentryBundle\Tracing\HttpClient\TraceableResponse; use Sentry\State\HubInterface; use Sentry\Tracing\Transaction; use Sentry\Tracing\TransactionContext; @@ -99,6 +98,48 @@ public function testRequest(): void ], $spans[1]->getTags()); } + public function testStream(): void + { + $transaction = new Transaction(new TransactionContext()); + $transaction->initSpanRecorder(); + + $this->hub->expects($this->once()) + ->method('getSpan') + ->willReturn($transaction); + + $decoratedHttpClient = new MockHttpClient(new MockResponse(['foo', 'bar'])); + $httpClient = new TraceableHttpClient($decoratedHttpClient, $this->hub); + $response = $httpClient->request('GET', 'https://www.example.com/test-page'); + $chunks = []; + + foreach ($httpClient->stream($response) as $chunkResponse => $chunk) { + $this->assertSame($response, $chunkResponse); + + $chunks[] = $chunk->getContent(); + } + + $spans = $transaction->getSpanRecorder()->getSpans(); + $expectedTags = [ + 'http.method' => 'GET', + 'http.url' => 'https://www.example.com/test-page', + ]; + + $this->assertSame('foobar', implode('', $chunks)); + $this->assertCount(2, $spans); + $this->assertNotNull($spans[1]->getEndTimestamp()); + $this->assertSame('http.client', $spans[1]->getOp()); + $this->assertSame('HTTP GET', $spans[1]->getDescription()); + $this->assertSame($expectedTags, $spans[1]->getTags()); + + $loopIndex = 0; + + foreach ($httpClient->stream($response) as $chunk) { + ++$loopIndex; + } + + $this->assertSame(1, $loopIndex); + } + public function testSetLoggerShouldBeForwardedToDecoratedInstance(): void { $logger = new NullLogger(); @@ -157,34 +198,6 @@ public function testWithOptions(): void $this->assertSame('https://www.example.org/test-page', $response->getInfo('url')); } - public function testStream(): void - { - if (!method_exists(MockHttpClient::class, 'stream')) { - self::markTestSkipped(); - } - - $transaction = new Transaction(new TransactionContext()); - $transaction->initSpanRecorder(); - - $mockResponse = MockResponse::fromRequest('GET', 'https://www.example.com', [], new MockResponse('test_body', ['http_code' => 201])); - $decoratedHttpClient = new MockHttpClient([$mockResponse], 'https://www.example.com'); - $httpClient = new TraceableHttpClient($decoratedHttpClient, $this->hub); - $response = new TraceableResponse($decoratedHttpClient, $mockResponse, $transaction); - - $stream = $httpClient->stream([$response]); - $chunks = iterator_to_array($stream, false); - - $this->assertCount(3, $chunks); - - $this->assertNotNull($transaction->getSpanRecorder()); - $spans = $transaction->getSpanRecorder()->getSpans(); - $this->assertCount(1, $spans); - $this->assertNotNull($spans[0]->getEndTimestamp()); - - $this->assertEquals('test_body', $response->getContent(false)); - $this->assertEquals(201, $response->getStatusCode()); - } - private static function isHttpClientPackageInstalled(): bool { return interface_exists(HttpClientInterface::class); From 8a824d7ff23c4d67a9dab5645f6c63c27f192874 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 7 Jul 2022 02:25:21 +0200 Subject: [PATCH 26/31] Improve the tests for the `TraceableResponse` class --- .../HttpClient/AbstractTraceableResponse.php | 20 ++-- .../DestructibleResponseInterface.php | 12 -- .../HttpClient/TraceableResponseTest.php | 110 +++++++----------- 3 files changed, 54 insertions(+), 88 deletions(-) delete mode 100644 tests/Tracing/HttpClient/Fixtures/DestructibleResponseInterface.php diff --git a/src/Tracing/HttpClient/AbstractTraceableResponse.php b/src/Tracing/HttpClient/AbstractTraceableResponse.php index bc8081c3..46e44c45 100644 --- a/src/Tracing/HttpClient/AbstractTraceableResponse.php +++ b/src/Tracing/HttpClient/AbstractTraceableResponse.php @@ -37,16 +37,6 @@ public function __construct(HttpClientInterface $client, ResponseInterface $resp $this->span = $span; } - public function __sleep(): array - { - throw new \BadMethodCallException('Cannot serialize ' . __CLASS__); - } - - public function __wakeup() - { - throw new \BadMethodCallException('Cannot unserialize ' . __CLASS__); - } - public function __destruct() { try { @@ -58,6 +48,16 @@ public function __destruct() } } + public function __sleep(): array + { + throw new \BadMethodCallException('Serializing instances of this class is forbidden.'); + } + + public function __wakeup() + { + throw new \BadMethodCallException('Unserializing instances of this class is forbidden.'); + } + public function getStatusCode(): int { return $this->response->getStatusCode(); diff --git a/tests/Tracing/HttpClient/Fixtures/DestructibleResponseInterface.php b/tests/Tracing/HttpClient/Fixtures/DestructibleResponseInterface.php deleted file mode 100644 index 9c6d566a..00000000 --- a/tests/Tracing/HttpClient/Fixtures/DestructibleResponseInterface.php +++ /dev/null @@ -1,12 +0,0 @@ -mockedResponse = $this->createMock(ResponseInterface::class); $this->client = $this->createMock(HttpClientInterface::class); $this->hub = $this->createMock(HubInterface::class); - $this->response = new TraceableResponse($this->client, $this->mockedResponse, null); } - public function testCannotBeSerialized(): void + public function testInstanceCannotBeSerialized(): void { $this->expectException(\BadMethodCallException::class); - serialize($this->response); + $this->expectExceptionMessage('Serializing instances of this class is forbidden.'); + + serialize(new TraceableResponse($this->client, new MockResponse(), null)); } - public function testCannotBeDeserialized(): void + public function testInstanceCannotBeUnserialized(): void { $this->expectException(\BadMethodCallException::class); + $this->expectExceptionMessage('Unserializing instances of this class is forbidden.'); + unserialize(sprintf('O:%u:"%s":0:{}', \strlen(TraceableResponse::class), TraceableResponse::class)); } @@ -63,16 +56,10 @@ public function testDestructor(): void $transaction = new Transaction(new TransactionContext(), $this->hub); $context = new SpanContext(); $span = $transaction->startChild($context); - - $this->mockedResponse = $this->createMock(DestructibleResponseInterface::class); - $this->mockedResponse - ->expects($this->once()) - ->method('__destruct'); - - $this->response = new TraceableResponse($this->client, $this->mockedResponse, $span); + $response = new TraceableResponse($this->client, new MockResponse(), $span); // Call gc to invoke destructors at the right time. - unset($this->response); + unset($response); gc_mem_caches(); gc_collect_cycles(); @@ -80,77 +67,68 @@ public function testDestructor(): void $this->assertNotNull($span->getEndTimestamp()); } - public function testGetHeaders(): void + public function testGetStatusCode(): void { - $this->mockedResponse - ->expects($this->once()) - ->method('getHeaders') - ->with(true); + $response = new TraceableResponse($this->client, new MockResponse(), null); - $this->response->getHeaders(true); + $this->assertSame(200, $response->getStatusCode()); } - public function testGetStatusCode(): void + public function testGetHeaders(): void { - $this->mockedResponse - ->expects($this->once()) - ->method('getStatusCode'); + $expectedHeaders = ['content-length' => ['0']]; + $response = new TraceableResponse($this->client, new MockResponse('', ['response_headers' => $expectedHeaders]), null); - $this->response->getStatusCode(); + $this->assertSame($expectedHeaders, $response->getHeaders()); } public function testGetContent(): void { - $this->mockedResponse - ->expects($this->once()) - ->method('getContent') - ->with(false); + $span = new Span(); + $httpClient = new MockHttpClient(new MockResponse('foobar')); + $response = new TraceableResponse($httpClient, $httpClient->request('GET', '/'), $span); - $this->response->getContent(false); + $this->assertSame('foobar', $response->getContent()); + $this->assertNotNull($span->getEndTimestamp()); } public function testToArray(): void { - $this->mockedResponse - ->expects($this->once()) - ->method('toArray') - ->with(false); + $span = new Span(); + $httpClient = new MockHttpClient(new MockResponse('{"foo":"bar"}')); + $response = new TraceableResponse($this->client, $httpClient->request('GET', '/'), $span); - $this->response->toArray(false); + $this->assertSame(['foo' => 'bar'], $response->toArray()); + $this->assertNotNull($span->getEndTimestamp()); } public function testCancel(): void { - $this->mockedResponse - ->expects($this->once()) - ->method('cancel'); + $span = new Span(); + $response = new TraceableResponse($this->client, new MockResponse(), $span); + + $response->cancel(); - $this->response->cancel(); + $this->assertTrue($response->getInfo('canceled')); + $this->assertNotNull($span->getEndTimestamp()); } public function testGetInfo(): void { - $this->mockedResponse - ->expects($this->once()) - ->method('getInfo') - ->with('type'); + $response = new TraceableResponse($this->client, new MockResponse(), null); - $this->response->getInfo('type'); + $this->assertSame(200, $response->getInfo('http_code')); } public function testToStream(): void { - if (!method_exists($this->response, 'toStream')) { - self::markTestSkipped('Response toStream method is not existent in this version of http-client'); - } + $httpClient = new MockHttpClient(new MockResponse('foobar')); + $response = new TraceableResponse($this->client, $httpClient->request('GET', '/'), null); - $this->mockedResponse = $this->createMock(StreamableResponseInterface::class); - $this->mockedResponse - ->expects($this->once()) - ->method('toStream') - ->with(false); + if (!method_exists($response, 'toStream')) { + $this->markTestSkipped('The TraceableResponse::toStream() method is not supported'); + } - $this->response = new TraceableResponse($this->client, $this->mockedResponse, null); - $this->response->toStream(false); + $this->assertSame('foobar', stream_get_contents($response->toStream())); } } From 27d9d539ece4502ea4d7528f4801a189bbb88813 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Mon, 11 Jul 2022 15:15:00 +0200 Subject: [PATCH 27/31] fix tests for lower deps --- .../Fixtures/StreamableResponseInterface.php | 12 ------------ tests/Tracing/HttpClient/TraceableResponseTest.php | 7 +++---- 2 files changed, 3 insertions(+), 16 deletions(-) delete mode 100644 tests/Tracing/HttpClient/Fixtures/StreamableResponseInterface.php diff --git a/tests/Tracing/HttpClient/Fixtures/StreamableResponseInterface.php b/tests/Tracing/HttpClient/Fixtures/StreamableResponseInterface.php deleted file mode 100644 index cb67babb..00000000 --- a/tests/Tracing/HttpClient/Fixtures/StreamableResponseInterface.php +++ /dev/null @@ -1,12 +0,0 @@ -request('GET', '/'), $span); + $response = new TraceableResponse($httpClient, $httpClient->request('GET', 'https://www.example.org/'), $span); $this->assertSame('foobar', $response->getContent()); $this->assertNotNull($span->getEndTimestamp()); @@ -96,7 +95,7 @@ public function testToArray(): void { $span = new Span(); $httpClient = new MockHttpClient(new MockResponse('{"foo":"bar"}')); - $response = new TraceableResponse($this->client, $httpClient->request('GET', '/'), $span); + $response = new TraceableResponse($this->client, $httpClient->request('GET', 'https://www.example.org/'), $span); $this->assertSame(['foo' => 'bar'], $response->toArray()); $this->assertNotNull($span->getEndTimestamp()); @@ -123,7 +122,7 @@ public function testGetInfo(): void public function testToStream(): void { $httpClient = new MockHttpClient(new MockResponse('foobar')); - $response = new TraceableResponse($this->client, $httpClient->request('GET', '/'), null); + $response = new TraceableResponse($this->client, $httpClient->request('GET', 'https://www.example.org/'), null); if (!method_exists($response, 'toStream')) { $this->markTestSkipped('The TraceableResponse::toStream() method is not supported'); From dcea7b57ee8a39328c42e433c10b967dd1e566cc Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Mon, 11 Jul 2022 15:32:13 +0200 Subject: [PATCH 28/31] fix static analysis --- phpstan-baseline.neon | 5 ----- src/Tracing/HttpClient/AbstractTraceableHttpClient.php | 2 ++ tests/Tracing/HttpClient/TraceableHttpClientTest.php | 2 ++ 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index df52dd92..92e2fa06 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -280,11 +280,6 @@ parameters: count: 1 path: src/Tracing/HttpClient/AbstractTraceableResponse.php - - - message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableResponse::stream\\(\\) has parameter \\$responses with no value type specified in iterable type iterable.$#" - count: 1 - path: src/Tracing/HttpClient/AbstractTraceableResponse.php - - message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\TraceableHttpClientForV6::withOptions\\(\\) has parameter \\$options with no value type specified in iterable type array.$#" count: 1 diff --git a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php index 9dd104ef..3ad27ad4 100644 --- a/src/Tracing/HttpClient/AbstractTraceableHttpClient.php +++ b/src/Tracing/HttpClient/AbstractTraceableHttpClient.php @@ -72,6 +72,8 @@ public function stream($responses, float $timeout = null): ResponseStreamInterfa { if ($responses instanceof AbstractTraceableResponse) { $responses = [$responses]; + } elseif (!is_iterable($responses)) { + throw new \TypeError(sprintf('"%s()" expects parameter 1 to be an iterable of TraceableResponse objects, "%s" given.', __METHOD__, get_debug_type($responses))); } return new ResponseStream(AbstractTraceableResponse::stream($this->client, $responses, $timeout)); diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index ed165254..069d8682 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -87,6 +87,7 @@ public function testRequest(): void $response->getContent(false); + $this->assertNotNull($transaction->getSpanRecorder()); $spans = $transaction->getSpanRecorder()->getSpans(); $this->assertCount(2, $spans); $this->assertNotNull($spans[1]->getEndTimestamp()); @@ -118,6 +119,7 @@ public function testStream(): void $chunks[] = $chunk->getContent(); } + $this->assertNotNull($transaction->getSpanRecorder()); $spans = $transaction->getSpanRecorder()->getSpans(); $expectedTags = [ 'http.method' => 'GET', From 71ba653a9c522839ca1199ab1db94b4f6fea1562 Mon Sep 17 00:00:00 2001 From: Alessandro Chitolina Date: Tue, 12 Jul 2022 11:23:41 +0200 Subject: [PATCH 29/31] add error conditions tests for stream methods --- phpstan-baseline.neon | 10 ++++++++++ tests/Tracing/HttpClient/TraceableHttpClientTest.php | 7 +++++++ tests/Tracing/HttpClient/TraceableResponseTest.php | 10 ++++++++++ 3 files changed, 27 insertions(+) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 92e2fa06..a27a5a7c 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -285,6 +285,16 @@ parameters: count: 1 path: src/Tracing/HttpClient/TraceableHttpClientForV6.php + - + message: "#^Parameter \\#1 $responses of method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableHttpClient::stream\\(\\) expects iterable<\\(int\\|string\\), Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface>|Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface, stdClass given\\.$#" + count: 1 + path: tests/Tracing/HttpClient/TraceableHttpClientTest.php + + - + message: "#^Parameter \\#2 \\$responses of static method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableResponse::stream\\(\\) expects iterable, array given\\.$#" + count: 1 + path: tests/Tracing/HttpClient/TraceableResponseTest.php + - message: "#^Class Symfony\\\\Component\\\\Debug\\\\Exception\\\\FatalErrorException not found\\.$#" count: 1 diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 069d8682..9a1551b3 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -142,6 +142,13 @@ public function testStream(): void $this->assertSame(1, $loopIndex); } + public function testStreamShouldThrowOnWrongParameterType(): void + { + $this->expectException(\TypeError::class); + $this->expectExceptionMessage('"Sentry\SentryBundle\Tracing\HttpClient\AbstractTraceableHttpClient::stream()" expects parameter 1 to be an iterable of TraceableResponse objects, "stdClass" given.'); + $this->httpClient->stream(new \stdClass()); + } + public function testSetLoggerShouldBeForwardedToDecoratedInstance(): void { $logger = new NullLogger(); diff --git a/tests/Tracing/HttpClient/TraceableResponseTest.php b/tests/Tracing/HttpClient/TraceableResponseTest.php index 1f2b213a..69a80612 100644 --- a/tests/Tracing/HttpClient/TraceableResponseTest.php +++ b/tests/Tracing/HttpClient/TraceableResponseTest.php @@ -15,6 +15,7 @@ use Symfony\Component\HttpClient\MockHttpClient; use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Contracts\HttpClient\HttpClientInterface; +use Symfony\Contracts\HttpClient\ResponseInterface; final class TraceableResponseTest extends TestCase { @@ -130,4 +131,13 @@ public function testToStream(): void $this->assertSame('foobar', stream_get_contents($response->toStream())); } + + public function testStreamWithWrongObjectsShouldThrow(): void + { + $httpClient = new MockHttpClient(new MockResponse('foobar')); + $response = $this->createMock(ResponseInterface::class); + + $this->expectException(\TypeError::class); + iterator_to_array(TraceableResponse::stream($httpClient, [$response], null)); + } } From 0ae25240608e6666daae82619fb35626161f6b5c Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 14 Jul 2022 17:56:13 +0200 Subject: [PATCH 30/31] Minor improvements to the tests --- .../HttpClient/AbstractTraceableResponse.php | 1 - .../HttpClient/TraceableHttpClientTest.php | 56 +++++++------------ .../HttpClient/TraceableResponseTest.php | 10 ++-- 3 files changed, 25 insertions(+), 42 deletions(-) diff --git a/src/Tracing/HttpClient/AbstractTraceableResponse.php b/src/Tracing/HttpClient/AbstractTraceableResponse.php index 46e44c45..71571294 100644 --- a/src/Tracing/HttpClient/AbstractTraceableResponse.php +++ b/src/Tracing/HttpClient/AbstractTraceableResponse.php @@ -5,7 +5,6 @@ namespace Sentry\SentryBundle\Tracing\HttpClient; use Sentry\Tracing\Span; -use Symfony\Component\HttpClient\TraceableHttpClient; use Symfony\Contracts\HttpClient\ChunkInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; use Symfony\Contracts\HttpClient\ResponseInterface; diff --git a/tests/Tracing/HttpClient/TraceableHttpClientTest.php b/tests/Tracing/HttpClient/TraceableHttpClientTest.php index 9a1551b3..ba9da822 100644 --- a/tests/Tracing/HttpClient/TraceableHttpClientTest.php +++ b/tests/Tracing/HttpClient/TraceableHttpClientTest.php @@ -4,7 +4,6 @@ namespace Sentry\SentryBundle\Tests\Tracing\HttpClient; -use PHPUnit\Framework\Constraint\Callback; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerAwareInterface; @@ -17,7 +16,6 @@ use Symfony\Component\HttpClient\MockHttpClient; use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Contracts\HttpClient\HttpClientInterface; -use Symfony\Contracts\HttpClient\ResponseInterface; use Symfony\Contracts\Service\ResetInterface; final class TraceableHttpClientTest extends TestCase @@ -28,7 +26,7 @@ final class TraceableHttpClientTest extends TestCase private $hub; /** - * @var MockObject&HttpClientInterface&LoggerAwareInterface&ResetInterface + * @var MockObject&TestableHttpClientInterface */ private $decoratedHttpClient; @@ -60,43 +58,29 @@ public function testRequest(): void ->method('getSpan') ->willReturn($transaction); - $response = $this->createMock(ResponseInterface::class); - $this->decoratedHttpClient->expects($this->once()) - ->method('request') - ->with('POST', 'http://www.example.org/test-page', new Callback(function ($value) use ($transaction) { - $this->assertArrayHasKey('headers', $value); - - return ['sentry-trace' => $transaction->toTraceparent()] === $value['headers']; - })) - ->willReturn($response); - - $response = $this->httpClient->request('POST', 'http://www.example.org/test-page', []); + $mockResponse = new MockResponse(); + $decoratedHttpClient = new MockHttpClient($mockResponse); + $httpClient = new TraceableHttpClient($decoratedHttpClient, $this->hub); + $response = $httpClient->request('GET', 'https://www.example.com/test-page'); $this->assertInstanceOf(AbstractTraceableResponse::class, $response); + $this->assertSame(200, $response->getStatusCode()); + $this->assertSame('GET', $response->getInfo('http_method')); + $this->assertSame('https://www.example.com/test-page', $response->getInfo('url')); + $this->assertSame(['sentry-trace: ' . $transaction->toTraceparent()], $mockResponse->getRequestOptions()['normalized_headers']['sentry-trace']); $this->assertNotNull($transaction->getSpanRecorder()); + $spans = $transaction->getSpanRecorder()->getSpans(); + $expectedTags = [ + 'http.method' => 'GET', + 'http.url' => 'https://www.example.com/test-page', + ]; $this->assertCount(2, $spans); $this->assertNull($spans[1]->getEndTimestamp()); $this->assertSame('http.client', $spans[1]->getOp()); - $this->assertSame('HTTP POST', $spans[1]->getDescription()); - $this->assertSame([ - 'http.method' => 'POST', - 'http.url' => 'http://www.example.org/test-page', - ], $spans[1]->getTags()); - - $response->getContent(false); - - $this->assertNotNull($transaction->getSpanRecorder()); - $spans = $transaction->getSpanRecorder()->getSpans(); - $this->assertCount(2, $spans); - $this->assertNotNull($spans[1]->getEndTimestamp()); - $this->assertSame('http.client', $spans[1]->getOp()); - $this->assertSame('HTTP POST', $spans[1]->getDescription()); - $this->assertSame([ - 'http.method' => 'POST', - 'http.url' => 'http://www.example.org/test-page', - ], $spans[1]->getTags()); + $this->assertSame('HTTP GET', $spans[1]->getDescription()); + $this->assertSame($expectedTags, $spans[1]->getTags()); } public function testStream(): void @@ -120,6 +104,7 @@ public function testStream(): void } $this->assertNotNull($transaction->getSpanRecorder()); + $spans = $transaction->getSpanRecorder()->getSpans(); $expectedTags = [ 'http.method' => 'GET', @@ -142,14 +127,15 @@ public function testStream(): void $this->assertSame(1, $loopIndex); } - public function testStreamShouldThrowOnWrongParameterType(): void + public function testStreamThrowsExceptionIfResponsesArgumentIsInvalid(): void { $this->expectException(\TypeError::class); - $this->expectExceptionMessage('"Sentry\SentryBundle\Tracing\HttpClient\AbstractTraceableHttpClient::stream()" expects parameter 1 to be an iterable of TraceableResponse objects, "stdClass" given.'); + $this->expectExceptionMessage('"Sentry\\SentryBundle\\Tracing\\HttpClient\\AbstractTraceableHttpClient::stream()" expects parameter 1 to be an iterable of TraceableResponse objects, "stdClass" given.'); + $this->httpClient->stream(new \stdClass()); } - public function testSetLoggerShouldBeForwardedToDecoratedInstance(): void + public function testSetLogger(): void { $logger = new NullLogger(); diff --git a/tests/Tracing/HttpClient/TraceableResponseTest.php b/tests/Tracing/HttpClient/TraceableResponseTest.php index 69a80612..e67616d5 100644 --- a/tests/Tracing/HttpClient/TraceableResponseTest.php +++ b/tests/Tracing/HttpClient/TraceableResponseTest.php @@ -15,7 +15,6 @@ use Symfony\Component\HttpClient\MockHttpClient; use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Contracts\HttpClient\HttpClientInterface; -use Symfony\Contracts\HttpClient\ResponseInterface; final class TraceableResponseTest extends TestCase { @@ -132,12 +131,11 @@ public function testToStream(): void $this->assertSame('foobar', stream_get_contents($response->toStream())); } - public function testStreamWithWrongObjectsShouldThrow(): void + public function testStreamThrowsExceptionIfResponsesArgumentIsInvalid(): void { - $httpClient = new MockHttpClient(new MockResponse('foobar')); - $response = $this->createMock(ResponseInterface::class); - $this->expectException(\TypeError::class); - iterator_to_array(TraceableResponse::stream($httpClient, [$response], null)); + $this->expectExceptionMessage('"Sentry\\SentryBundle\\Tracing\\HttpClient\\TraceableHttpClient::stream()" expects parameter 1 to be an iterable of TraceableResponse objects, "stdClass" given.'); + + iterator_to_array(TraceableResponse::stream($this->client, [new \stdClass()], null)); } } From d9a3a71b72d6f616797cc8affc7f7ee459f4583c Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 14 Jul 2022 18:15:49 +0200 Subject: [PATCH 31/31] Improve the tests for the `HttpClientTracingPass` class --- phpstan-baseline.neon | 62 +++++++++--------- .../Compiler/HttpClientTracingPass.php | 15 ++--- .../Compiler/HttpClientTracingPassTest.php | 64 +++++++++++-------- 3 files changed, 74 insertions(+), 67 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index a27a5a7c..aa34e31c 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -105,6 +105,11 @@ parameters: count: 1 path: src/DependencyInjection/SentryExtension.php + - + message: "#^Parameter \\#2 \\$config of method Sentry\\\\SentryBundle\\\\DependencyInjection\\\\SentryExtension\\:\\:registerHttpClientTracingConfiguration\\(\\) expects array\\, mixed given\\.$#" + count: 1 + path: src/DependencyInjection/SentryExtension.php + - message: "#^Parameter \\#2 \\$config of method Sentry\\\\SentryBundle\\\\DependencyInjection\\\\SentryExtension\\:\\:registerMessengerListenerConfiguration\\(\\) expects array\\, mixed given\\.$#" count: 1 @@ -135,11 +140,6 @@ parameters: count: 1 path: src/DependencyInjection/SentryExtension.php - - - message: "#^Parameter \\#2 \\$config of method Sentry\\\\SentryBundle\\\\DependencyInjection\\\\SentryExtension\\:\\:registerHttpClientTracingConfiguration\\(\\) expects array, mixed given\\.$#" - count: 1 - path: src/DependencyInjection/SentryExtension.php - - message: "#^Parameter \\#2 \\$value of method Symfony\\\\Component\\\\DependencyInjection\\\\Container\\:\\:setParameter\\(\\) expects array\\|bool\\|float\\|int\\|string\\|UnitEnum\\|null, mixed given\\.$#" count: 2 @@ -241,59 +241,59 @@ parameters: path: src/Tracing/Doctrine/DBAL/TracingStatementForV3.php - - message: "#^Cannot access offset 'sample_rate' on mixed\\.$#" + message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableHttpClient\\:\\:request\\(\\) has parameter \\$options with no value type specified in iterable type array\\.$#" count: 1 - path: tests/DependencyInjection/ConfigurationTest.php + path: src/Tracing/HttpClient/AbstractTraceableHttpClient.php - - message: "#^Cannot access offset 'traces_sample_rate' on mixed\\.$#" + message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableResponse\\:\\:toArray\\(\\) return type has no value type specified in iterable type array\\.$#" count: 1 - path: tests/DependencyInjection/ConfigurationTest.php + path: src/Tracing/HttpClient/AbstractTraceableResponse.php - - message: "#^Argument of an invalid type mixed supplied for foreach, only iterables are supported\\.$#" + message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\TraceableHttpClientForV6\\:\\:withOptions\\(\\) has parameter \\$options with no value type specified in iterable type array\\.$#" count: 1 - path: tests/DependencyInjection/SentryExtensionTest.php + path: src/Tracing/HttpClient/TraceableHttpClientForV6.php - - message: "#^Cannot access offset 'dsn' on mixed\\.$#" + message: "#^Call to an undefined method Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface\\:\\:toStream\\(\\)\\.$#" count: 1 - path: tests/DependencyInjection/SentryExtensionTest.php + path: src/Tracing/HttpClient/TraceableResponseForV5.php - - message: "#^Cannot access offset 'error_types' on mixed\\.$#" + message: "#^Call to an undefined method Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface\\:\\:toStream\\(\\)\\.$#" count: 1 - path: tests/DependencyInjection/SentryExtensionTest.php + path: src/Tracing/HttpClient/TraceableResponseForV6.php - - message: "#^Cannot access offset 'integrations' on mixed\\.$#" + message: "#^Cannot access offset 'sample_rate' on mixed\\.$#" count: 1 - path: tests/DependencyInjection/SentryExtensionTest.php + path: tests/DependencyInjection/ConfigurationTest.php - - message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableHttpClient::request\\(\\) has parameter \\$options with no value type specified in iterable type array.$#" + message: "#^Cannot access offset 'traces_sample_rate' on mixed\\.$#" count: 1 - path: src/Tracing/HttpClient/AbstractTraceableHttpClient.php + path: tests/DependencyInjection/ConfigurationTest.php - - message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableResponse::toArray\\(\\) return type has no value type specified in iterable type array.$#" + message: "#^Argument of an invalid type mixed supplied for foreach, only iterables are supported\\.$#" count: 1 - path: src/Tracing/HttpClient/AbstractTraceableResponse.php + path: tests/DependencyInjection/SentryExtensionTest.php - - message: "#^Method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\TraceableHttpClientForV6::withOptions\\(\\) has parameter \\$options with no value type specified in iterable type array.$#" + message: "#^Cannot access offset 'dsn' on mixed\\.$#" count: 1 - path: src/Tracing/HttpClient/TraceableHttpClientForV6.php + path: tests/DependencyInjection/SentryExtensionTest.php - - message: "#^Parameter \\#1 $responses of method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableHttpClient::stream\\(\\) expects iterable<\\(int\\|string\\), Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface>|Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface, stdClass given\\.$#" + message: "#^Cannot access offset 'error_types' on mixed\\.$#" count: 1 - path: tests/Tracing/HttpClient/TraceableHttpClientTest.php + path: tests/DependencyInjection/SentryExtensionTest.php - - message: "#^Parameter \\#2 \\$responses of static method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableResponse::stream\\(\\) expects iterable, array given\\.$#" + message: "#^Cannot access offset 'integrations' on mixed\\.$#" count: 1 - path: tests/Tracing/HttpClient/TraceableResponseTest.php + path: tests/DependencyInjection/SentryExtensionTest.php - message: "#^Class Symfony\\\\Component\\\\Debug\\\\Exception\\\\FatalErrorException not found\\.$#" @@ -441,11 +441,11 @@ parameters: path: tests/Tracing/Doctrine/DBAL/TracingStatementForV2Test.php - - message: "#^Call to an undefined method Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface::toStream\\(\\)\\.$#" + message: "#^Parameter \\#1 \\$responses of method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableHttpClient\\:\\:stream\\(\\) expects iterable\\<\\(int\\|string\\), Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface\\>\\|Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface, stdClass given\\.$#" count: 1 - path: src/Tracing/HttpClient/TraceableResponseForV5.php + path: tests/Tracing/HttpClient/TraceableHttpClientTest.php - - message: "#^Call to an undefined method Symfony\\\\Contracts\\\\HttpClient\\\\ResponseInterface::toStream\\(\\)\\.$#" + message: "#^Parameter \\#2 \\$responses of static method Sentry\\\\SentryBundle\\\\Tracing\\\\HttpClient\\\\AbstractTraceableResponse\\:\\:stream\\(\\) expects iterable\\, array\\ given\\.$#" count: 1 - path: src/Tracing/HttpClient/TraceableResponseForV6.php + path: tests/Tracing/HttpClient/TraceableResponseTest.php diff --git a/src/DependencyInjection/Compiler/HttpClientTracingPass.php b/src/DependencyInjection/Compiler/HttpClientTracingPass.php index 1ebcdc4f..2464532b 100644 --- a/src/DependencyInjection/Compiler/HttpClientTracingPass.php +++ b/src/DependencyInjection/Compiler/HttpClientTracingPass.php @@ -17,21 +17,16 @@ final class HttpClientTracingPass implements CompilerPassInterface */ public function process(ContainerBuilder $container): void { - if ( - !$container->getParameter('sentry.tracing.enabled') - || !$container->getParameter('sentry.tracing.http_client.enabled') - ) { + if (!$container->getParameter('sentry.tracing.enabled') || !$container->getParameter('sentry.tracing.http_client.enabled')) { return; } foreach ($container->findTaggedServiceIds('http_client.client') as $id => $tags) { $container->register('.sentry.traceable.' . $id, TraceableHttpClient::class) - ->setArguments([ - new Reference('.sentry.traceable.' . $id . '.inner'), - new Reference(HubInterface::class), - ]) - ->addTag('kernel.reset', ['method' => 'reset']) - ->setDecoratedService($id); + ->setDecoratedService($id) + ->setArgument(0, new Reference('.sentry.traceable.' . $id . '.inner')) + ->setArgument(1, new Reference(HubInterface::class)) + ->addTag('kernel.reset', ['method' => 'reset']); } } } diff --git a/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php index 4a45ab0e..9f3d12a7 100644 --- a/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php +++ b/tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php @@ -7,10 +7,8 @@ use PHPUnit\Framework\TestCase; use Sentry\SentryBundle\DependencyInjection\Compiler\HttpClientTracingPass; use Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClient; -use Sentry\State\HubAdapter; use Sentry\State\HubInterface; use Symfony\Component\DependencyInjection\ContainerBuilder; -use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag; use Symfony\Component\HttpClient\HttpClient; use Symfony\Contracts\HttpClient\HttpClientInterface; @@ -23,47 +21,61 @@ public static function setUpBeforeClass(): void } } + public function testProcess(): void + { + $container = $this->createContainerBuilder(true, true); + $container->compile(); + + $this->assertSame(TraceableHttpClient::class, $container->findDefinition('http.client')->getClass()); + } + /** - * @param array $params - * - * @dataProvider provideDisableContainerParameters + * @dataProvider processDoesNothingIfConditionsForEnablingTracingAreMissingDataProvider */ - public function testShouldNotDecorateHttpClientServicesIfDisabled(array $params): void + public function testProcessDoesNothingIfConditionsForEnablingTracingAreMissing(bool $isTracingEnabled, bool $isHttpClientTracingEnabled): void { - $container = new ContainerBuilder(new ParameterBag($params)); - $container->register('http.client', HttpClient::class) - ->setPublic(true) - ->addTag('http_client.client'); - - $container->addCompilerPass(new HttpClientTracingPass()); + $container = $this->createContainerBuilder($isTracingEnabled, $isHttpClientTracingEnabled); $container->compile(); - $this->assertEquals(HttpClient::class, $container->getDefinition('http.client')->getClass()); + $this->assertSame(HttpClient::class, $container->getDefinition('http.client')->getClass()); } /** - * @return iterable + * @return \Generator */ - public function provideDisableContainerParameters(): iterable + public function processDoesNothingIfConditionsForEnablingTracingAreMissingDataProvider(): \Generator { - yield [['sentry.tracing.enabled' => true, 'sentry.tracing.http_client.enabled' => false]]; - yield [['sentry.tracing.enabled' => false, 'sentry.tracing.http_client.enabled' => false]]; - yield [['sentry.tracing.enabled' => false, 'sentry.tracing.http_client.enabled' => true]]; + yield [ + true, + false, + ]; + + yield [ + false, + false, + ]; + + yield [ + false, + true, + ]; } - public function testShouldDecorateHttpClients(): void + private function createContainerBuilder(bool $isTracingEnabled, bool $isHttpClientTracingEnabled): ContainerBuilder { - $container = new ContainerBuilder(new ParameterBag(['sentry.tracing.enabled' => true, 'sentry.tracing.http_client.enabled' => true])); - $container->register(HubInterface::class) - ->setFactory([HubAdapter::class, 'getInstance']); + $container = new ContainerBuilder(); + $container->addCompilerPass(new HttpClientTracingPass()); + $container->setParameter('sentry.tracing.enabled', $isTracingEnabled); + $container->setParameter('sentry.tracing.http_client.enabled', $isHttpClientTracingEnabled); + + $container->register(HubInterface::class, HubInterface::class) + ->setPublic(true); + $container->register('http.client', HttpClient::class) ->setPublic(true) ->addTag('http_client.client'); - $container->addCompilerPass(new HttpClientTracingPass()); - $container->compile(); - - $this->assertEquals(TraceableHttpClient::class, $container->findDefinition('http.client')->getClass()); + return $container; } private static function isHttpClientPackageInstalled(): bool