Add ability to assign your own SQLLogger

[rjd22]

At this moment adding your own SQL logger is not possible because the set is fixed.

I've been working on the implementation of the Sentry Performance integration and after a lot of poking and prodding there just doesn't seem to be a way to do this nicely. So I had to come up with trickery by replacing the DbalLogger.

Would it be acceptable for you to give an PR that allows you to set your own custom logger next to the DbalLogger?

Link to sentry PR:
getsentry/sentry-symfony#426

[ste93cry]

It looks like this is already supported when configuring the logging option of the connection.

DoctrineBundle/DependencyInjection/DoctrineExtension.php

Lines 154 to 163 in 6a4b958

	if ($logger !== null) {
	$chainLogger = new ChildDefinition('doctrine.dbal.logger.chain');
	$chainLogger->addMethodCall('addLogger', [$profilingLogger]);
	$loggerId = 'doctrine.dbal.logger.chain.' . $name;
	$container->setDefinition($loggerId, $chainLogger);
	$logger = new Reference($loggerId);
	} else {
	$logger = $profilingLogger;
	}

[rjd22]

@ste93cry All these loggers are non configurable. And as soon as the loggerChain is made there isn't a possibility to add another logger because the addLogger method is deprecated. As you can see in the later version of the Extension:

DoctrineBundle/DependencyInjection/DoctrineExtension.php

Lines 162 to 180 in 015fdd4

	if ($logger !== null) {
	$chainLogger = $container->register(
	'doctrine.dbal.logger.chain',
	LoggerChain::class
	);
	if (! method_exists(SQLParserUtils::class, 'getPositionalPlaceholderPositions') && method_exists(LoggerChain::class, 'addLogger')) {
	// doctrine/dbal < 2.10.0
	$chainLogger->addMethodCall('addLogger', [$profilingLogger]);
	} else {
	$chainLogger->addArgument([$logger, $profilingLogger]);
	}
	$loggerId = 'doctrine.dbal.logger.chain.' . $name;
	$container->setDefinition($loggerId, $chainLogger);
	$logger = new Reference($loggerId);
	} else {
	$logger = $profilingLogger;
	}
	}

Another problem is for the loggerChain to work you need to enable dbal.profiling what also enables the DebugStack and BacktraceLogger. This is most likely something you want to avoid on production.

Only setting dbal.logging enables the DbalLogger that can be easily replaced. But it would be better if we could set a loggerChain so that a developer can decide to replace it or add another logger.

Thank you for checking up on this, I could really use another pair of eyes on thing issue.

[ste93cry]

And as soon as the loggerChain is made there isn't a possibility to add another logger because the addLogger method is deprecated

That should not be an issue, we can always edit the argument of the constructor in our compiler pass to add the logger, even though it would be nicer if the services were tagged so that we could use !tagged in the service definition

Another problem is for the loggerChain to work you need to enable dbal.profiling

You're definitely right, I totally missed the wrapping if ($connection['profiling']) { statement. I see two ways to solve the issue: either we change the implementation here to always use the LoggerChain logger, or in our own compiler pass in Sentry we enforce that the logger is always a LoggerChain

[rjd22]

@ste93cry my recommendation would be to add an extra configuration option where you can set the loggers you want to attach when logging is enabled that contains the DbalLogger by default.

This will make it easy for the developer to attach an extra logger or replace the DbalLogger when enabling logging per connection.

[ostrolucky]

Yes, this is welcome. I've put building blocks for this feature one year ago - with that I expected that providing this feature would be trivial. This is not the first time we got such request , we even had a PR adding this, but was rejected because of various reasons. What we expect here though is not adding more configuration options, but having doctrine.dbal.logger tag. DoctrineBundle then automatically picks up everything tagged as such via !tagged.

I'm also up to dropping dbal 2.9.0. Only reason compatibility layer with dbal 2.9 was added was because DBAL 3.x support was already present in doctrine-bundle 1.12.x, but was incomplete/broken and needed fixing.

