Make prefix/suffix interceptors for phpredis logging customizable

By inversing dependency for prefix/suffix closures and unifying it into something that looks more like an middleware, we are making interceptor customizable. One use case I can think of is building interceptor for APM agents - this way, we don't have to integrate such support inside this bundle, but 3rd party package can be created to facilitate such use cases.
snc · Jan 16, 2022 · 5ed18b0 · 5ed18b0
1 parent 54f1204
commit 5ed18b0
Show file tree

Hide file tree

Showing 7 changed files with 136 additions and 99 deletions.
diff --git a/phpcs.xml.dist b/phpcs.xml.dist
@@ -22,4 +22,9 @@
             <property name="spacing" value="0"/>
         </properties>
     </rule>
+
+    <rule ref="SlevomatCodingStandard.TypeHints.DeclareStrictTypes.DeclareStrictTypesMissing">
+        <!-- Produces "Redis::flushAll(): Argument #1 ($async) must be of type bool, null given". Is fixed in dev version of phpredis -->
+        <exclude-pattern>src/Logger/RedisCallInterceptor.php</exclude-pattern>
+    </rule>
 </ruleset>
diff --git a/src/DependencyInjection/SncRedisExtension.php b/src/DependencyInjection/SncRedisExtension.php
@@ -21,6 +21,7 @@
 use Snc\RedisBundle\DependencyInjection\Configuration\RedisDsn;
 use Snc\RedisBundle\DependencyInjection\Configuration\RedisEnvDsn;
 use Snc\RedisBundle\Factory\PredisParametersFactory;
+use Snc\RedisBundle\Logger\RedisCallInterceptor;
 use Symfony\Component\Config\Definition\ConfigurationInterface;
 use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
 use Symfony\Component\Config\FileLocator;
@@ -52,11 +53,11 @@ public function load(array $configs, ContainerBuilder $container): void
         $phpredisFactoryDefinition = $container->getDefinition('snc_redis.phpredis_factory');
 
         if (!$container->getParameter('kernel.debug')) {
-            $phpredisFactoryDefinition->replaceArgument('$stopwatch', null);
+            $container->getDefinition(RedisCallInterceptor::class)->replaceArgument(1, null);
         }
 
         if (!class_exists(\ProxyManager\Configuration::class)) {
-            $phpredisFactoryDefinition->replaceArgument('$proxyConfiguration', null);
+            $phpredisFactoryDefinition->replaceArgument(1, null);
         }
 
         foreach ($config['class'] as $name => $class) {

diff --git a/src/Factory/PhpredisClientFactory.php b/src/Factory/PhpredisClientFactory.php
@@ -13,28 +13,19 @@
 use ReflectionClass;
 use ReflectionMethod;
 use Snc\RedisBundle\DependencyInjection\Configuration\RedisDsn;
-use Snc\RedisBundle\Logger\RedisLogger;
 use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
-use Symfony\Component\Stopwatch\Stopwatch;
 
 use function array_key_exists;
 use function array_keys;
 use function array_map;
-use function array_values;
 use function count;
 use function defined;
 use function implode;
 use function in_array;
 use function is_a;
 use function is_array;
-use function is_numeric;
-use function is_scalar;
-use function microtime;
 use function spl_autoload_register;
 use function sprintf;
-use function strtoupper;
-use function strval;
-use function trim;
 
 /** @internal */
 class PhpredisClientFactory
@@ -48,14 +39,14 @@ class PhpredisClientFactory
         'getDbNum',
         'getPersistentID',
     ];
-    private RedisLogger $logger;
-    private ?Stopwatch $stopwatch              = null;
-    private ?Configuration $proxyConfiguration = null;
+    private ?Configuration $proxyConfiguration;
+    /** @var callable(object, string, array, ?string): mixed */
+    private $interceptor;
 
-    public function __construct(RedisLogger $logger, ?Configuration $proxyConfiguration = null, ?Stopwatch $stopwatch = null)
+    /** @param callable(object, string, array, ?string): mixed $interceptor */
+    public function __construct(callable $interceptor, ?Configuration $proxyConfiguration = null)
     {
-        $this->logger             = $logger;
-        $this->stopwatch          = $stopwatch;
+        $this->interceptor        = $interceptor;
         $this->proxyConfiguration = $proxyConfiguration;
 
         if (!$this->proxyConfiguration) {
@@ -231,7 +222,6 @@ private function loadSlaveFailoverType(string $type): int
     private function createLoggingProxy(object $client, string $alias): object
     {
         $prefixInterceptors     = [];
-        $suffixInterceptors     = [];
         $classToCopyMethodsFrom = $client instanceof Redis ? Redis::class : RedisCluster::class;
 
         foreach ((new ReflectionClass($classToCopyMethodsFrom))->getMethods(ReflectionMethod::IS_PUBLIC) as $method) {
@@ -245,76 +235,16 @@ private function createLoggingProxy(object $client, string $alias): object
                 AccessInterceptorInterface $proxy,
                 object $instance,
                 string $method,
-                array $args
-            ) use (
-                &$time,
-                &$event
-            ): void {
-                $time = microtime(true);
-
-                if (!$this->stopwatch) {
-                    return;
-                }
-
-                $event = $this->stopwatch->start($this->getCommandString($method, array_values($args)), 'redis');
-            };
-            $suffixInterceptors[$name] = function (
-                AccessInterceptorInterface $proxy,
-                object $instance,
-                string $method,
-                array $args
-            ) use (
-                $alias,
-                &$time,
-                &$event
-            ): void {
-                $this->logger->logCommand($this->getCommandString($method, array_values($args)), microtime(true) - $time, $alias);
-
-                if (!$event) {
-                    return;
-                }
-
-                $event->stop();
+                array $args,
+                bool &$returnEarly
+            ) use ($alias) {
+                $returnEarly = true;
+
+                return ($this->interceptor)($instance, $method, $args, $alias);
             };
         }
 
         return (new AccessInterceptorValueHolderFactory($this->proxyConfiguration))
-            ->createProxy($client, $prefixInterceptors, $suffixInterceptors);
-    }
-
-    /**
-     * Returns a string representation of the given command including arguments.
-     *
-     * @param mixed[] $arguments List of command arguments
-     */
-    private function getCommandString(string $command, array $arguments): string
-    {
-        $list = [];
-        $this->flatten($arguments, $list);
-
-        return trim(strtoupper($command) . ' ' . implode(' ', $list));
-    }
-
-    /**
-     * Flatten arguments to single dimension array.
-     *
-     * @param mixed[] $arguments An array of command arguments
-     * @param mixed[] $list      Holder of results
-     */
-    private function flatten(array $arguments, array &$list): void
-    {
-        foreach ($arguments as $key => $item) {
-            if (!is_numeric($key)) {
-                $list[] = $key;
-            }
-
-            if (is_scalar($item)) {
-                $list[] = strval($item);
-            } elseif ($item === null) {
-                $list[] = '<null>';
-            } else {
-                $this->flatten($item, $list);
-            }
-        }
+            ->createProxy($client, $prefixInterceptors);
     }
 }
