Add doctrine dbal tracing support that is optional to enable

[rjd22]

This is the tracing support for dbal. This also took me half a day figuring out how to make it optional to use and enable. The problem is that there is no clear way to enable tracing because there actually is no hook.

I cheated the system by replacing the symfony DbalLogger that is often used to log queries to monolog. So enabling this will actually disable that functionality. If someone else has an better idea feel free to help out I've been hitting my head on making this work better for 6 hours straight now and could not find any better way.

To use Dbal tracing you will have to enable the following options:

doctrine:
    dbal:
        logging: true

sentry:
    tracing:
        dbal_tracing: true

Good luck!

[rjd22]

The only neat way I could think of to could fix the Class Doctrine\DBAL\Logging\SQLLogger not found. error. Is to install the suggested dependencies for the test. This way we can also unit test the DbalListener.

[ste93cry]

install the suggested dependencies for the test

This is the way 👍

I cheated the system by replacing the symfony DbalLogger that is often used to log queries to monolog

As far as I know, the Symfony Profiler works the same way by adding a custom SQL logger, which is the public-facing API nearest to where the actual query is really executed. During the past few days I also explored to hook into more low-level points like the driver or the entity manager itself so that we can get more precise tracing of different parts of Doctrine, but for an initial implementation of this feature what you did is more than enough imho

[rjd22]

@ste93cry You can indeed wrap the whole driver with composition for more complex tracing a good example of this I found in the following opentracing bundle:

https://github.com/auxmoney/OpentracingBundle-Doctrine-DBAL

[rjd22]

@ste93cry I added tests, I feel that this one is GTG. Afaik merging this will not have any impact because You need to attach it and enable it manually for it to work.

[ste93cry]

Thank you for the hard work. I did a first pass of code review, my idea is to get this PR merged before reviewing the others so that we don't unnecessarily have to change the others based on agreements we decide here on the initial implementation of things

[rjd22]

@ste93cry I applied the changes you requested.

[ste93cry]

Still a few things missing/that can be improved:

• A CHANGELOG entry with a reference to this PR
• Some tests for the compiler pass
• Some tests for the bundle's configuration when the feature is enabled (add the dbal_tracing_enabled fixture to tests/DependencyInjection/Fixtures)

[rjd22]

@ste93cry @Jean85 Okay, I'm going to be very honest here. I'm a freelance developer and doing this in my free time. I've been trough 2 review passes already and am burned out. Contributing is not easy and I've now spend 40 of my hours researching develping and making the PR's and applying review comments.

Thank you for taking the time to review this.

I might come back later for this, after I recharged. If you have the time to apply the last touches, please do so :).

[ste93cry]

First of all, I wanna thank you for the help you gave us until now

I'm a freelance developer and doing this in my free time

As you know, Sentry is a company, however most of the contributors of this package like you do this in their free time without getting paid. Myself is not getting paid for this, I do it both to contribute back to the community and because I like the product and I feel that working on it is satisfying. Even though, and I know this, often I leave a lot of comments which someone (or a lot of people) may find uninteresting and/or unuseful, I strongly want to keep the code standard and quality of this package the highest I can and this means usually going through many review and changes. Some of these changes are indeed opinionable personal preference and I will try to make my best in the future to improve on this and instead make them myself to not bother people.

If you have the time to apply the last touches, please do so :).

Sure, once I will have a bit of free time during these days I will happily do it 👍

[rjd22]

@ste93cry thank you for your understanding answer. Could you and @Jean85 make time to check if all the open comments now are all you want changed?

If so I will make one last commit that applies them.

[ste93cry]

I cannot promise that these changes will be the last, even though I think that after them we should be ok. As I replied above, if you don't have the time to work on this anymore I will anyway step up and end the work because it's indeed something we want to merge

[Jean85]

Code LGTM, great work both of you!

Only thing that I would add is a reference to the new config option.

[ste93cry]

Where would you add it? The only place I see that makes sense is the documentation on the official website

[Jean85]

I would put the default values with a comment here: https://github.com/getsentry/sentry-symfony#configuration-of-the-sdk

[ste93cry]

Are we sure we want to do that? The direction I would like to take as already done for Monolog is moving all documentation besides the install instruction to https://github.com/getsentry/sentry-docs to avoid having to maintain in one place only the docs, so adding more things in the README goes against this

[Jean85]

I'm 👍 on moving that stuff to the docs if we leave a clear reference to it. Obviously it should end up in a different PR.

[rjd22]

@Jean85 @ste93cry thank you for all the work! It's great to see this one merged. Maybe it would be best to look over the main one after this one and finish with the twig tracing:

#423

[VincentLanglet]

Hi @rjd22 Could some documentation of this option be added in the Readme ?
I don't understand what it does.