Add query backtrace logger

[ottaviano]

Q	A
Type	feature
BC Break	no
Fixed issues	~

Summary

inspired by doctrine-stats I think it'll be nice to log backtrace of each query.

use case with doctrine-bundle:

doctrine-bundle pull request

[Ocramius]

I'd avoid coupling the BacktraceLogger to the existing DebugStack one, if possible

[stof]

well, the whole point here is to add the backtrace into the data stored for each query. Not coupling it would defeat the point.

[greg0ire]

Links to previous attempts: #3390 , #535 , #719 cc @pierredup @carnage @dmecke . People want that feature badly, it would be great to have a list of requirements for its implementation somewhere.

1. It should not keep the stack trace into memory, but process it added backtrace to query logging #535 (comment) EDIT: looks like @Ocramius disagrees with this (Added TraceLogger to get backtrace for executed queries #719 (comment))
2. It should avoid coupling with DebugStack
3. logs should be serializable added backtrace to query logging #535 (comment)
4. it should be implemented as/with a PR3 adapter Added TraceLogger to get backtrace for executed queries #719 (comment) (this one probably needs some clarification)

[ottaviano]

Thank you @greg0ire for this summary 👍

1. debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS) ignores all arguments and doesn't keep any object in memory. Am I wrong, @stof?

2. honesty I don't really see how to do it :) with DebugStack public properties

3. cf point n1, as the backtrace contains only scalar values, it's completely serializable

4. I'm not sure I understand this point

[morozov]

Regarding #‌4 and #719 (comment), after trying to implement a PSR-3 adapter (#3156) and later reading an article on Domain-Oriented Observability, I realized that our logger is not really a logger. It's a probe. It allows plugging in loggers, metrics and their combinations.

I'm still not convinced that we should be adding any new implementations to the stack. As long as the interface allows for extensibility, the needed functionality can be part of the application which needs it or a separate package.

[stof]

@morozov well, if we use that feature in DoctrineBundle itself, it means that the implementation would be maintained by Doctrine anyway. Then, having it in DBAL itself means that other framework integrations can also use it directly instead of having to copy it from DoctrineBundle.

[greg0ire]

honesty I don't really see how to do it :) with DebugStack public properties

@Ocramius can you maybe help with this? I don't understand what you are suggesting either (composition, copy/paste, something else entirely, no suggestion?)

[Ocramius]

I'm suggesting c&p and avoiding the inheritance

…

On Sat, 27 Apr 2019, 15:23 Grégoire Paris, ***@***.***> wrote: honesty I don't really see how to do it :) with DebugStack public properties @Ocramius <https://github.com/Ocramius> can you maybe help with this? I don't understand what you are suggesting either (composition, copy/paste, something else entirely, no suggestion?) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3513 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABFVEDMYH35NCTJA4DDYS3PSRHWHANCNFSM4HFZHDZA> .

[ostrolucky]

LGTM. inheritance > c&p

[Ocramius]

Disagree massively: adding inheritance reduces our ability to introduce international changes. If inheritance is not also introducing added "meaning", it is absolutely NEVER to be used as a code reuse mechanism.

[greg0ire]

If this goes towards copy and paste, would you say public properties should be kept, or should encapsulation be used this time?

[Ocramius]

Considering how widespread the library is, I'd at least use a safer API around here, because properties are invariants, therefore they expose a BC surface both for reads and writes.

In other words: yes, we'd need some sort of accessors/reset methods.

[Ocramius]

Thinking more about API and state, @morozov got to the right thought at #3513 (comment)

The logger being the primary reason why doctrine has memleaks in symfony-related setuos (think profiler), maybe it is a good idea if we didn't even have a logger ourselves at all, and instead only provided the hook mechanism?

[greg0ire]

Finally got around to reading the article linked by @morozov , that was really a great read! Just to be clear, are you saying that the DebugStack class should be deprecated, and that the code in this PR should be moved to the doctrine bundle, making that part of the code a bit more Symfony-owned (although it would still be in the Doctrine org)?

[morozov]

Just to be clear, are you saying that the DebugStack class should be deprecated, and that the code in this PR should be moved to the doctrine bundle, making that part of the code a bit more Symfony-owned (although it would still be in the Doctrine org)?

I didn't think of it in these details but sounds right. I believe everyone would benefit from having code closer to the owner in order to avoid unproductive discussions and wasted effort.

I do believe that neither DebugStack nor backtrace logger belong to the DBAL because they solve the application developer experience problems, not the DBAL ones. In a longer run, a good DX may involve adding some more details to the bracktrace which would again require changes in DBAL which would trigger the same round of discussions.

On the other hand, in my own experience of developing DBAL, having DebugStack enabled in the test suite doesn't add any value and just creates noise. Unlike an application, where failure is likely to be caused by a sequence of SQL queries, in the DBAL test suite, a test will fail because a specific query failed and the list of previously executed SQL queries is irrelevant.

[greg0ire]

Looks like we have reached an agreement :) @ottaviano, I think you can close this PR and make your bundle PR independent from it by adding a new SqlLogger implementation to it.

[Ocramius]

Closing here as per #3513 (comment)

[alcaeus]

For the record, while I agree with the semantics surrounding DebugStack, its very unfortunate public API (public properties, no clear structure for logged queries, etc.) I don't agree that this shouldn't be in DBAL. If we move this into DoctrineBundle, we'll also have to have it in the corresponding module for Zend Framework or any other framework integrations. People not using a framework integration will have to duplicate and maintain the code for this logic themselves, which is even more unfortunate than having it here.

I'm all for cleaning up the API and dropping the current SQLLogger interface in 3.0, but I don't think not adding new functionality because the current API is not as good as we'd like to have it is unfortunate.

Regarding decoupling from DebugStack, this is not possible unfortunately. Since the SQLLogger interface does not specify how something is logged/can be retrieved from the logger, any packages using such instrumentation have to type hint against a concrete implementation. In the case of the Symfony Doctrine bridge, the data collectors that make use of SQL logging are coded against DebugStack, meaning that any additional loggers would always have to extend the DebugStack class.

[morozov]

If we move this into DoctrineBundle, we'll also have to have it in the corresponding module for Zend Framework or any other framework integrations.

Which is natural. As long as observability logic can be framework-specific, it's fine to have separate implementations in each bundle/module.

People not using a framework integration will have to duplicate and maintain the code for this logic themselves, which is even more unfortunate than having it here.

Which is also fine. People may have different needs than frameworks. Besides that, the current implementation of DebugStack primitive but not extensible. It won't be a maintenance burden for the end developer but is for us.

Regarding decoupling from DebugStack, this is not possible unfortunately. Since the SQLLogger interface does not specify how something is logged/can be retrieved from the logger, any packages using such instrumentation have to type hint against a concrete implementation. In the case of the Symfony Doctrine bridge, the data collectors that make use of SQL logging are coded against DebugStack, meaning that any additional loggers would always have to extend the DebugStack class.

Frameworks may define their own interfaces for data retrieval. An instrumentation component can implement both SQLLogger to be injected into DBAL and the data retrieval interface to be integrated with the framework.