diff --git a/src/Logger/RedisCallInterceptor.php b/src/Logger/RedisCallInterceptor.php
@@ -0,0 +1,92 @@
+<?php
+
+namespace Snc\RedisBundle\Logger;
+
+use Symfony\Component\Stopwatch\Stopwatch;
+
+use function array_values;
+use function implode;
+use function is_numeric;
+use function is_scalar;
+use function microtime;
+use function strtoupper;
+use function strval;
+use function trim;
+
+class RedisCallInterceptor
+{
+    private RedisLogger $logger;
+    private ?Stopwatch $stopwatch;
+
+    public function __construct(RedisLogger $logger, ?Stopwatch $stopwatch = null)
+    {
+        $this->logger    = $logger;
+        $this->stopwatch = $stopwatch;
+    }
+
+    /**
+     * @param mixed[] $args
+     *
+     * @return mixed
+     */
+    public function __invoke(
+        object $instance,
+        string $method,
+        array $args,
+        ?string $connection
+    ) {
+        $args    = array_values($args);
+        $command = $this->getCommandString($method, $args);
+        $time    = microtime(true);
+
+        if ($this->stopwatch) {
+            $event = $this->stopwatch->start($command, 'redis');
+        }
+
+        $return = $instance->$method(...$args);
+
+        $this->logger->logCommand($command, microtime(true) - $time, $connection);
+
+        if (isset($event)) {
+            $event->stop();
+        }
+
+        return $return;
+    }
+
+    /**
+     * Returns a string representation of the given command including arguments.
+     *
+     * @param mixed[] $arguments List of command arguments
+     */
+    private function getCommandString(string $command, array $arguments): string
+    {
+        $list = [];
+        $this->flatten($arguments, $list);
+
+        return trim(strtoupper($command) . ' ' . implode(' ', $list));
+    }
+
+    /**
+     * Flatten arguments to single dimension array.
+     *
+     * @param mixed[] $arguments An array of command arguments
+     * @param mixed[] $list      Holder of results
+     */
+    private function flatten(array $arguments, array &$list): void
+    {
+        foreach ($arguments as $key => $item) {
+            if (!is_numeric($key)) {
+                $list[] = $key;
+            }
+
+            if (is_scalar($item)) {
+                $list[] = strval($item);
+            } elseif ($item === null) {
+                $list[] = '<null>';
+            } else {
+                $this->flatten($item, $list);
+            }
+        }
+    }
+}
diff --git a/src/Resources/config/services.php b/src/Resources/config/services.php
@@ -7,6 +7,7 @@
 use ProxyManager\GeneratorStrategy\FileWriterGeneratorStrategy;
 use Snc\RedisBundle\Command\RedisQueryCommand;
 use Snc\RedisBundle\Factory\PhpredisClientFactory;
+use Snc\RedisBundle\Logger\RedisCallInterceptor;
 use Symfony\Component\DependencyInjection\Definition;
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
 use Symfony\Component\DependencyInjection\Loader\Configurator\InlineServiceConfigurator;
@@ -33,10 +34,13 @@
             (new ReferenceConfigurator('var_dumper.cloner'))->nullOnInvalid(),
         ]);
 
+    $container->set(RedisCallInterceptor::class)
+        ->class(RedisCallInterceptor::class)
+        ->args([new ReferenceConfigurator('snc_redis.logger'), new ReferenceConfigurator('debug.stopwatch')]);
+
     $container->set('snc_redis.phpredis_factory', PhpredisClientFactory::class)
-        ->arg('$logger', new ReferenceConfigurator('snc_redis.logger'))
-        ->arg(
-            '$proxyConfiguration',
+        ->args([
+            new ReferenceConfigurator(RedisCallInterceptor::class),
             new InlineServiceConfigurator(
                 (new Definition(Configuration::class))
                     ->addMethodCall('setGeneratorStrategy', [
@@ -46,7 +50,6 @@
                         ),
                     ])
                     ->addMethodCall('setProxiesTargetDir', ['%kernel.cache_dir%'])
-            )
-        )
-        ->arg('$stopwatch', (new ReferenceConfigurator('debug.stopwatch'))->nullOnInvalid());
+            ),
+        ]);
 };
diff --git a/tests/Factory/PhpredisClientFactoryTest.php b/tests/Factory/PhpredisClientFactoryTest.php
@@ -10,6 +10,7 @@
 use Redis;
 use RedisCluster;
 use Snc\RedisBundle\Factory\PhpredisClientFactory;
