add Doctrine span links and more SemConv (#374)

* add Doctrine span links and more SemConv
track doctrine prepared statements, and when they are executed create a span link back to the span that
prepared.
add some more SemConv to db spans, particularly db.operation.name
align span names with how PDO does it (eg Doctrine::prepare)

* phan

* allow doctrine + php 8.1

* apply review feedback
- tidy constructor
- do not use a WeakMap to track prepared statements, as the SpanContext is lost when prepare span flushed/exported

* revert SplObjectStorage usage

Author: brettmc

.github/workflows/php.yml

@@ -64,8 +64,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'

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",

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,24 +107,57 @@ 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
     {
         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;
     }
 
     /**
      * Resolve attribute `db.query.summary`
      * 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;
     }
 }

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,18 +23,19 @@ class DoctrineInstrumentation
     public static function register(): void
     {
         $instrumentation = new CachedInstrumentation('io.opentelemetry.contrib.php.doctrine');
+        $tracker = new DoctrineTracker();
 
         hook(
             \Doctrine\DBAL\Driver::class,
             'connect',
             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,

src/Instrumentation/Doctrine/src/DoctrineTracker.php

@@ -0,0 +1,30 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenTelemetry\Contrib\Instrumentation\Doctrine;
+
+use Doctrine\DBAL\Driver\Statement;
+use OpenTelemetry\API\Trace\SpanContextInterface;
+use WeakMap;
+
+/**
+ * @internal
+ */
+class DoctrineTracker
+{
+    public function __construct(
+        private WeakMap $statementToSpanContextMap = new WeakMap(),
+    ) {
+    }
+
+    public function trackStatement(Statement $statement, SpanContextInterface $context): void
+    {
+        $this->statementToSpanContextMap[$statement] = $context;
+    }
+
+    public function getSpanContextForStatement(Statement $statement): ?SpanContextInterface
+    {
+        return $this->statementToSpanContextMap[$statement] ?? null;
+    }
+}

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());