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

[opentelemetry-php-contrib] Add support for Doctrine library #1393

Open
DominicDetta opened this issue Sep 30, 2024 · 6 comments
Open

[opentelemetry-php-contrib] Add support for Doctrine library #1393

DominicDetta opened this issue Sep 30, 2024 · 6 comments

Comments

@DominicDetta
Copy link

Hi,

No auto-instrumentation for Doctrine library exists at the moment and I was wondering whether there is a planned integration of this instrumentation as a separated package. In the meantime I'm currently using my solution inspired by the existing PDO instrumentation.

This instrumentation hooks on the classes \Doctrine\DBAL\Driver and \Doctrine\DBAL\Driver\Connection
and has been tested using Microsoft SQL Server.

Are you in favor of including my proposal as a pull request?

Any comments or enhancements are greatly appreciated.

@brettmc
Copy link
Collaborator

brettmc commented Sep 30, 2024

Hello,
I think that if it provides functionality that the existing PDO auto-instrumentation cannot, then sure we'd accept it. I understand that doctrine can use PDO or other drivers internally, so I guess this covers the non-PDO use-case?
The code in your solution looks pretty sensible, I'd want to see some tests and static analysis to bring it up to the standard of other auto-instrumentations.

@DominicDetta
Copy link
Author

DominicDetta commented Sep 30, 2024

You perfectly nailed the situation. We are using non-PDO driver in our projects.
Is there a guideline on how to implement tests and static analysis?

@brettmc
Copy link
Collaborator

brettmc commented Sep 30, 2024

Is there a guideline on how to implement tests and static analysis?

Copy what PDO auto-instrumentation does :)
Usually when starting a new auto-instrumentation, I just copy the one that's most like it, and start from there.

@DominicDetta
Copy link
Author

Thanks I will try to do that and when I'm ready we can discuss how to proceed to create a pull request.
Do you have a docker-compose.yml of example to create the environment for test and analysis?
If not I will try to create it to run the tests and the analysis

@brettmc
Copy link
Collaborator

brettmc commented Sep 30, 2024

Everything you need should already be in core of the contrib repo. Once you've created your directory (src/Instrumentation/Doctrine) and copied in source etc , you can run PHP_VERSION=8.3 PROJECT=Instrumentation/Doctrine make all which is going to attempt to run all static analysis tools and tests like other instrumentations do.
Hopefully you can write some integration tests against sqlite, since it's already there, but if necessary you can install something else (see docker/Dockerfile)

@DominicDetta
Copy link
Author

Ok I created the PR I'm waiting my company manager to accept the CLA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants