fix PDO instrumentation in PHP 8.4 #993
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Great job with the investigation and fix! |
lavarou
force-pushed
the
feat/php-8.4/fix-api-number
branch
from
508be9b to
bbe7ed0
Compare
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
from
62506bd to
2f8830a
Compare
ZNeumann
approved these changes
Dec 11, 2024
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
from
2f8830a to
dcc238f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## php84 #993 +/- ##
==========================================
+ Coverage 78.03% 78.04% +0.01%
==========================================
Files 196 196
Lines 27084 27104 +20
==========================================
+ Hits 21134 21154 +20
Misses 5950 5950
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
from
dcc238f to
9807f77
Compare
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
from
ea6cce9 to
043f6bf
Compare
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
from
e121c97 to
4387b13
Compare
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
3 times, most recently
from
3405121 to
5a40841
Compare
lavarou
commented
Dec 24, 2024
| */ | ||
|
|
||
| /*INI | ||
| ;comment=Set explain_threshold to 0 to ensure that the slow query is recorded. |
There was a problem hiding this comment.
I wanted the comments explaining these settings to be as close to the settings as possible. The reason for ;comment=... construct is because integration_runner expects key=value format for anything (including comments) in INI directive.
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
2 times, most recently
from
381d972 to
adf1f02
Compare
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
from
adf1f02 to
f5471b1
Compare
zsistla
reviewed
Dec 30, 2024
tests/integration/pdo/mysql/base-class/test_instance_reporting_1.php
Outdated
Show resolved
Hide resolved
zsistla
reviewed
Dec 30, 2024
tests/integration/pdo/mysql/base-class/test_prepared_stmt_1.php
Outdated
Show resolved
Hide resolved
mfulb
reviewed
Jan 6, 2025
tests/integration/pdo/mysql/base-class/test_instance_reporting_1.php
Outdated
Show resolved
Hide resolved
mfulb
reviewed
Jan 6, 2025
mfulb
reviewed
Jan 6, 2025
mfulb
approved these changes
Jan 7, 2025
bduranleau-nr
approved these changes
Jan 7, 2025
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
from
bd5f704 to
4484c3f
Compare
PHP 8.4 includes implementation of PDO driver specific sub-classes RFC (see https://wiki.php.net/rfc/pdo_driver_specific_subclasses). This means that when database access code uses new factory method `PDO::connect` to create PDO object, an instance of driver specific sub-class of `PDO` is returned instead of an instance of generic `PDO` class. This means that instrumentation of generic `PDO` class is not enough to provide instrumentation of datastores. Add wrappers for driver specific subclasses of `PDO` supported by the agent: `Pdo\Firebird`, `Pdo\Mysql`, `Pdo\Odbc`, `Pdo\Pgsql`, `Pdo\Sqlite`.
Back in 2015 when datastore integration tests were refactored to extract common configuration into `config.php`, setting the value of global variable `$MYSQL_DB` to the value of `MYSQL_DB` env var with help of `isset_or` was not preserved - it was left out as a TODO item. However, since mysql tests need to occasionally create tables in the database, as well as insert data into those tables, `$MYSQL_USER` needs permissions to do that. When mysqldb test service is provisioned, permissions are granted automatically for the database name stored in `$MYSQL_DB` env var. Therefore `$MYSQL_DB` must be set to the value of `MYSQL_DB` env var in order for create table to work. However, this means that all queries about MySQL's tables metadata stored in `information_schema` database need to specify full table path, i.e.: `SELECT * from information_schema.tables` rather than just `SELECT * from tables`.
Test output depends on PHP version and backing database - value for id column can be returned as int or string. Instead of splitting the test by PHP version and backing database normalize test output to always return value of id column as int.
Test that agent creates datastore metrics when `PDO::query` and `PDO::execute` are provided connection object created using either `PDO::connect` factory method or PDO's specialized subclass constructor.
Test different variants of calls to PDO::query with various types of conn object for mysql, pgsql and sqlite databases.
Test that agent creates datastore metrics, datastore spans and slow sql traces when `PDO::prepare` is provided connection object created using all supported methods: either `PDO::connect` factory method or PDO's base class or specialized subclass constructor.
Use legacy name for SQLite's schema table (sqlite_master) for compatibility with older PHPs (7.2 and 7.3).
Test that agent creates datastore instance metrics, and includes datastore instance attributes in datastore spans and slow sql traces when connection object is created using all supported methods: either `PDO::connect` factory method or PDO's base class or specialized subclass constructor.
Explain how pdo/*.inc test files are used by driver specific tests.
Make test purpose clearer by using more expressive test file names that reflect test purpose.
Make test purpose clearer by using more expressive test file names that reflect test purpose.
Make test purpose clearer by using more expressive test file names that reflect test purpose.
lavarou
force-pushed
the
feat/php-8.4/fix-pdo-instrumentation
branch
from
4484c3f to
8cca073
Compare
zsistla
approved these changes
Jan 7, 2025
ZNeumann
pushed a commit
that referenced
this pull request
Jan 7, 2025
PHP 8.4 includes implementation of PDO driver specific sub-classes RFC (see https://wiki.php.net/rfc/pdo_driver_specific_subclasses). This means that when database access code uses new factory method `PDO::connect` to create PDO object, an instance of driver specific sub-class of `PDO` is returned instead of an instance of generic `PDO` class. This means that instrumentation of generic `PDO` class is not enough to provide instrumentation of datastores. Add wrappers for driver specific subclasses of `PDO` supported by the agent: `Pdo\Firebird`, `Pdo\Mysql`, `Pdo\Odbc`, `Pdo\Pgsql`, `Pdo\Sqlite`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PHP 8.4 includes implementation of PDO driver specific sub-classes RFC (see https://wiki.php.net/rfc/pdo_driver_specific_subclasses). This means that when database access code uses new factory method
PDO::connectto create PDO object, an instance of driver specific sub-class ofPDOis returned instead of an instance of genericPDOclass. This means that instrumentation of genericPDOclass is not enough to provide instrumentation of datastores. Add wrappers for driver specific subclasses ofPDOsupported by the agent:Pdo\Firebird,Pdo\Mysql,Pdo\Odbc,Pdo\Pgsql,Pdo\Sqlite.