Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DbalTracingPass is overridden by DoctrineBundle's own MiddlewaresPass #805

Closed
fridtjof opened this issue Feb 4, 2024 · 4 comments · Fixed by #808
Closed

DbalTracingPass is overridden by DoctrineBundle's own MiddlewaresPass #805

fridtjof opened this issue Feb 4, 2024 · 4 comments · Fixed by #808

Comments

@fridtjof
Copy link

fridtjof commented Feb 4, 2024

How do you use Sentry?

Sentry SaaS (sentry.io)

SDK version

4.13.2

Steps to reproduce

Relevant dependencies:

Package Version
symfony 5.4
symfony/dependency-injection 5.4.34
doctrine/doctrine-bundle 2.11.1
doctrine/dbal 3.7.3

Nothing special otherwise, just have Sentry-Symfony and its profiling integration activated.

Expected result

Sentry hooks into DBAL, to enable its profiling functionality for queries

Actual result

Sentry fails to hook into DBAL, because DBALs MiddlewarePass adds another setMiddlewares invocation that ends up overwriting the call placed there by DbalTracingPass. MiddlewarePass always seems to place that call, no matter if the set of middlewares it adds is empty or not (e.g. with or without Symfony's own profiling middleware).

I suppose the order in which these passes run is important (it'll work if DbalTracingPass runs last (because it removes a previous setMiddlewares call anyway), but it won't work the other way around)

Looking at the generated container in var/cache:

protected function getDoctrine_Dbal_DefaultConnectionService()
    {
        $a = new \Doctrine\DBAL\Configuration();

        $b = new \Doctrine\Bundle\DoctrineBundle\Middleware\DebugMiddleware(($this->privates['doctrine.debug_data_holder'] ?? ($this->privates['doctrine.debug_data_holder'] = new \Doctrine\Bundle\DoctrineBundle\Middleware\BacktraceDebugDataHolder([]))), ($this->privates['debug.stopwatch'] ?? ($this->privates['debug.stopwatch'] = new \Symfony\Component\Stopwatch\Stopwatch(true))));
        $b->setConnectionName('default');

        $a->setSchemaManagerFactory(new \Doctrine\DBAL\Schema\LegacySchemaManagerFactory());
        // DbalTracingPass:
        $a->setMiddlewares([0 => new \Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverMiddleware(new \Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverConnectionFactory(($this->privates['Sentry\\State\\HubInterface'] ?? $this->getHubInterfaceService())))]);
        // MiddlewarePass:
        $a->setMiddlewares([0 => $b]);

[...]
    }

It seems to me that the proper way to go here if possible, is to cooperate with the MiddlewarePass instead of fighting it, e.g. attempting to get TracingDriverMiddleware tagged properly.
Usually, the appropriate tag needed for MiddlewarePass to find middlewares should be added automatically through autowiring to anything implementing its Middleware interface, or marked through the AsMiddleware PHP attribute. The tagging is done by DoctrineExtension.

I don't know enough about autowiring and Symfony's DI system to answer why this isn't working:

  • TracingDriverMiddleware (indirectly) implements DBAL v3's Middleware interface - maybe it would need to implement it directly?
  • Manually adding the AsMiddleware attribute did not help, so maybe autowiring just ignores this? Or it's not configured correctly to look at it..
@fridtjof
Copy link
Author

fridtjof commented Feb 4, 2024

So, adding autoconfigure="true" for TracingDriverMiddleware in the bundle's services.xml magically made it work! (even though I can't choose which connections to trace like this)

I can see that Doctrine lets you specify a connection on a tag, so rewriting DbalTracingPass to just add the middleware tag(s) manually should make this much more robust - unless I missed something?

@ste93cry
Copy link
Collaborator

ste93cry commented Feb 5, 2024

Can you please try registering the Sentry bundle after DoctrineBundle in config/bundles.php?

@fridtjof
Copy link
Author

fridtjof commented Feb 6, 2024

Makes no difference, unfortunately. MiddlewaresPass still seems to run first

@fridtjof
Copy link
Author

fridtjof commented Feb 6, 2024

For now, I'm working around this with the following snippet in services.yaml:

Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverMiddleware:
  autoconfigure: true
  arguments:
    - '@Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverConnectionFactoryInterface'

This is basically what I did before, just in my application's service configuration so I don't need to modify dependency code.

It seems to work, even though it needlessly instantiates TracingDriverMiddleware again (because my definition is technically a separate service, I suppose)

protected static function getDoctrine_Dbal_DefaultConnectionService($container)
    {
        $a = new \Doctrine\DBAL\Configuration();

        $b = new \Doctrine\Bundle\DoctrineBundle\Middleware\DebugMiddleware(($container->privates['doctrine.debug_data_holder'] ??= new \Doctrine\Bundle\DoctrineBundle\Middleware\BacktraceDebugDataHolder([])), ($container->services['debug.stopwatch'] ??= new \Symfony\Component\Stopwatch\Stopwatch(true)));
        $b->setConnectionName('default');

        $a->setSchemaManagerFactory(new \Doctrine\DBAL\Schema\LegacySchemaManagerFactory());
        $a->setMiddlewares([($container->services['Sentry\\SentryBundle\\Tracing\\Doctrine\\DBAL\\TracingDriverMiddleware'] ?? self::getTracingDriverMiddlewareService($container))]);
        $a->setMiddlewares([new \Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverMiddleware(($container->privates['Sentry\\SentryBundle\\Tracing\\Doctrine\\DBAL\\TracingDriverConnectionFactory'] ?? self::getTracingDriverConnectionFactoryService($container))), $b]);

      // ...
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants