diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml index 8673a2b46..f5a4ceaa2 100644 --- a/.github/workflows/php.yml +++ b/.github/workflows/php.yml @@ -61,8 +61,6 @@ jobs: - project: 'Instrumentation/Curl' php-version: 8.1 - - project: 'Instrumentation/Doctrine' - php-version: 8.1 - project: 'Instrumentation/ExtAmqp' php-version: 8.1 - project: 'Instrumentation/ExtRdKafka' diff --git a/src/Instrumentation/Doctrine/composer.json b/src/Instrumentation/Doctrine/composer.json index f32f2568e..a69882e75 100644 --- a/src/Instrumentation/Doctrine/composer.json +++ b/src/Instrumentation/Doctrine/composer.json @@ -16,7 +16,7 @@ "minimum-stability": "dev", "prefer-stable": true, "require": { - "php": "^8.2", + "php": "^8.1", "ext-opentelemetry": "*", "doctrine/dbal": "^3 || ^4 || ^5", "open-telemetry/api": "^1.0", diff --git a/src/Instrumentation/Doctrine/src/AttributesResolver.php b/src/Instrumentation/Doctrine/src/AttributesResolver.php index de7a6e261..7e9ea152e 100644 --- a/src/Instrumentation/Doctrine/src/AttributesResolver.php +++ b/src/Instrumentation/Doctrine/src/AttributesResolver.php @@ -55,9 +55,9 @@ public static function get(string $attributeName, array $params): string|int|nul /** * Resolve attribute `server.address` */ - private static function getServerAddress(array $params): string + private static function getServerAddress(array $params): ?string { - return $params[1][0]['host'] ?? 'unknown'; + return $params[1][0]['host'] ?? null; } /** @@ -107,7 +107,7 @@ private static function getDbCollectionName(array $params): string /** * Resolve attribute `db.query.text` - * No sanitization is implemented because implicitly the query is expected to be expressed as a preparated statement + * No sanitization is implemented because implicitly the query is expected to be expressed as a prepared statement * which happen automatically in Doctrine if parameters are bound to the query. */ private static function getDbQueryText(array $params): string @@ -115,9 +115,34 @@ private static function getDbQueryText(array $params): string return $params[1][0] ?? 'undefined'; } - private static function getDbNamespace(array $params): string + private static function getDbNamespace(array $params): ?string { - return $params[1][0]['dbname'] ?? 'unknown'; + return $params[1][0]['dbname'] ?? null; + } + + public static function getTarget(array $params): ?string + { + $query = $params[0] ?? null; + + if (!$query) { + return null; + } + + // Fetch target name + $matches = []; + preg_match_all('/( from| into| update| join)\s*([a-zA-Z0-9`"[\]_]+)/i', $query, $matches); + + $targetName = null; + if ($matches !== []) { + $targetName = $matches[2][0] ?? null; + } + if ($targetName === null) { + return null; + } + //strip quotes and backticks from the target name + $targetName = str_replace(['`', '"', '[', ']'], '', $targetName); + + return $targetName; } /** @@ -125,6 +150,14 @@ private static function getDbNamespace(array $params): string * See https://opentelemetry.io/docs/specs/semconv/database/database-spans/#generating-a-summary-of-the-query-text */ public static function getDbQuerySummary(array $params): string + { + $operationName = self::getDbOperationName($params); + $targetName = self::getTarget($params); + + return $operationName . ($targetName ? ' ' . $targetName : ''); + } + + public static function getDbOperationName(array $params): string { $query = $params[0] ?? null; @@ -136,19 +169,6 @@ public static function getDbQuerySummary(array $params): string $operationName = explode(' ', $query); $operationName = $operationName[0]; - // Fetch target name - $matches = []; - preg_match_all('/( from| into| update| join)\s*([a-zA-Z0-9`"[\]_]+)/i', $query, $matches); - - $targetName = null; - if (strtolower($operationName) == 'select') { - if ($matches[2]) { - $targetName = implode(' ', $matches[2]); - } - } elseif ($matches) { - $targetName = $matches[2][0] ?? ''; - } - - return $operationName . ($targetName ? ' ' . $targetName : ''); + return $operationName; } } diff --git a/src/Instrumentation/Doctrine/src/DoctrineInstrumentation.php b/src/Instrumentation/Doctrine/src/DoctrineInstrumentation.php index c37003850..82f6eb9b9 100644 --- a/src/Instrumentation/Doctrine/src/DoctrineInstrumentation.php +++ b/src/Instrumentation/Doctrine/src/DoctrineInstrumentation.php @@ -4,15 +4,15 @@ namespace OpenTelemetry\Contrib\Instrumentation\Doctrine; +use Doctrine\DBAL\Driver\Result as ResultInterface; +use Doctrine\DBAL\Driver\Statement; use OpenTelemetry\API\Instrumentation\CachedInstrumentation; use OpenTelemetry\API\Trace\Span; use OpenTelemetry\API\Trace\SpanBuilderInterface; use OpenTelemetry\API\Trace\SpanKind; use OpenTelemetry\API\Trace\StatusCode; use OpenTelemetry\Context\Context; - use function OpenTelemetry\Instrumentation\hook; - use OpenTelemetry\SemConv\TraceAttributes; use Throwable; @@ -23,6 +23,7 @@ class DoctrineInstrumentation public static function register(): void { $instrumentation = new CachedInstrumentation('io.opentelemetry.contrib.php.doctrine'); + $tracker = new DoctrineTracker(); hook( \Doctrine\DBAL\Driver::class, @@ -30,11 +31,11 @@ public static function register(): void pre: static function (\Doctrine\DBAL\Driver $driver, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) { /** @psalm-suppress ArgumentTypeCoercion */ $builder = self::makeBuilder($instrumentation, 'Doctrine\DBAL\Driver::connect', $function, $class, $filename, $lineno) - ->setSpanKind(SpanKind::KIND_CLIENT) - ->setAttribute(TraceAttributes::SERVER_ADDRESS, AttributesResolver::get(TraceAttributes::SERVER_ADDRESS, func_get_args())) - ->setAttribute(TraceAttributes::SERVER_PORT, AttributesResolver::get(TraceAttributes::SERVER_PORT, func_get_args())) - ->setAttribute(TraceAttributes::DB_SYSTEM_NAME, AttributesResolver::get(TraceAttributes::DB_SYSTEM_NAME, func_get_args())) - ->setAttribute(TraceAttributes::DB_NAMESPACE, AttributesResolver::get(TraceAttributes::DB_NAMESPACE, func_get_args())); + ->setSpanKind(SpanKind::KIND_CLIENT) + ->setAttribute(TraceAttributes::SERVER_ADDRESS, AttributesResolver::get(TraceAttributes::SERVER_ADDRESS, func_get_args())) + ->setAttribute(TraceAttributes::SERVER_PORT, AttributesResolver::get(TraceAttributes::SERVER_PORT, func_get_args())) + ->setAttribute(TraceAttributes::DB_SYSTEM_NAME, AttributesResolver::get(TraceAttributes::DB_SYSTEM_NAME, func_get_args())) + ->setAttribute(TraceAttributes::DB_NAMESPACE, AttributesResolver::get(TraceAttributes::DB_NAMESPACE, func_get_args())); $parent = Context::getCurrent(); $span = $builder->startSpan(); Context::storage()->attach($span->storeInContext($parent)); @@ -52,6 +53,8 @@ public static function register(): void $builder = self::makeBuilder($instrumentation, AttributesResolver::getDbQuerySummary($params), $function, $class, $filename, $lineno) ->setSpanKind(SpanKind::KIND_CLIENT); $builder->setAttribute(TraceAttributes::DB_QUERY_TEXT, AttributesResolver::get(TraceAttributes::DB_QUERY_TEXT, func_get_args())); + $builder->setAttribute(TraceAttributes::DB_OPERATION_NAME, AttributesResolver::getDbOperationName($params)); + $builder->setAttribute(TraceAttributes::DB_COLLECTION_NAME, AttributesResolver::getTarget($params)); $parent = Context::getCurrent(); $span = $builder->startSpan(); Context::storage()->attach($span->storeInContext($parent)); @@ -68,7 +71,9 @@ public static function register(): void /** @psalm-suppress ArgumentTypeCoercion */ $builder = self::makeBuilder($instrumentation, AttributesResolver::getDbQuerySummary($params), $function, $class, $filename, $lineno) ->setSpanKind(SpanKind::KIND_CLIENT) - ->setAttribute(TraceAttributes::DB_QUERY_TEXT, AttributesResolver::get(TraceAttributes::DB_QUERY_TEXT, func_get_args())); + ->setAttribute(TraceAttributes::DB_QUERY_TEXT, AttributesResolver::get(TraceAttributes::DB_QUERY_TEXT, func_get_args())) + ->setAttribute(TraceAttributes::DB_OPERATION_NAME, AttributesResolver::getDbOperationName($params)) + ->setAttribute(TraceAttributes::DB_COLLECTION_NAME, AttributesResolver::getTarget($params)); $parent = Context::getCurrent(); $span = $builder->startSpan(); @@ -86,13 +91,22 @@ public static function register(): void /** @psalm-suppress ArgumentTypeCoercion */ $builder = self::makeBuilder($instrumentation, AttributesResolver::getDbQuerySummary($params), $function, $class, $filename, $lineno) ->setSpanKind(SpanKind::KIND_CLIENT) - ->setAttribute(TraceAttributes::DB_QUERY_TEXT, AttributesResolver::get(TraceAttributes::DB_QUERY_TEXT, func_get_args())); + ->setAttribute(TraceAttributes::DB_QUERY_TEXT, AttributesResolver::get(TraceAttributes::DB_QUERY_TEXT, func_get_args())) + ->setAttribute(TraceAttributes::DB_OPERATION_NAME, 'prepare'); $parent = Context::getCurrent(); $span = $builder->startSpan(); Context::storage()->attach($span->storeInContext($parent)); }, - post: static function (\Doctrine\DBAL\Driver\Connection $connection, array $params, mixed $statement, ?Throwable $exception) { + post: static function (\Doctrine\DBAL\Driver\Connection $connection, array $params, ?Statement $statement, ?Throwable $exception) use ($tracker) { + if ($statement) { + $scope = Context::storage()->scope(); + $context = $scope?->context(); + if ($context) { + $span = Span::fromContext($context); + $tracker->trackStatement($statement, $span->getContext()); + } + } self::end($exception); } ); @@ -102,8 +116,9 @@ public static function register(): void 'beginTransaction', pre: static function (\Doctrine\DBAL\Driver\Connection $connection, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) { /** @psalm-suppress ArgumentTypeCoercion */ - $builder = self::makeBuilder($instrumentation, 'Doctrine\DBAL\Driver\Connection::beginTransaction', $function, $class, $filename, $lineno) - ->setSpanKind(SpanKind::KIND_CLIENT); + $builder = self::makeBuilder($instrumentation, 'Doctrine::beginTransaction', $function, $class, $filename, $lineno) + ->setSpanKind(SpanKind::KIND_CLIENT) + ->setAttribute(TraceAttributes::DB_OPERATION_NAME, 'begin'); $parent = Context::getCurrent(); $span = $builder->startSpan(); @@ -119,8 +134,9 @@ public static function register(): void 'commit', pre: static function (\Doctrine\DBAL\Driver\Connection $connection, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) { /** @psalm-suppress ArgumentTypeCoercion */ - $builder = self::makeBuilder($instrumentation, 'Doctrine\DBAL\Driver\Connection::commit', $function, $class, $filename, $lineno) - ->setSpanKind(SpanKind::KIND_CLIENT); + $builder = self::makeBuilder($instrumentation, 'Doctrine::commit', $function, $class, $filename, $lineno) + ->setSpanKind(SpanKind::KIND_CLIENT) + ->setAttribute(TraceAttributes::DB_OPERATION_NAME, 'commit'); $parent = Context::getCurrent(); $span = $builder->startSpan(); @@ -136,8 +152,9 @@ public static function register(): void 'rollBack', pre: static function (\Doctrine\DBAL\Driver\Connection $connection, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) { /** @psalm-suppress ArgumentTypeCoercion */ - $builder = self::makeBuilder($instrumentation, 'Doctrine\DBAL\Driver\Connection::rollBack', $function, $class, $filename, $lineno) - ->setSpanKind(SpanKind::KIND_CLIENT); + $builder = self::makeBuilder($instrumentation, 'Doctrine::rollBack', $function, $class, $filename, $lineno) + ->setSpanKind(SpanKind::KIND_CLIENT) + ->setAttribute(TraceAttributes::DB_OPERATION_NAME, 'rollback'); $parent = Context::getCurrent(); $span = $builder->startSpan(); Context::storage()->attach($span->storeInContext($parent)); @@ -146,7 +163,29 @@ public static function register(): void self::end($exception); } ); + + hook( + \Doctrine\DBAL\Driver\Statement::class, + 'execute', + pre: static function (\Doctrine\DBAL\Driver\Statement $statement, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation, $tracker) { + /** @psalm-suppress ArgumentTypeCoercion */ + $builder = self::makeBuilder($instrumentation, 'Doctrine::execute', $function, $class, $filename, $lineno) + ->setSpanKind(SpanKind::KIND_CLIENT) + ->setAttribute(TraceAttributes::DB_OPERATION_NAME, 'execute'); + if ($ctx = $tracker->getSpanContextForStatement($statement)) { + $builder->addLink($ctx); + } + $parent = Context::getCurrent(); + $span = $builder->startSpan(); + + Context::storage()->attach($span->storeInContext($parent)); + }, + post: static function (\Doctrine\DBAL\Driver\Statement $statement, array $params, ResultInterface $result, ?Throwable $exception) { + self::end($exception); + } + ); } + private static function makeBuilder( CachedInstrumentation $instrumentation, string $name, diff --git a/src/Instrumentation/Doctrine/src/DoctrineTracker.php b/src/Instrumentation/Doctrine/src/DoctrineTracker.php new file mode 100644 index 000000000..19d45675d --- /dev/null +++ b/src/Instrumentation/Doctrine/src/DoctrineTracker.php @@ -0,0 +1,30 @@ +statementToSpanContextMap[$statement] = $context; + } + + public function getSpanContextForStatement(Statement $statement): ?SpanContextInterface + { + return $this->statementToSpanContextMap[$statement] ?? null; + } +} diff --git a/src/Instrumentation/Doctrine/tests/Integration/DoctrineInstrumentationTest.php b/src/Instrumentation/Doctrine/tests/Integration/DoctrineInstrumentationTest.php index d4aa4feef..b1e37172b 100644 --- a/src/Instrumentation/Doctrine/tests/Integration/DoctrineInstrumentationTest.php +++ b/src/Instrumentation/Doctrine/tests/Integration/DoctrineInstrumentationTest.php @@ -108,12 +108,64 @@ public function test_statement_execution(): void $connection->prepare('SELECT * FROM `technology`'); $span = $this->storage->offsetGet(2); - $this->assertSame('SELECT `technology`', $span->getName()); - $this->assertCount(3, $this->storage); + $this->assertSame('SELECT technology', $span->getName()); + $this->assertSame('prepare', $span->getAttributes()->get(TraceAttributes::DB_OPERATION_NAME)); $connection->executeQuery('SELECT * FROM `technology`'); $span = $this->storage->offsetGet(3); - $this->assertSame('SELECT `technology`', $span->getName()); + $this->assertSame('SELECT technology', $span->getName()); + $this->assertSame('SELECT', $span->getAttributes()->get(TraceAttributes::DB_OPERATION_NAME)); + $this->assertCount(4, $this->storage); + } + + public function test_prepare_then_execute_statement(): void + { + $connection = self::createConnection(); + $statement = self::fillDB(); + $connection->executeStatement($statement); + + $stmt = $connection->prepare('SELECT * FROM `technology` WHERE name = :name'); + $prepare = $this->storage->offsetGet(2); + $this->assertSame('prepare', $prepare->getAttributes()->get(TraceAttributes::DB_OPERATION_NAME)); + $this->assertSame('SELECT technology', $prepare->getName()); + + $stmt->bindValue('name', 'PHP'); + $stmt->executeQuery(); + $execute = $this->storage->offsetGet(3); + $this->assertSame('Doctrine::execute', $execute->getName()); + $this->assertSame('execute', $execute->getAttributes()->get(TraceAttributes::DB_OPERATION_NAME)); + $this->assertCount(1, $execute->getLinks()); + $this->assertEquals($prepare->getContext(), $execute->getLinks()[0]->getSpanContext(), 'execute span is linked to prepare span'); + } + + public function test_tracked_span_context_when_prepare_span_flushed(): void + { + $connection = self::createConnection(); + $statement = self::fillDB(); + $connection->executeStatement($statement); + + $stmt = $connection->prepare('SELECT * FROM `technology` WHERE name = :name'); + $this->storage->exchangeArray([]); //removes the reference to prepared span, including the tracked SpanContext + $stmt->bindValue('name', 'PHP'); + $stmt->executeQuery(); + + $execute = $this->storage->offsetGet(0); + $this->assertCount(1, $execute->getLinks()); + } + + public function test_query_with_bind_variables(): void + { + $connection = self::createConnection(); + $statement = self::fillDB(); + $connection->executeStatement($statement); + + $connection->executeQuery('SELECT * FROM `technology` WHERE name = :name', ['name' => 'PHP']); + $prepare = $this->storage->offsetGet(2); + $this->assertSame('SELECT technology', $prepare->getName()); + $execute = $this->storage->offsetGet(3); + $this->assertSame('Doctrine::execute', $execute->getName()); + $this->assertCount(1, $execute->getLinks()); + $this->assertEquals($prepare->getContext(), $execute->getLinks()[0]->getSpanContext(), 'execute span is linked to prepare span'); $this->assertCount(4, $this->storage); } @@ -122,7 +174,8 @@ public function test_transaction(): void $connection = self::createConnection(); $connection->beginTransaction(); $span = $this->storage->offsetGet(1); - $this->assertSame('Doctrine\DBAL\Driver\Connection::beginTransaction', $span->getName()); + $this->assertSame('Doctrine::beginTransaction', $span->getName()); + $this->assertSame('begin', $span->getAttributes()->get(TraceAttributes::DB_OPERATION_NAME)); $this->assertCount(2, $this->storage); $statement = self::fillDB(); @@ -131,7 +184,8 @@ public function test_transaction(): void $this->assertSame('CREATE technology', $span->getName()); $connection->commit(); $span = $this->storage->offsetGet(3); - $this->assertSame('Doctrine\DBAL\Driver\Connection::commit', $span->getName()); + $this->assertSame('Doctrine::commit', $span->getName()); + $this->assertSame('commit', $span->getAttributes()->get(TraceAttributes::DB_OPERATION_NAME)); $this->assertCount(4, $this->storage); $connection->beginTransaction(); @@ -140,7 +194,8 @@ public function test_transaction(): void $connection->executeStatement("INSERT INTO technology(`name`, `date`) VALUES('Java', '1995-05-23');"); $connection->rollback(); $span = $this->storage->offsetGet(6); - $this->assertSame('Doctrine\DBAL\Driver\Connection::rollBack', $span->getName()); + $this->assertSame('Doctrine::rollBack', $span->getName()); + $this->assertSame('rollback', $span->getAttributes()->get(TraceAttributes::DB_OPERATION_NAME)); $this->assertCount(7, $this->storage); $this->assertFalse($connection->isTransactionActive());