add Doctrine span links and more SemConv #374
Conversation
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)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #374 +/- ##
============================================
+ Coverage 81.68% 83.16% +1.47%
- Complexity 1741 1957 +216
============================================
Files 133 143 +10
Lines 7307 8190 +883
============================================
+ Hits 5969 6811 +842
- Misses 1338 1379 +41 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 19 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@open-telemetry/php-approvers ping! |
|
|
||
| public function trackStatement(Statement $statement, SpanContextInterface $context): void | ||
| { | ||
| $this->statementToSpanContextMap[$statement] = WeakReference::create($context); |
There was a problem hiding this comment.
This looks like it will return null after the ::prepare() span is flushed and exported? Should be testable by something like:
$stmt = $connection->prepare('SELECT * FROM `technology` WHERE name = :name');
$this->storage->exchangeArray([]);
$stmt->bindValue('name', 'PHP');
$stmt->executeQuery();
$execute = $this->storage->offsetGet(0);
$this->assertCount(1, $execute->getLinks());| $this->statementToSpanContextMap[$statement] = WeakReference::create($context); | |
| $this->statementToSpanContextMap[$statement] = $context; |
There was a problem hiding this comment.
Yes, you're right. I've used SplObjectStorage now, and added a test. It doesn't look like prepared statements can be destroyed easily, so I guess holding on to span context forever is okay...
There was a problem hiding this comment.
Should revert the WeakMap->SplObjectStorage change to avoid leaking memory; only the usage of WeakReference was problematic.
There was a problem hiding this comment.
Reverted to WeakMap
- tidy constructor - do not use a WeakMap to track prepared statements, as the SpanContext is lost when prepare span flushed/exported
Fixes: open-telemetry/opentelemetry-php#1578