search

open-telemetry / opentelemetry-php

The OpenTelemetry PHP Library
https://opentelemetry.io/docs/instrumentation/php/
Apache License 2.0
752 stars 186 forks source link

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

Open DominicDetta opened 1 month ago

DominicDetta commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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