Skip to content
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

Fix missing instrumentation of the Statement::execute() method of Doctrine DBAL #548

Merged

Conversation

ste93cry
Copy link
Collaborator

Fixes #544: thanks to @jokaorgua I found out that the Doctrine\DBAL\Driver\Statement::execute() method has to be instrumented as well for the tracing of SQL queries to work properly in case a prepared statement is used, e.g. when calling the Doctrine\DBAL\Connection::fetch*() methods and passing some query params to them. I voluntarily avoided a child span for the TracingStatement::execute() method because you may prepare a query and run it later, so it doesn't make sense to have it nested into the one of the TracingDriverConnection::prepare() method as it would have happened by creating it inside the callback of the TracingDriverConnection::traceFunction() function

@ste93cry ste93cry added this to the 4.2 milestone Aug 20, 2021
@ste93cry ste93cry requested a review from Jean85 August 20, 2021 19:59
@ste93cry ste93cry force-pushed the fix/missing-instrumentation-of-statement-execute-method branch 3 times, most recently from de8cac6 to 0b3318c Compare August 22, 2021 16:38
@ste93cry ste93cry force-pushed the fix/missing-instrumentation-of-statement-execute-method branch from 0b3318c to d9fb860 Compare August 22, 2021 16:40
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit complicated, but we don't have any other way around it...

@jokaorgua
Copy link

@ste93cry could you please fix the phpstan errors? seems to me these errors hold the pull request to be merged and a new version released

@Jean85
Copy link
Collaborator

Jean85 commented Aug 26, 2021

@jokaorgua we're still having issues in solving this. PHPStan doesn't seem to be able to overcome the fact that we're conditionally declaring a class, so it stumbles on the incompatible interface.

We haven't still found a way around it.

@ste93cry
Copy link
Collaborator Author

That's right, but the solution to solve those errors is to ignore analysis for entire files because PHPStan crash badly otherwise, which imho it shouldn't. I'm investigating if there is any solution or if reporting the issue upstream would be acceptable

@Jean85
Copy link
Collaborator

Jean85 commented Aug 27, 2021

Finally, it's green!!

@ste93cry
Copy link
Collaborator Author

@jokaorgua before we get this released, likely next week, can you please try it out and see if everything works as expected?

@jokaorgua
Copy link

@jokaorgua before we get this released, likely next week, can you please try it out and see if everything works as expected?

will do. once checked will make a comment here

@jokaorgua
Copy link

jokaorgua commented Aug 27, 2021

@ste93cry everything is ok. I see sql.stmt.execute row

@ste93cry ste93cry merged commit 8ea6769 into master Aug 28, 2021
@ste93cry ste93cry deleted the fix/missing-instrumentation-of-statement-execute-method branch August 28, 2021 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepared queries when using Doctrine DBAL are not instrumented
3 participants