The only things that could be tricky (this was also a problem in #692) is handling of dbal.connections.*.logging, because it means we can neither register profiling/backtrace loggers into main chain logger, nor clear existing tags, as it would affect all connections. But I'll leave that up to whoever creates PR to solve it.

[rjd22]

Do you mean with the tricky part, that you need the ability to assign a logger per connection?

Can't we solve this by adding an argument to the tag that contains the connection you want to add the logger to and else if it's empty to logger will be added to all connections?

Do I understand this correctly?

[ostrolucky]

Yes to all

[ostrolucky]

Hmm actually we cannot drop DBAL < 2.10, because 2.10 requires PHP 7.2 :(

[rjd22]

@ostrolucky Well I've read the PR that was not merged, I feel that to make it work nicely we need to rebuild the whole logger attach section. This is not a trivial amount of work.

I also wonder if you want to attach all loggers with a tag. So also the DbalLogger and profiler loggers. If so it might be unavoidable to attach the ChainLogger even if we only have one logger. I wonder if that is acceptable because it would keep things move consistent overall, with the downside of a really small overhead.

Edit:
To be honest I kind of feel the whole logger section is kind of a mess, all to avoid attaching the ChainLogger when there is only one logger. Also I think that it would serve the developer better if we make the ChainLogger independent of the logging option since developers might want to attach a logger but not enable logging itself.

[ostrolucky]

Yes, having ChainLogger always registered is acceptable. Also, I think we cannot use tagged_iterator in the end, because it would register all loggers no matter the connection :/ So I think we will have to go old fashioned way, where we iterate over tagged services in compiler pass ourselves - ther we can read connection attribute of the tag.

I think that it would serve the developer better if we make the ChainLogger independent of the logging option since developers might want to attach a logger but not enable logging itself.

I think I agree. So perhaps attach doctrine.dbal.logger tag to doctrine.dbal.logger only if logging is enabled, but attach all the other services with such tags no matter the option?

[rjd22]

@ostrolucky The section where I just can't wrap my head around is the following section. It seems that the logger is attached to the logger chain but never used for default connections. But I feel that this is not possible:

DoctrineBundle/DependencyInjection/DoctrineExtension.php

Lines 88 to 99 in 015fdd4

	protected function dbalLoad(array $config, ContainerBuilder $container)
	{
	$loader = new XmlFileLoader($container, new FileLocator(__DIR__ . '/../Resources/config'));
	$loader->load('dbal.xml');
	$chainLogger = $container->getDefinition('doctrine.dbal.logger.chain');
	$logger = new Reference('doctrine.dbal.logger');
	if (! method_exists(SQLParserUtils::class, 'getPositionalPlaceholderPositions') && method_exists(LoggerChain::class, 'addLogger')) {
	// doctrine/dbal < 2.10.0
	$chainLogger->addMethodCall('addLogger', [$logger]);
	} else {
	$chainLogger->addArgument([$logger]);
	}

[ostrolucky]

Indeed, looks like logging doesn't work unless profiling is enabled. And code you quoted looks ineffective since 2c377e2#diff-afe5f060789287a8ad428a4b5df1ad844184f496ece0297e421f6b1fc442b5a4L146. Until then, ChildDefinition was used

[ostrolucky]

@rjd22 any update? did you give up on this?

[rjd22]

@ostrolucky to be honest I have not been working on this. For Sentry we've gone a different way because we wanted access to more information. So we wrapped the connection.

Since I lost my personal need for this I don't think I will work on this anymore. So if someone else wants to pick this up it's okay to do so.

[ste93cry]

Since I believe that this improvement would be useful, I may be able to work on it in the next weeks. @ostrolucky feel free to take it if you want of course, otherwise I would leave this issue open for a bit more time. If for any reason should you start working on it, please let me know so that we don't do the work twice 😄

[ostrolucky]

Solved by #1472