Allow the database query log to be dispatched as log events

I have fix CS error. Please review it

I wonder if this is something that meets the 80% use-case, or is it better kept as a patch that someone could apply for when they need additional logging of database logs, but otherwise not run in production (as in itself it adds a performance overhead)

+++ b/core/lib/Drupal/Core/Database/Log.php
@@ -31,6 +37,13 @@ class Log {
+  protected $eventDispatcher = [];

@@ -57,11 +70,15 @@ public function __construct($key = 'default') {
+    $this->eventDispatcher[$logging_key] = $event_dispatcher;

Do we need an array of event dispatchers? wouldn't there ever only be the one service from core?

It's a nice idea.

I think the first hurdle would be to agree that we add an event system to the Database API. It would make a lot of sense to me in 2022.

Then, IMHO if we move in this direction it would be better to completely swap the current logging facility with a series of events (fired at statement execution start, at statement execution end, etc ...) dispatched to the container's event_dispatcher, and convert the current logging to an event subscriber instead (i.e. for it to become one of the possible ways to trace SQL execution).

Certainly performance impact should be assessed, this would be running on low level code; in my understanding SQL logging was meant so far more or less for debugging only, an use case like #6 would open up for it to be used in real time monitoring of prod sites.

I'm working on a concept for #7.

In the MR, some changes to implement events on the Statement objects in place of the logger, i.e. on very low level. It's a concept, if that seems the right direction then we'll have to make it work with changes and deprecations on higher levels.

I like the idea of using events for logging and it is the Symfony way of doing this.
What for me is very important is that when logging is of, Drupal does not get a performance hit.
The MR from @mondrake is to me the solution we should go for.
I did a review on the MR and it looks good to me.
We need testing for this.

Thanks for review @daffie.

Re. tests I was also wondering... we already have LogTest that covers logging, and it shows that the changes are BC since they end up with the same logging results. I wouldn't add tests that prove that event dispatching and subscribing works... they're not for here. Input welcome.

added a draft placeholder CR and adjusted deprecations

@mondrake Can we add testing for the new method from Connection class and the method Log::logFromEvent(). The problem is that the deprecated tests from LogTest will be removed in D11.

Added unit tests.

Would we add events for transaction start/commit/rollback in this issue or a followup?

In a follow-up, please. We need to land a first instalment of an event system connected to the database API, first. And IMHO we the smaller its scope, the higher the chances.

All code changes look good to me.
I have updated the IS and the CR.
Testing has been added.
For me it is RTBC.

Thanks for filling in the CR, @daffie. I made a few changes, trying to make clear that statement exection events are not strictly bound to the logging (i.e. you can have events dispatched even if the logging is not enabled).

Added a review to the MR.

Thank you @alexpott. I will be on this; I also think we could do something more, too.

Addressed @alexpott's review points in last commit.

Now planning to:

• use S6.2 syntax for subscribing service
• split event in two, one for execution start one and one for execution end - and use their FQCN as the event name instead - so to get rid of DatabaseEvents constants. In S6.2+, the event dispatch method requires an event object, and a literal name is optional: if not passed in, the FQCN is used https://github.com/symfony/symfony/blob/6.3/src/Symfony/Contracts/EventD...
• make Connection::enableStatementEvents() and friends generic so that if other events are added later, we won't not need additional methods

Use FQCN as the service name - precedent: #3327856-29: Performance regression introduced by container serialization solution

Done #25.

Appreciated help to list what tests are missing at this point, as well as updates to IS and CR.

Added some Unit and Kernel tests for the new API. Also, Connection::dispatchEvent() now throws an exception if the container is not initialized yet.

@mondrake: I have just one question: Why is the StatementExecutionStartEvent not dispatched in core/lib/Drupal/Core/Database/StatementPrefetch.php and core/lib/Drupal/Core/Database/StatementWrapper.php and StatementExecutionEndEvent is dispatched? Ok, my bad. I missed the part $this->connection->dispatchEvent($startEvent);.
The rest of the MR looks great.

All remarks of @alexpott has been addressed or answered.
The CR has been updated.
Back to RTBC.

Self reviewing, I think we could try using spl_object_id to relate independent event objects to the same statement object. Would be relevant for multi-thread or multi-connection monitoring.

Adding the ID of the StatementInterface object as returned by spl_object_id() is to me a good idea.
There is testing added for the extra functionality.
Back to RTBC.

#35 nice!

#35 looks exciting! Adding tags for release notes + highlights. Haven't reviewed this enough to be able to commit it yet. What I did review didn't raise any massive flags - don't see a way around the \Drupal:: calls, and making it conditional on events being enabled at all is a good plan.

Rerolled.

This will also be useful for #638078: Automated performance testing for core, not that it's anywhere close but I've started working on it again after a decade.

Reviewed again and couldn't find anything to complain about. Going to untag for release notes because there's nothing really for site owners here, but we can leave it tagged for highlights as an API improvement - can always drop it later.

Committed d7313cc and pushed to 10.1.x. Thanks!

Created #3348581: Create span for database execution for OpenTelemetry.

Filed #3348590: Add transaction-related events to the Database API for follow-up.