Add support for Doctrine auto-instrumentation #300
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #300 +/- ##
============================================
- Coverage 80.35% 79.51% -0.84%
+ Complexity 1025 626 -399
============================================
Files 98 68 -30
Lines 4112 2738 -1374
============================================
- Hits 3304 2177 -1127
+ Misses 808 561 -247 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 33 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
@DominicDetta one more thing to add is an entry to the top-level |
|
@brettmc I included the Doctrine directory in the files you specified |
|
I agreed the EasyCLA and waiting for the merge approval |
|
Can you also update workflows/php to exclude 7.4 from the version matrix? The php version requirement is ^8.2, but I think it would work back to 8.0 since it doesn't hook internal functions. Either way, we need to either drop the requirement back to 8.0 or exclude pre-8.2 versions from the test matrix. |
|
it's done |
|
In the end I integrated again the PHP 7.4 version and excluded Doctrine from the pre-8.2 versions tests |
|
Green build is an excellent start. I'm happy to push this out as a 0.0.1 release. Since this works against doctrine 3 (according to the composer.json), and doesn't hook any internal/extension functions, I think it would probably work in 8.0 and 8.1 as well. But, that's not a blocker to getting this out if you're happy with the PR as it is. |
|
I'm pretty confident it works even with PHP 8.0 version. |
There was a problem hiding this comment.
Hooking driver methods will cause duplicated spans if a middleware like Symfony Debug Middleware is used.
An alternative approach to avoid this problem would be to move the implementation into a middleware and use a hook to inject this middleware into every connection; see also Nevay/otel-instrumentation-doctrine-dbal.
| $builder | ||
| ->setAttribute(TraceAttributes::SERVER_ADDRESS, $params[0]['host'] ?? 'unknown') | ||
| ->setAttribute(TraceAttributes::SERVER_PORT, $params[0]['port'] ?? 'unknown') | ||
| ->setAttribute(TraceAttributes::DB_SYSTEM, $params[0]['driver'] ?? 'unknown') |
There was a problem hiding this comment.
IMO we should try to follow semconv even if we don't specify a schema url; see semconv:
db.system has the following list of well-known values. If one of them applies, then the respective value MUST be used
There was a problem hiding this comment.
Ok, I didnt know there was a convention. I will apply the correct ones.
Co-authored-by: Tobias Bachert <git@b-privat.de>
Co-authored-by: Tobias Bachert <git@b-privat.de>
The duplicate problem is also a common problem with other auto-instrumentations. In fact, we could not use psr-15 as it generated too many spans. I wonder if I really need to fix this problem or is there a way to filter the results. |
AFAIK the idea is to allow the agent to do that: |
There was a problem hiding this comment.
My proposal is to figure out if this instrumentation should depend on the existence of PDO and then define how the two interact. My understanding is that if a PDO is used and the PDO instrumentation is active, there will be 2 nested CLIENT spans for the same interaction. - IMHO this is severely impacting the user experience.
| 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); |
There was a problem hiding this comment.
How do we think about the duality of PDO and dbal here? Most instrumented methods have a pendant in the PDO instrumentation i.e. - a compatible pdo driver will cause two client spans and the hierarchy will probably be
my_func (internal)
-> connect (client) # from dbal
-> PDO::__construct (client ) # from pdo
IMO the two should be either mutually exclusive or enrich each other. WDYT?
There was a problem hiding this comment.
We do not have a way for two auto-instrumentations to cooperate like this (it's an idea that has come up before, and would be cool). So I think mutually exclusive is the best we have - or at least it's up to the user to install only one instrumentation, I don't think we need to go as far as setting them up as conflicting in composer.
Is documenting that recommendation good enough for now?
edit: a hacky approach would be to have this package check if PDO instrumentation is installed and enabled, and only apply some of its hooks (if it provides any value that PDO itself doesn't)...?
There was a problem hiding this comment.
Could the PDO instrumentation re-write the doctrine span from CLIENT to internal or bail if it recognizes the parent is already client?
There was a problem hiding this comment.
To your point - i think documenting it makes the experience worse because users then need to make a whole lot of decisions when instrumenting
There was a problem hiding this comment.
i think documenting it makes the experience worse
I only meant documenting such as "use pdo or doctrine auto-instrumentation, but probably not both" - I expect users to make decisions about which auto-instrumentations to install based on their workload/stack...just installing everything available is probably too noisy.
Could the PDO instrumentation re-write the doctrine span from CLIENT to internal or bail if it recognizes the parent is already client?
I think the issue here would be that pre/post hooks operate in isolation, so there's currently no way for a post hook to know whether the pre hook created a span or just modified the active span. All the implementations we have assume that pre created and activated a span, and post just closes whichever is the active span at the time.
We have previously brainstormed whether we could manage some state between a pre and post hook, but we thought it was going to be a hard problem.
There was a problem hiding this comment.
I expect users to make decisions about which auto-instrumentations to install based on their workload/stack...just installing everything available is probably too noisy.
I completely agree on this matter.
There was a problem hiding this comment.
Could the PDO instrumentation ... bail if it recognizes the parent is already client?
Bailing out requires storing additional data within the Context to be able to detect duplicate instrumentations, see Javas span suppression strategies:
Instrumentation span suppression behavior
SpanSuppressionStrategy.java
SpanSuppressors.java
AFAIK the idea is to allow the agent to do that
Dropping at the agent is too late as it may lead to broken traces "Dropping a span may lead to orphaned spans if the dropped span is a parent." ref.
there's currently no way for a post hook to know whether the pre hook created a span or just modified the active span
Scopes attached via Context::storage()->attach() implement ArrayAccess to allow propagating state from pre hook to post hook ref.
hook(
null,
'example',
static function() use ($tracer): void {
$context = Context::getCurrent();
if (lcg_value() > .5 /* some suppression condition */) {
$span = $tracer
->spanBuilder('example')
->startSpan();
$context = $span->storeInContext($context);
}
$scope = Context::storage()->attach($context);
$scope[SpanInterface::class] = $span ?? null;
},
static function(): void {
if (!$scope = Context::storage()->scope()) {
return;
}
$scope->detach();
/** @var SpanInterface|null $span */
$span = $scope[SpanInterface::class] ?? null;
$span?->end();
},
);| 'query', | ||
| 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::query', $function, $class, $filename, $lineno) |
There was a problem hiding this comment.
The recommended span name here would be something like SELECT my_table.
The duplication with the pdo instrumentation is the same here.
https://opentelemetry.io/docs/specs/semconv/database/database-spans/#name
There was a problem hiding this comment.
Ok. Do you know a good agnostic SQL parser to achieve this? The one I found is focusing on Mysql syntax.
There was a problem hiding this comment.
Doctrine already knows the table name(s).
There was a problem hiding this comment.
Doctrine already knows the table name(s).
With the approach I'm following right now, I dont have this information. Are you referring to Doctrine ORM? I'm creating hooks on the methods of \Doctrine\DBAL\Driver and \Doctrine\DBAL\Driver\Connection
There was a problem hiding this comment.
I see, that's a good point.
The issue with this approach is whichever parser you choose is bound to be quite slow and will probably be slowest part of the call (which includes the actual query), making it unusable for production use. ORM itself has a DQL query parser (which is close enough for high level comparison) and it's basically unusable in prod without it being cached with ORM query cache.
Running a random query parser on every DB query is unlikely to be viable.
There was a problem hiding this comment.
I think I will use a simple regex expression to identify the db.operation.name and target from the query text
There was a problem hiding this comment.
IMO we could consider the ORM instrumentation even if it's still not here yet.
For example, since ORM will know what the table names are, you could assume they will be put in context by the ORM instrumentation and then only try to detect it if not there? This would future proof this implementation and allow extensions when they happen.
There was a problem hiding this comment.
Sorry for the late response but I'm pretty busy at the moment.
IMO we could consider the ORM instrumentation even if it's still not here yet.
For example, since ORM will know what the table names are, you could assume they will be put in context by the ORM instrumentation and then only try to detect it if not there? This would future proof this implementation and allow extensions when they happen.
I see your point but my focus is to implement hooks for Doctrine DBAL and in the future time permitted we could refactor the functions to behave as you suggested.
No description provided.