+use Snc\RedisBundle\Logger\RedisCallInterceptor;
 use Snc\RedisBundle\Logger\RedisLogger;
 use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
 
@@ -37,7 +38,7 @@ protected function setUp(): void
     public function testCreateMinimalConfig(): void
     {
         $this->logger->expects($this->never())->method('debug');
-        $factory = new PhpredisClientFactory($this->redisLogger);
+        $factory = new PhpredisClientFactory(new RedisCallInterceptor($this->redisLogger));
 
         $client = $factory->create(Redis::class, ['redis://localhost:6379'], ['connection_timeout' => 5], 'default', false);
 
@@ -52,7 +53,7 @@ public function testCreateMinimalConfig(): void
     public function testUnixDsnConfig(): void
     {
         $this->expectExceptionMessage('php_network_getaddresses');
-        (new PhpredisClientFactory($this->redisLogger))
+        (new PhpredisClientFactory(new RedisCallInterceptor($this->redisLogger)))
             ->create(
                 Redis::class,
                 ['redis:///run/redis/redis.sock'],
@@ -65,7 +66,7 @@ public function testUnixDsnConfig(): void
     public function testCreateMinimalClusterConfig(): void
     {
         $this->logger->expects($this->never())->method('debug');
-        $factory = new PhpredisClientFactory($this->redisLogger);
+        $factory = new PhpredisClientFactory(new RedisCallInterceptor($this->redisLogger));
 
         $client = $factory->create(
             RedisCluster::class,
@@ -85,7 +86,7 @@ public function testCreateMinimalClusterConfig(): void
 
     public function testCreateFullConfig(): void
     {
-        $factory = new PhpredisClientFactory($this->redisLogger);
+        $factory = new PhpredisClientFactory(new RedisCallInterceptor($this->redisLogger));
 
         $client = $factory->create(
             Redis::class,
@@ -123,7 +124,7 @@ public function testDsnConfig(): void
             ['Executing command "SELECT 2"']
         );
 
-        $factory = new PhpredisClientFactory($this->redisLogger);
+        $factory = new PhpredisClientFactory(new RedisCallInterceptor($this->redisLogger));
 
         $client = $factory->create(
             Redis::class,
@@ -147,7 +148,7 @@ public function testDsnConfig(): void
 
     public function testNestedDsnConfig(): void
     {
-        $factory = new PhpredisClientFactory($this->redisLogger);
+        $factory = new PhpredisClientFactory(new RedisCallInterceptor($this->redisLogger));
 
         $client = $factory->create(
             Redis::class,
@@ -172,7 +173,7 @@ public function testNestedDsnConfig(): void
     /** @dataProvider serializationTypes */
     public function testLoadSerializationType(string $serializationType, int $serializer): void
     {
-        $factory = new PhpredisClientFactory($this->redisLogger);
+        $factory = new PhpredisClientFactory(new RedisCallInterceptor($this->redisLogger));
 
         $client = $factory->create(
             Redis::class,
@@ -190,7 +191,7 @@ public function testLoadSerializationType(string $serializationType, int $serial
 
     public function testLoadSerializationTypeFail(): void
     {
-        $factory = new PhpredisClientFactory($this->redisLogger);
+        $factory = new PhpredisClientFactory(new RedisCallInterceptor($this->redisLogger));
         $this->expectException(InvalidConfigurationException::class);
 
         $factory->create(

diff --git a/tests/Functional/IntegrationTest.php b/tests/Functional/IntegrationTest.php
@@ -28,6 +28,11 @@ class IntegrationTest extends WebTestCase
 {
     private ?KernelBrowser $client = null;
 
+    /**
+     * Muted deprecation "Passing null to parameter #1 ($async) of type bool is deprecated" - would be fixed by next phpredis release with fixed reflection on its own
+     *
+     * @group legacy
+     */
     protected function setUp(): void
     {
         $fs = new Filesystem();