-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add doctrine dbal tracing support that is optional to enable #426
Add doctrine dbal tracing support that is optional to enable #426
Conversation
a4ed8a4
to
390cace
Compare
The only neat way I could think of to could fix the |
This is the way 👍
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 |
@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: |
ef52228
to
6aa45ad
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/DependencyInjection/Compiler/ConnectDbalQueryListenerPass.php
Outdated
Show resolved
Hide resolved
src/DependencyInjection/Compiler/ConnectDbalQueryListenerPass.php
Outdated
Show resolved
Hide resolved
a6d7382
to
e7be267
Compare
@ste93cry I applied the changes you requested. |
e7be267
to
c9b673d
Compare
src/DependencyInjection/Compiler/ConnectDbalQueryListenerPass.php
Outdated
Show resolved
Hide resolved
src/DependencyInjection/Compiler/ConnectDbalQueryListenerPass.php
Outdated
Show resolved
Hide resolved
Still a few things missing/that can be improved:
|
@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 :). |
First of all, I wanna thank you for the help you gave us until now
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.
Sure, once I will have a bit of free time during these days I will happily do it 👍 |
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 |
103b410
to
29543f8
Compare
6a542a4
to
27f602b
Compare
27f602b
to
6d505b3
Compare
dbaa3dd
to
368aa38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, great work both of you!
Only thing that I would add is a reference to the new config option.
Where would you add it? The only place I see that makes sense is the documentation on the official website |
I would put the default values with a comment here: https://github.com/getsentry/sentry-symfony#configuration-of-the-sdk |
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 |
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. |
Hi @rjd22 Could some documentation of this option be added in the Readme ? |
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:
Good luck!