From 59b40c0576a814deac22e75cd62856011c2bb560 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Mon, 29 Mar 2021 21:09:17 +0200 Subject: [PATCH 1/3] Add distributed tracing support for the Symfony Cache component --- .github/workflows/tests.yaml | 2 +- CHANGELOG.md | 2 +- composer.json | 1 + psalm-baseline.xml | 11 +- .../Compiler/CacheTracingPass.php | 43 ++++ src/DependencyInjection/Configuration.php | 4 + src/DependencyInjection/SentryExtension.php | 17 ++ src/Resources/config/schema/sentry-1.0.xsd | 5 + src/Resources/config/services.xml | 10 + src/SentryBundle.php | 2 + src/Tracing/Cache/TraceableCacheAdapter.php | 33 +++ .../Cache/TraceableCacheAdapterTrait.php | 207 ++++++++++++++++++ .../Cache/TraceableTagAwareCacheAdapter.php | 43 ++++ .../Compiler/CacheTracingPassTest.php | 85 +++++++ .../DependencyInjection/ConfigurationTest.php | 4 + 15 files changed, 465 insertions(+), 4 deletions(-) create mode 100644 src/DependencyInjection/Compiler/CacheTracingPass.php create mode 100644 src/Tracing/Cache/TraceableCacheAdapter.php create mode 100644 src/Tracing/Cache/TraceableCacheAdapterTrait.php create mode 100644 src/Tracing/Cache/TraceableTagAwareCacheAdapter.php create mode 100644 tests/DependencyInjection/Compiler/CacheTracingPassTest.php diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index ec537aa8..f63dca9a 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -108,7 +108,7 @@ jobs: restore-keys: ${{ runner.os }}-${{ matrix.php }}-composer-${{ matrix.dependencies }}- - name: Remove optional packages - run: composer remove doctrine/dbal doctrine/doctrine-bundle symfony/messenger symfony/twig-bundle --dev --no-update + run: composer remove doctrine/dbal doctrine/doctrine-bundle symfony/messenger symfony/twig-bundle symfony/cache --dev --no-update - name: Install highest dependencies run: composer update --no-progress --no-interaction --prefer-dist diff --git a/CHANGELOG.md b/CHANGELOG.md index eebd23cb..f3a400ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ - Add support for distributed tracing of Twig template rendering (#430) - Add support for distributed tracing of SQL queries while using Doctrine DBAL (#426) - Add support for distributed tracing when running a console command (#455) -- Added missing `capture-soft-fails` config schema option (#417) +- Add support for distributed tracing of cache pools (#) - Deprecate the `Sentry\SentryBundle\EventListener\ConsoleCommandListener` class in favor of its parent class `Sentry\SentryBundle\EventListener\ConsoleListener` (#429) - Lower the required version of `symfony/psr-http-message-bridge` to allow installing it on a project that uses Symfony `3.4.x` components only (#480) diff --git a/composer.json b/composer.json index ca862c28..2595acad 100644 --- a/composer.json +++ b/composer.json @@ -45,6 +45,7 @@ "phpstan/phpstan-phpunit": "^0.12", "phpunit/phpunit": "^8.5||^9.0", "symfony/browser-kit": "^3.4.44||^4.4.20||^5.0.11", + "symfony/cache": "^3.4.44||^4.4.20||^5.0.11", "symfony/dom-crawler": "^3.4.44||^4.4.20||^5.0.11", "symfony/framework-bundle": "^3.4.44||^4.4.20||^5.0.11", "symfony/messenger": "^4.4.20||^5.0.11", diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 51383e36..f72cae2a 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,9 +1,16 @@ - + ConsoleListener - + + public function __construct(HubInterface $hub, bool $captureErrors = true) + + + + + getItems + diff --git a/src/DependencyInjection/Compiler/CacheTracingPass.php b/src/DependencyInjection/Compiler/CacheTracingPass.php new file mode 100644 index 00000000..4454b2f1 --- /dev/null +++ b/src/DependencyInjection/Compiler/CacheTracingPass.php @@ -0,0 +1,43 @@ +getParameter('sentry.tracing.cache.enabled')) { + return; + } + + foreach ($container->findTaggedServiceIds('cache.pool') as $serviceId => $tags) { + $cachePoolDefinition = $container->getDefinition($serviceId); + + if ($cachePoolDefinition->isAbstract()) { + continue; + } + + if (is_subclass_of($cachePoolDefinition->getClass(), TagAwareAdapterInterface::class)) { + $traceableCachePoolDefinition = new ChildDefinition('sentry.tracing.traceable_tag_aware_cache_adapter'); + } else { + $traceableCachePoolDefinition = new ChildDefinition('sentry.tracing.traceable_cache_adapter'); + } + + $traceableCachePoolDefinition->setDecoratedService($serviceId); + $traceableCachePoolDefinition->replaceArgument(1, new Reference($serviceId . '.traceable.inner')); + + $container->setDefinition($serviceId . '.traceable', $traceableCachePoolDefinition); + } + } +} diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 25c09a17..d12c695b 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -13,6 +13,7 @@ use Symfony\Component\Config\Definition\Builder\TreeBuilder; use Symfony\Component\Config\Definition\ConfigurationInterface; use Symfony\Component\Messenger\MessageBusInterface; +use Symfony\Contracts\Cache\CacheInterface; final class Configuration implements ConfigurationInterface { @@ -163,6 +164,9 @@ private function addDistributedTracingSection(ArrayNodeDefinition $rootNode): vo ->arrayNode('twig') ->{class_exists(TwigBundle::class) ? 'canBeDisabled' : 'canBeEnabled'}() ->end() + ->arrayNode('cache') + ->{interface_exists(CacheInterface::class) ? 'canBeDisabled' : 'canBeEnabled'}() + ->end() ->end() ->end() ->end(); diff --git a/src/DependencyInjection/SentryExtension.php b/src/DependencyInjection/SentryExtension.php index eb40ac28..c800653d 100644 --- a/src/DependencyInjection/SentryExtension.php +++ b/src/DependencyInjection/SentryExtension.php @@ -35,6 +35,7 @@ use Symfony\Component\DependencyInjection\Reference; use Symfony\Component\ErrorHandler\Error\FatalError; use Symfony\Component\HttpKernel\DependencyInjection\ConfigurableExtension; +use Symfony\Contracts\Cache\CacheInterface; final class SentryExtension extends ConfigurableExtension { @@ -68,6 +69,7 @@ protected function loadInternal(array $mergedConfig, ContainerBuilder $container $this->registerTracingConfiguration($container, $mergedConfig['tracing']); $this->registerDbalTracingConfiguration($container, $mergedConfig['tracing']); $this->registerTwigTracingConfiguration($container, $mergedConfig['tracing']); + $this->registerCacheTracingConfiguration($container, $mergedConfig['tracing']); } /** @@ -214,6 +216,21 @@ private function registerTwigTracingConfiguration(ContainerBuilder $container, a } } + /** + * @param array $config + */ + private function registerCacheTracingConfiguration(ContainerBuilder $container, array $config): void + { + $isConfigEnabled = $this->isConfigEnabled($container, $config) + && $this->isConfigEnabled($container, $config['cache']); + + if ($isConfigEnabled && !interface_exists(CacheInterface::class)) { + throw new LogicException('Cache tracing support cannot be enabled because the symfony/cache Composer package is not installed.'); + } + + $container->setParameter('sentry.tracing.cache.enabled', $isConfigEnabled); + } + /** * @param string[] $integrations * @param array $config diff --git a/src/Resources/config/schema/sentry-1.0.xsd b/src/Resources/config/schema/sentry-1.0.xsd index 1a256e50..cfaf0286 100644 --- a/src/Resources/config/schema/sentry-1.0.xsd +++ b/src/Resources/config/schema/sentry-1.0.xsd @@ -85,6 +85,7 @@ + @@ -101,4 +102,8 @@ + + + + diff --git a/src/Resources/config/services.xml b/src/Resources/config/services.xml index 9c536b82..c5fe4c33 100644 --- a/src/Resources/config/services.xml +++ b/src/Resources/config/services.xml @@ -102,6 +102,16 @@ + + + + + + + + + + diff --git a/src/SentryBundle.php b/src/SentryBundle.php index 7dc981f6..4098f8b3 100644 --- a/src/SentryBundle.php +++ b/src/SentryBundle.php @@ -4,6 +4,7 @@ namespace Sentry\SentryBundle; +use Sentry\SentryBundle\DependencyInjection\Compiler\CacheTracingPass; use Sentry\SentryBundle\DependencyInjection\Compiler\DbalTracingPass; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\HttpKernel\Bundle\Bundle; @@ -17,5 +18,6 @@ public function build(ContainerBuilder $container): void parent::build($container); $container->addCompilerPass(new DbalTracingPass()); + $container->addCompilerPass(new CacheTracingPass()); } } diff --git a/src/Tracing/Cache/TraceableCacheAdapter.php b/src/Tracing/Cache/TraceableCacheAdapter.php new file mode 100644 index 00000000..a8de4fe9 --- /dev/null +++ b/src/Tracing/Cache/TraceableCacheAdapter.php @@ -0,0 +1,33 @@ + + */ + use TraceableCacheAdapterTrait; + + /** + * @param HubInterface $hub The current hub + * @param AdapterInterface $decoratedAdapter The decorated cache adapter + */ + public function __construct(HubInterface $hub, AdapterInterface $decoratedAdapter) + { + $this->hub = $hub; + $this->decoratedAdapter = $decoratedAdapter; + } +} diff --git a/src/Tracing/Cache/TraceableCacheAdapterTrait.php b/src/Tracing/Cache/TraceableCacheAdapterTrait.php new file mode 100644 index 00000000..f96a9506 --- /dev/null +++ b/src/Tracing/Cache/TraceableCacheAdapterTrait.php @@ -0,0 +1,207 @@ +traceFunction('cache.get_item', function () use ($key) { + return $this->decoratedAdapter->getItem($key); + }); + } + + /** + * {@inheritdoc} + */ + public function getItems(array $keys = []) + { + return $this->traceFunction('cache.get_items', function () use ($keys) { + return $this->decoratedAdapter->getItems($keys); + }); + } + + /** + * {@inheritdoc} + */ + public function clear(string $prefix = ''): bool + { + return $this->traceFunction('cache.clear', function () use ($prefix): bool { + return $this->decoratedAdapter->clear($prefix); + }); + } + + /** + * {@inheritdoc} + * + * @param mixed[] $metadata + */ + public function get(string $key, callable $callback, float $beta = null, array &$metadata = null) + { + return $this->traceFunction('cache.get_item', function () use ($key, $callback, $beta, &$metadata) { + if (!$this->decoratedAdapter instanceof CacheInterface) { + throw new \BadMethodCallException(sprintf('The %s() method is not supported on an adapter that does not implement the "%s" interface.', __FUNCTION__, CacheInterface::class)); + } + + return $this->decoratedAdapter->get($key, $callback, $beta, $metadata); + }); + } + + /** + * {@inheritdoc} + */ + public function delete(string $key): bool + { + return $this->traceFunction('cache.delete_item', function () use ($key) { + if (!$this->decoratedAdapter instanceof CacheInterface) { + throw new \BadMethodCallException(sprintf('The %s() method is not supported on an adapter that does not implement the "%s" interface.', __FUNCTION__, CacheInterface::class)); + } + + return $this->decoratedAdapter->delete($key); + }); + } + + /** + * {@inheritdoc} + */ + public function hasItem($key): bool + { + return $this->traceFunction('cache.has_item', function () use ($key): bool { + return $this->decoratedAdapter->hasItem($key); + }); + } + + /** + * {@inheritdoc} + */ + public function deleteItem($key): bool + { + return $this->traceFunction('cache.delete_item', function () use ($key): bool { + return $this->decoratedAdapter->deleteItem($key); + }); + } + + /** + * {@inheritdoc} + */ + public function deleteItems(array $keys): bool + { + return $this->traceFunction('cache.delete_items', function () use ($keys): bool { + return $this->decoratedAdapter->deleteItems($keys); + }); + } + + /** + * {@inheritdoc} + */ + public function save(CacheItemInterface $item): bool + { + return $this->traceFunction('cache.save', function () use ($item): bool { + return $this->decoratedAdapter->save($item); + }); + } + + /** + * {@inheritdoc} + */ + public function saveDeferred(CacheItemInterface $item): bool + { + return $this->traceFunction('cache.save_deferred', function () use ($item): bool { + return $this->decoratedAdapter->saveDeferred($item); + }); + } + + /** + * {@inheritdoc} + */ + public function commit(): bool + { + return $this->traceFunction('cache.commit', function (): bool { + return $this->decoratedAdapter->commit(); + }); + } + + /** + * {@inheritdoc} + */ + public function prune(): bool + { + return $this->traceFunction('cache.prune', function (): bool { + if (!$this->decoratedAdapter instanceof PruneableInterface) { + throw new \BadMethodCallException(sprintf('The %s() method is not supported on an adapter that does not implement the "%s" interface.', __FUNCTION__, PruneableInterface::class)); + } + + return $this->decoratedAdapter->prune(); + }); + } + + /** + * {@inheritdoc} + */ + public function reset(): void + { + if (!$this->decoratedAdapter instanceof ResettableInterface) { + throw new \BadMethodCallException(sprintf('The %s() method is not supported on an adapter that does not implement the "%s" interface.', __FUNCTION__, ResettableInterface::class)); + } + + $this->decoratedAdapter->reset(); + } + + /** + * @phpstan-template TResult + * + * @phpstan-param \Closure(): TResult $callback + * + * @phpstan-return TResult + */ + private function traceFunction(string $spanOperation, \Closure $callback) + { + $span = $this->hub->getSpan(); + + 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/Cache/TraceableTagAwareCacheAdapter.php b/src/Tracing/Cache/TraceableTagAwareCacheAdapter.php new file mode 100644 index 00000000..7375eb5d --- /dev/null +++ b/src/Tracing/Cache/TraceableTagAwareCacheAdapter.php @@ -0,0 +1,43 @@ + + */ + use TraceableCacheAdapterTrait; + + /** + * @param HubInterface $hub The current hub + * @param TagAwareAdapterInterface $decoratedAdapter The decorated cache adapter + */ + public function __construct(HubInterface $hub, TagAwareAdapterInterface $decoratedAdapter) + { + $this->hub = $hub; + $this->decoratedAdapter = $decoratedAdapter; + } + + /** + * {@inheritdoc} + */ + public function invalidateTags(array $tags): bool + { + return $this->traceFunction('cache.invalidate_tags', function () use ($tags): bool { + return $this->decoratedAdapter->invalidateTags($tags); + }); + } +} diff --git a/tests/DependencyInjection/Compiler/CacheTracingPassTest.php b/tests/DependencyInjection/Compiler/CacheTracingPassTest.php new file mode 100644 index 00000000..575c12f9 --- /dev/null +++ b/tests/DependencyInjection/Compiler/CacheTracingPassTest.php @@ -0,0 +1,85 @@ +createMock(AdapterInterface::class); + $tagAwareCacheAdapter = $this->createMock(TagAwareAdapterInterface::class); + $container = $this->createContainerBuilder(true); + + $container->register('app.cache.foo', \get_class($tagAwareCacheAdapter)) + ->setPublic(true) + ->addTag('cache.pool'); + + $container->register('app.cache.bar', \get_class($cacheAdapter)) + ->setPublic(true) + ->addTag('cache.pool'); + + $container->register('app.cache.baz', CacheInterface::class) + ->setPublic(true) + ->setAbstract(true) + ->addTag('cache.pool'); + + $container->compile(); + + $cacheTraceableDefinition = $container->findDefinition('app.cache.foo'); + + $this->assertSame(TraceableTagAwareCacheAdapter::class, $cacheTraceableDefinition->getClass()); + $this->assertInstanceOf(Definition::class, $cacheTraceableDefinition->getArgument(1)); + $this->assertSame(\get_class($tagAwareCacheAdapter), $cacheTraceableDefinition->getArgument(1)->getClass()); + + $cacheTraceableDefinition = $container->findDefinition('app.cache.bar'); + + $this->assertSame(TraceableCacheAdapter::class, $cacheTraceableDefinition->getClass()); + $this->assertInstanceOf(Definition::class, $cacheTraceableDefinition->getArgument(1)); + $this->assertSame(\get_class($cacheAdapter), $cacheTraceableDefinition->getArgument(1)->getClass()); + + $this->assertFalse($container->hasDefinition('app.cache.baz')); + } + + public function testProcessDoesNothingIfConditionsForEnablingTracingAreMissing(): void + { + $container = $this->createContainerBuilder(false); + $container->register('app.cache', CacheInterface::class); + $container->compile(); + + $this->assertFalse($container->hasDefinition('app.cache.traceable.inner')); + } + + private function createContainerBuilder(bool $isTracingActive): ContainerBuilder + { + $container = new ContainerBuilder(); + $container->addCompilerPass(new CacheTracingPass()); + $container->setParameter('sentry.tracing.cache.enabled', $isTracingActive); + + $container->register(HubInterface::class, HubInterface::class); + $container->register('sentry.tracing.traceable_cache_adapter', TraceableCacheAdapter::class) + ->setAbstract(true) + ->setArgument(0, new Reference(HubInterface::class)) + ->setArgument(1, null); + + $container->register('sentry.tracing.traceable_tag_aware_cache_adapter', TraceableTagAwareCacheAdapter::class) + ->setAbstract(true) + ->setArgument(0, new Reference(HubInterface::class)) + ->setArgument(1, null); + + return $container; + } +} diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 39458c52..655c0ad8 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -12,6 +12,7 @@ use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException; use Symfony\Component\Config\Definition\Processor; use Symfony\Component\Messenger\MessageBusInterface; +use Symfony\Contracts\Cache\CacheInterface; final class ConfigurationTest extends TestCase { @@ -46,6 +47,9 @@ public function testProcessConfigurationWithDefaultConfiguration(): void 'twig' => [ 'enabled' => class_exists(TwigBundle::class), ], + 'cache' => [ + 'enabled' => interface_exists(CacheInterface::class), + ], ], ]; From 8627eeb79f4185aba2009bf81925143e10144456 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Tue, 6 Apr 2021 21:56:03 +0200 Subject: [PATCH 2/3] Fix CR issues --- tests/DependencyInjection/Fixtures/php/full.php | 3 +++ tests/DependencyInjection/Fixtures/xml/full.xml | 1 + tests/DependencyInjection/Fixtures/yml/full.yml | 2 ++ 3 files changed, 6 insertions(+) diff --git a/tests/DependencyInjection/Fixtures/php/full.php b/tests/DependencyInjection/Fixtures/php/full.php index e2255ded..45d4ea69 100644 --- a/tests/DependencyInjection/Fixtures/php/full.php +++ b/tests/DependencyInjection/Fixtures/php/full.php @@ -50,5 +50,8 @@ 'twig' => [ 'enabled' => false, ], + 'cache' => [ + 'enabled' => false, + ], ], ]); diff --git a/tests/DependencyInjection/Fixtures/xml/full.xml b/tests/DependencyInjection/Fixtures/xml/full.xml index cc4197f5..3cfbfe11 100644 --- a/tests/DependencyInjection/Fixtures/xml/full.xml +++ b/tests/DependencyInjection/Fixtures/xml/full.xml @@ -42,6 +42,7 @@ default + diff --git a/tests/DependencyInjection/Fixtures/yml/full.yml b/tests/DependencyInjection/Fixtures/yml/full.yml index e413964b..543e5789 100644 --- a/tests/DependencyInjection/Fixtures/yml/full.yml +++ b/tests/DependencyInjection/Fixtures/yml/full.yml @@ -44,3 +44,5 @@ sentry: - default twig: enabled: false + cache: + enabled: false From a0e3e78d3f12842d867ca526fba9f6ddf9e91cc0 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Sat, 10 Apr 2021 16:53:10 +0200 Subject: [PATCH 3/3] Add suggestion to install the symfony/cache component to enable its tracing --- composer.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 2595acad..97619aff 100644 --- a/composer.json +++ b/composer.json @@ -58,7 +58,8 @@ "suggest": { "monolog/monolog": "Allow sending log messages to Sentry by using the included Monolog handler.", "doctrine/doctrine-bundle": "Allow distributed tracing of database queries using Sentry.", - "symfony/twig-bundle": "Allow distributed tracing of Twig template rendering using Sentry." + "symfony/twig-bundle": "Allow distributed tracing of Twig template rendering using Sentry.", + "symfony/cache": "Allow distributed tracing of cache pools using Sentry." }, "autoload": { "files": [