From 182ddb0cc8c816a7db83acb41669e152b782067f Mon Sep 17 00:00:00 2001 From: grzdasz Date: Sun, 27 Oct 2024 10:53:20 +0100 Subject: [PATCH 1/3] Fix broken Serializer when using Laravel Illuminate\Queue\LuaScripts (#309) Since introduction of RedisCommandWatcher, auto-instrumentation for Laravel fails when using Lua scripts for Queue management, since command argument array can contains arrays. --- .../Laravel/src/Watchers/RedisCommand/Serializer.php | 3 +++ .../Laravel/tests/Unit/Watches/RedisCommand/SerializerTest.php | 3 +++ 2 files changed, 6 insertions(+) diff --git a/src/Instrumentation/Laravel/src/Watchers/RedisCommand/Serializer.php b/src/Instrumentation/Laravel/src/Watchers/RedisCommand/Serializer.php index 9e76f7ce..a64c4081 100644 --- a/src/Instrumentation/Laravel/src/Watchers/RedisCommand/Serializer.php +++ b/src/Instrumentation/Laravel/src/Watchers/RedisCommand/Serializer.php @@ -69,6 +69,9 @@ public static function serializeCommand(string $command, array $params): string $paramsToSerialize[] = '[' . (count($params) - $paramsToSerializeNum) . ' other arguments]'; } + // In some cases (for example when using LUA scripts) arrays are valid parameters + $paramsToSerialize = array_map(function($param) { return is_array($param) ? json_encode($param) : $param; }, $paramsToSerialize); + return $command . ' ' . implode(' ', $paramsToSerialize); } } diff --git a/src/Instrumentation/Laravel/tests/Unit/Watches/RedisCommand/SerializerTest.php b/src/Instrumentation/Laravel/tests/Unit/Watches/RedisCommand/SerializerTest.php index 69c831f3..3749f65e 100644 --- a/src/Instrumentation/Laravel/tests/Unit/Watches/RedisCommand/SerializerTest.php +++ b/src/Instrumentation/Laravel/tests/Unit/Watches/RedisCommand/SerializerTest.php @@ -31,5 +31,8 @@ public function serializeCases(): iterable // Serialize all params yield ['DEL', ['param1', 'param2', 'param3', 'param4'], 'DEL param1 param2 param3 param4']; + + // Parameters of array type + yield ['EVAL', ['param1', 'param2', ['arg1', 'arg2']], 'EVAL param1 param2 ["arg1","arg2"]']; } } From 73e81d865033ac5de3adaec4d450acfb063e18e5 Mon Sep 17 00:00:00 2001 From: Chris Lightfoot-Wild Date: Sun, 27 Oct 2024 21:22:57 +0000 Subject: [PATCH 2/3] QA workflow jobs sorted alphabetically. (#312) --- .github/workflows/php.yml | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml index aaa66b31..ee1d2ee4 100644 --- a/.github/workflows/php.yml +++ b/.github/workflows/php.yml @@ -1,4 +1,3 @@ - name: PHP QA on: @@ -19,28 +18,30 @@ jobs: fail-fast: false matrix: php-version: ['7.4', '8.0', '8.1', '8.2', '8.3'] + # Sorted alphabetically to ease finding the desired run in the GitHub Workflow UI. project: [ 'Aws', 'Context/Swoole', + 'Instrumentation/CakePHP', + 'Instrumentation/CodeIgniter', 'Instrumentation/ExtAmqp', 'Instrumentation/ExtRdKafka', 'Instrumentation/Guzzle', 'Instrumentation/HttpAsyncClient', - 'Instrumentation/Slim', - 'Instrumentation/CakePHP', + 'Instrumentation/IO', + 'Instrumentation/Laravel', + 'Instrumentation/MongoDB', + 'Instrumentation/OpenAIPHP', + 'Instrumentation/PDO', + # Sort PSRs numerically. 'Instrumentation/Psr3', 'Instrumentation/Psr6', 'Instrumentation/Psr14', 'Instrumentation/Psr15', 'Instrumentation/Psr16', 'Instrumentation/Psr18', - 'Instrumentation/IO', - 'Instrumentation/PDO', + 'Instrumentation/Slim', 'Instrumentation/Symfony', - 'Instrumentation/OpenAIPHP', - 'Instrumentation/Laravel', - 'Instrumentation/MongoDB', - 'Instrumentation/CodeIgniter', 'Instrumentation/Yii', 'Logs/Monolog', 'Propagation/ServerTiming', @@ -49,7 +50,7 @@ jobs: 'ResourceDetectors/Container', 'Sampler/RuleBased', 'Shims/OpenTracing', - 'Symfony' + 'Symfony', ] exclude: - project: 'Instrumentation/Guzzle' From 0a412162fa9b78125ae1270aef58dbbbb3228588 Mon Sep 17 00:00:00 2001 From: Chris Lightfoot-Wild Date: Sun, 27 Oct 2024 21:23:49 +0000 Subject: [PATCH 3/3] Laravel: catch any TypeError thrown within LogWatcher when attempting to filter log levels. (#311) --- .../Laravel/src/Watchers/LogWatcher.php | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/Instrumentation/Laravel/src/Watchers/LogWatcher.php b/src/Instrumentation/Laravel/src/Watchers/LogWatcher.php index 7bacfa70..7adcddd9 100644 --- a/src/Instrumentation/Laravel/src/Watchers/LogWatcher.php +++ b/src/Instrumentation/Laravel/src/Watchers/LogWatcher.php @@ -10,6 +10,7 @@ use OpenTelemetry\API\Instrumentation\CachedInstrumentation; use OpenTelemetry\API\Logs\LogRecord; use OpenTelemetry\API\Logs\Map\Psr3; +use TypeError; class LogWatcher extends Watcher { @@ -36,9 +37,17 @@ public function recordLog(MessageLogged $log): void { $underlyingLogger = $this->logger->getLogger(); - /** @phan-suppress-next-line PhanUndeclaredMethod */ - if (method_exists($underlyingLogger, 'isHandling') && !$underlyingLogger->isHandling($log->level)) { - return; + /** + * This assumes that the underlying logger (expected to be monolog) would accept `$log->level` as a string. + * With monolog < 3.x, this method would fail. Let's prevent this blowing up in Laravel<10.x. + */ + try { + /** @phan-suppress-next-line PhanUndeclaredMethod */ + if (method_exists($underlyingLogger, 'isHandling') && !$underlyingLogger->isHandling($log->level)) { + return; + } + } catch (TypeError) { + // Should this fail, we should continue to emit the LogRecord. } $attributes = [