Pass the exception into the stopQuery.

[jcchavezs]

Feature Request

The SQLLogger::stopQuery method does not receive any information about the output of the query.

Q	A
New Feature	yes
RFC	yes
BC Break	no

Summary

Problem 1: When calling startQuery, stopQuery is always being called.

stopQuery is not being called when there is an exception thrown in Connection::executeQuery, Connection::query or Connection::exec.

This makes it impossible to finish any sort of span or trace once it is initialized in startQuery.

My proposal is that we add the stopQuery in the catch, same as in Statement::execute.

Problem 2: When calling stopQuery after an error, the error is not passed to the stopQuery.

Some more sophisticated logging or tracing require to report an error when it happens. The current SQLLogger implementation does not support that as stopQuery does not receive any parameter.

Existing code in Statement does:

...
 try {
            $stmt = $this->stmt->execute($params);
        } catch (\Exception $ex) {
            if ($logger) {
                $logger->stopQuery();
            }
            throw DBALException::driverExceptionDuringQuery(
                $this->conn->getDriver(),
                $ex,
                $this->sql,
                $this->conn->resolveParams($this->params, $this->types)
            );
        }

        if ($logger) {
            $logger->stopQuery();
        }

So from the logger point of view the query finished but it has no more information about it.

Since SQLLogger is an interface, adding support for the parameter will break existing implementations and will be hard to release. I propose we don't change the interface and pass the exception as parameter, delegating the responsibility for clients to deal with that information. This can be released in a minor version in semver.

...
 try {
            $stmt = $this->stmt->execute($params);
        } catch (\Exception $ex) {
            if ($logger) {
                // HERE I PASS THE EXCEPTION
                $logger->stopQuery($ex);
            }
            throw DBALException::driverExceptionDuringQuery(
                $this->conn->getDriver(),
                $ex,
                $this->sql,
                $this->conn->resolveParams($this->params, $this->types)
            );
        }

        if ($logger) {
            $logger->stopQuery();
        }

In the future, we can change the interface of SQLLogger accepting the exception as parameter but that will be a breaking change and will require a major version.

Another option is to pass stopQuery(true) instead of the exception itself.

Ping: @beberlei @Ocramius @deeky666. I can open a PR if you feel this can be done.

PS for @beberlei you might want to see this problem with even more context in this issue: jcchavezs/zipkin-instrumentation-doctrine#1

[jcchavezs]

Ping @morozov @AlessandroMinoccheri

[morozov]

@jcchavezs I like the idea. We do a similar thing in our project by using a custom connection wrapper class. There are a few things to keep in mind though:

1. We cannot change the stopQuery() signature in 2.x since it'd be a breaking change. It could be temporarily worked around using func_get_args().
2. The implementation probably should use the finaly block to avoid code duplication.

Please file a pull request so that we could proceed.

[jcchavezs]

Hi Sergei, Glad to see this is feasible. @beberlei suggested we instead introduce a new interface (let say SQLLoggerWithErrors) extending from SQLLogger that includes the method `stopQueryWithError` and we pass the exception there. His idea is to check if the logger is `instanceof` SQLLoggerWithErrors or SQLLogger and then decide what method to use. The advantage of this approach is that we have a meaningful method in logger for errors. Otherwise, the initial proposal is still working for me and this new interface can be added in v2. Could you please give us some input so I can update my [PR](#3174)? Thanks!

[morozov]

@jcchavezs personally, I think adding a new method is overkill given that there's a backward-compatible way of introducing the feature without adding a method.

What I'm concerned about more, is that our logging API still doesn't comply with the Open/closed principle. Every time we want to add some details to logging, we need to change the API.

There's an ongoing effort #3156 where we're trying to introduce the PSR-3 adapter but the Open/closed problem is still unsolved there. Maybe we could address both issues at once without changing the SQLLogger API?

Could you please give us some input so I can update my PR?

Have you filed one? I cannot see any.

[jcchavezs]

Sorry, in the end I did not open it: #3174

There's an ongoing effort #3156 where we're trying to introduce the PSR-3 adapter but the Open/closed problem is still unsolved there. Maybe we could address both issues at once without changing the SQLLogger API?

I don't think the PSR3 introduction will solve the problem. To me, the problem with logger is that it was good enough by the time it was designed but now it is required to do more sophisticated instrumentation and changes are required: more information passed to the logger, call stopQuery when the query fails, etc.

I would go for merging my PR and release a minor, what do you think?

[bkdotcom]

in addition to the exception, it would be useful if stopQuery also receives a $rowCount param (number of affected/returned rows)

[morozov]

Closing in favor of #4941.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.