feat: sqlcommenter for PDO, MySqli, PostgreSql (pdo_mysql, pdo_pgsql) #442

jerrytfleung · 2025-09-17T01:31:04Z

It is a followup / replacement of #406

Description:

Currently, there isn't any sqlcommenter for the database instrumentation library that allows customers to correlate their span with the database query. This PR adds the implementation for sqlcommenter according to open-telemetry/semantic-conventions#2495

Decoupled sqlcommenter logic to a separate package, proposed open-telemetry/opentelemetry-sqlcommenter
Updated PDO (pdo_mysql, pdo_pgsql) & MySqli & PostgreSql instrumentation to add sqlcommenter feature

Testing:

Unit Testing:

Wrote unit tests for sql comment for correctness and they're all passing.

Manual Testing:

Once requiring open-telemetry/opentelemetry-sqlcommenter, the sqlcommenter feature is enabled.

The following extracted the span attributes.
PDO

"attributes": {
            "code.function.name": "PDO::query",
            "db.query.text": "SELECT code, name, region from country LIMIT 1\/*traceparent='00-03d0617d3d102d4db6d345e1e6a10714-660de8b2725490e0-01'*\/;",
            "server.address": "44.203.164.2",
            "db.namespace": "world-db",
            "db.system.name": "postgresql"
        }

MySqli

"attributes": {
            "code.function.name": "mysqli::query",
            "db.query.text": "SELECT Code, Name, Region from country LIMIT 1\/*traceparent='00-79c1c1c373c34772ac73ff8575c3ee4f-0656f3d396b84beb-01'*\/;",
            "db.operation.name": "SELECT",
            "db.system.name": "mysql",
            "server.address": "34.239.101.164",
            "server.port": "3306",
            "db.namespace": "world"
        }

PostgreSql

"attributes": {
            "code.function.name": "PDO::query",
            "db.query.text": "SELECT code, name, region from country LIMIT 1\/*traceparent='00-0a7a38884b2dfc3b514376fc7bf4033a-81c6031fe6a0e18b-01'*\/;",
            "server.address": "44.203.164.2",
            "db.namespace": "world-db",
            "db.system.name": "postgresql"
        }

codecov · 2025-09-17T01:32:40Z

Codecov Report

❌ Patch coverage is 65.45455% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.52%. Comparing base (1e2fa80) to head (2b2be6c).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Instrumentation/PDO/src/PDOInstrumentation.php	25.00%	30 Missing ⚠️
...strumentation/MySqli/src/MySqliInstrumentation.php	58.82%	14 Missing ⚠️
...ation/PostgreSql/src/PostgreSqlInstrumentation.php	54.54%	10 Missing ⚠️
src/SqlCommenter/src/ContextPropagatorFactory.php	93.33%	2 Missing ⚠️
src/SqlCommenter/src/SqlCommenter.php	96.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #442      +/-   ##
============================================
+ Coverage     81.17%   82.52%   +1.35%     
- Complexity     1454     2414     +960     
============================================
  Files           102      167      +65     
  Lines          5439     9711    +4272     
============================================
+ Hits           4415     8014    +3599     
- Misses         1024     1697     +673

Flag	Coverage Δ
Aws	`93.41% <ø> (ø)`
Context/Swoole	`0.00% <ø> (ø)`
Exporter/Instana	`49.42% <ø> (ø)`
Instrumentation/AwsSdk	`81.13% <ø> (ø)`
Instrumentation/CakePHP	`20.40% <ø> (ø)`
Instrumentation/CodeIgniter	`78.99% <ø> (+5.00%)`	⬆️
Instrumentation/Curl	`87.66% <ø> (ø)`
Instrumentation/Doctrine	`92.92% <ø> (ø)`
Instrumentation/ExtAmqp	`88.48% <ø> (?)`
Instrumentation/ExtRdKafka	`86.11% <ø> (?)`
Instrumentation/Guzzle	`75.58% <ø> (ø)`
Instrumentation/HttpAsyncClient	`78.04% <ø> (ø)`
Instrumentation/IO	`0.00% <ø> (-70.69%)`	⬇️
Instrumentation/Laravel	`71.66% <ø> (?)`
Instrumentation/MongoDB	`74.28% <ø> (?)`
Instrumentation/MySqli	`93.94% <58.82%> (?)`
Instrumentation/OpenAIPHP	`87.21% <ø> (ø)`
Instrumentation/PDO	`86.24% <25.00%> (-7.97%)`	⬇️
Instrumentation/PostgreSql	`91.89% <54.54%> (?)`
Instrumentation/Psr14	`76.47% <ø> (ø)`
Instrumentation/Psr15	`89.15% <ø> (ø)`
Instrumentation/Psr16	`97.50% <ø> (?)`
Instrumentation/Psr18	`77.46% <ø> (ø)`
Instrumentation/Psr3	`67.01% <ø> (ø)`
Instrumentation/Psr6	`97.61% <ø> (ø)`
Instrumentation/ReactPHP	`99.45% <ø> (ø)`
Instrumentation/Session	`94.52% <ø> (ø)`
Instrumentation/Slim	`84.28% <ø> (-1.83%)`	⬇️
Instrumentation/Symfony	`84.03% <ø> (?)`
Instrumentation/Yii	`83.05% <ø> (+5.18%)`	⬆️
Logs/Monolog	`100.00% <ø> (ø)`
Propagation/CloudTrace	`89.77% <ø> (ø)`
Propagation/Instana	`98.11% <ø> (ø)`
Propagation/ServerTiming	`94.73% <ø> (-5.27%)`	⬇️
Propagation/TraceResponse	`94.73% <ø> (-5.27%)`	⬇️
ResourceDetectors/Azure	`91.66% <ø> (ø)`
ResourceDetectors/Container	`93.02% <ø> (ø)`
ResourceDetectors/DigitalOcean	`100.00% <ø> (ø)`
Sampler/RuleBased	`33.51% <ø> (ø)`
Sampler/Xray	`78.23% <ø> (ø)`
Shims/OpenTracing	`92.45% <ø> (ø)`
SqlCommenter	`95.65% <95.65%> (?)`
Symfony	`87.81% <ø> (?)`
Utils/Test	`87.53% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/SqlCommenter/src/Utils.php	`100.00% <100.00%> (ø)`
src/SqlCommenter/src/SqlCommenter.php	`96.00% <96.00%> (ø)`
src/SqlCommenter/src/ContextPropagatorFactory.php	`93.33% <93.33%> (ø)`
...ation/PostgreSql/src/PostgreSqlInstrumentation.php	`93.95% <54.54%> (ø)`
...strumentation/MySqli/src/MySqliInstrumentation.php	`94.40% <58.82%> (ø)`
src/Instrumentation/PDO/src/PDOInstrumentation.php	`83.77% <25.00%> (-10.50%)`	⬇️

... and 69 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e2fa80...2b2be6c. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

brettmc · 2025-09-17T23:27:32Z

src/SqlCommenter/src/ContextPropagatorFactory.php

+{
+    use LogsMessagesTrait;
+
+    public function create(): ?TextMapPropagatorInterface


I'm not understanding the point of this (actually, the entire src/ContextPropagator)... isn't that what open-telemetry/opentelemetry-php#1712 does?

They are not the same.

The response propagator in open-telemetry/opentelemetry-php#1712 supports propagating context in the http response.
(e.g. Propagate server-timing and traceresponse in the http response of slim instrumentation).

< HTTP/1.1 200 OK < Host: localhost:8080 < Date: Thu, 11 Sep 2025 23:28:27 GMT < Connection: close < X-Powered-By: PHP/8.3.6 < server-timing: traceparent;desc=00-73dbf0f78c24b08faf273fee7ae5d77b-76c077b8629b08d1-01 < traceresponse: 00-73dbf0f78c24b08faf273fee7ae5d77b-76c077b8629b08d1-01 < Content-type: text/html; charset=UTF-8

The context propagator in this PR works quite similar to the propagator OTEL_PROPAGATORS in the SDK. However, they are not the same.
OTEL_PROPAGATORS=baggage,tracecontext propagates baggage and tracecontext from one service to another service.
The context propagator propagates context from one service to the database (via sqlcommenter & query statement). The spec mentioned the instrumentation should allow user to pass a propagator to overwrite the global propagator. This context propagator maintains a list of propagator to serve that purpose.

e.g.
OTEL_EXPERIMENTAL_RESPONSE_PROPAGATORS=servertiming,traceresponse OTEL_PROPAGATORS=b3,baggage,tracecontext OTEL_PHP_INSTRUMENTATION_CONTEXT_PROPAGATORS=tracecontext means

Propagating server-timing & traceresponse in http instrumentation library (e.g. slim...)

Propagating b3, baggage & tracecontext across service

Propagating tracecontext to database

OTEL_EXPERIMENTAL_RESPONSE_PROPAGATORS=servertiming,traceresponse OTEL_PROPAGATORS=b3,baggage,tracecontext means

Propagating server-timing & traceresponse in http instrumentation library (e.g. slim...)

Propagating b3, baggage & tracecontext across service and database

Let me know if it is making sense to you.

In the previous PR #406 ContextPropagator was under the PDO Instrumentation and thus naming made some sense. If we move it out like proposed in this PR, it should be renamed to something like SqlCommenterContext.... Also, any reason this class can't just be part of the SqlCommenter i.e under the proposed src/SqlCommenter directory?

Beside sqlcommenter, there are other ways to propagate the context to database. e.g. https://opentelemetry.io/docs/specs/semconv/database/sql-server/#set-context_info & open-telemetry/semantic-conventions#2610

So I think it would be better to decouple the context propagator logic.

SET_CONTEXT_INFO and V$SESSION.ACTION work pretty differently from SQL Commenting and are prescriptive in the value type/size that can be injected, meaning i don't think they'd take a composite list of propagators. The example implementations i can find so far do not introduce a generic "database context propagator" concept:

[Instrumentation.SqlClient] Introducing context propagation to SQL Server opentelemetry-dotnet-contrib#2709

[jdbc] Oracle database context propagation signalfx/splunk-otel-java#2405

Or maybe i misunderstand your intention with this ContextPropagator package--how do you envision using it for SET_CONTEXT_INFO / V$SESSION.ACTION propagation?

I envision ContextPropagator acts as the source of the database context and put the information into a list, and SqlCommenter / SET_CONTEXT_INFO / V$SESSION.ACTION propagation can retrieve the required information from the list and propagate.

... mentioned the instrumentation should allow user to pass a propagator to overwrite the global propagator

So can we describe this component as a "per-service context propagator override", and give it a more meaningful name? (It's too similar to the core context propagation concept, IMO, and will cause confusion).
The reason I call out "per-service" (or per system/component/something), is to allow for a scenario where an application connects to multiple databases, and wants to propagate context to each of them in a different way. That doesn't seem unreasonable.

Yes, "per-service / per-system / per-component / something context propagator override" seems reasonable to me.

To clarify more and make sure we are on the same page.
Says we have an application using PDO to connect multiple databases

MySqli (pdo_mysql)

PostgreSQL (pdo_pgsql)

Oracle Database (pdo_oci)

SQL Server (pdo_sqlsrv)

IBM DB2 (pdo_ibm)

Firebird (pdo_firebird)

Ways of context propagation

As all above database supports C-style comments, the context could be propagated via sqlcommenter
e.g.

SELECT * FROM songs /*traceparent='00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01',tracestate='congo%3Dt61rcWkgMzE%2Crojo%3D00f067aa0ba902b7'*/

SQL Server could use SET CONTEXT_INFO way to propagate traceparent (55 bytes) only (128 bytes length limitation)
e.g.

DECLARE @traceparent varbinary(55); SET @traceparent = CAST('00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01' AS varbinary(55)); SET CONTEXT_INFO @traceparent;

Then run the query
4. Oracle database opts for V$SESSION.ACTION way to propagate traceparent
e.g.

BEGIN DBMS_APPLICATION_INFO.SET_ACTION('00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01'); END;

Then run the query

Questions

Does it mean the "context propagator overrides" in this case should be per-component?
e.g. I tried to mock up the names for illustrations.
OTEL_DATABASE_CONTEXT_PROPAGATORS_FOR_SQLCOMMENTER=tracecontext
OTEL_DATABASE_CONTEXT_PROPAGATORS_FOR_SET_CONTEXT_INFO=tracecontext
OTEL_DATABASE_CONTEXT_PROPAGATORS_FOR_SET_ACTION=tracecontext

Should Oracle database (pdo_oci) opt for V$SESSION.ACTION(highest priority) > SET CONTEXT_INFO > sqlcommenter (least priority )?

Should MySqli (pdo_mysql) opt for sqlcommenter only as it doesn't support other propagation ways?

@brettmc I moved the propagator logic into SqlCommenter namespace so as to describe that as a "per sqlcommenter (component) context propagator override". Please take a look to see if I get your point correctly. Thanks!

cheempz · 2025-09-18T20:35:42Z

src/Instrumentation/MySqli/src/MySqliInstrumentation.php

+
        self::addTransactionLink($tracker, $span, $mysqli);
+
+        if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query !== self::UNDEFINED) {


In the previous PR #406 the SQL Commenter feature was enabled via an explicit configuration. This PR proposes instead the customers opt-in by installing the sqlcommenter package, correct? Can the READMEs for each database instrumentation be updated to provide usage / configuration info?

Yes, the opt-in is by installing the sqlcommenter & context-propagator.
Updated the README.md per each database instrumentation library.

cheempz · 2025-09-18T20:42:29Z

src/Instrumentation/MySqli/src/MySqliInstrumentation.php

+            if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter')) {
+                $query = \OpenTelemetry\Contrib\SqlCommenter\SqlCommenter::inject($query, $comments);
+            }
+            if (class_exists('OpenTelemetry\Contrib\ContextPropagator\ContextPropagation') && \OpenTelemetry\Contrib\ContextPropagator\ContextPropagation::isAttributeEnabled()) {


nit and personal opinion, this should be a SqlCommenter (not ContextPropagation) configuration option, whether the db query text attribute reflects value pre-or-post comment injection.

There could be other ways (e.g. SET_CONTEXT_INFO / V$SESSION.ACTION) to inject comments into query but we have one way to manage context propagator.

I think it would be better to do that in context propagator package.

src/Instrumentation/MySqli/src/MySqliInstrumentation.php

cheempz · 2025-09-18T20:52:20Z

src/Instrumentation/MySqli/src/MySqliInstrumentation.php

+        $span = self::startSpan($spanName, $instrumentation, $class, $function, $filename, $lineno, []);
+        $mysqli = $obj ? $obj : $params[0];
+        self::addTransactionLink($tracker, $span, $mysqli);
+    }


Just curious why this prehook was added?

It is for multi_query scenario where it works so differently when compared to query and real_query.
In order to reflect the db query text attribute for pre-or-post comment injection, I need to move the assignment of DB_QUERY_TEXT from post hook to pre hook. query and real_query works in this way.

For multi_query, the DB_QUERY_TEXT is set at the post hook and also the next_result post hook. There is no easy way to maintain the same QueryPrehook for multi_query. So I added a pre hook.

Still not seeing what this added code is doing for multi query, can you explain.

Added comments to the function to explain.

In short, multi_query shared the same pre-hook function QueryPreHook as query and real_query. I need to create a new pre hook to keep multi_query works the same logic as before.

cheempz · 2025-09-18T23:41:14Z

src/Instrumentation/PDO/README.md

+Enable context propagation for database queries by installing the following packages:
+```shell
+composer require open-telemetry/opentelemetry-context-propagator
+composer require open-telemetry/opentelemetry-sqlcommenter


Right now it would be enabled for both postgresql or mysql under PDO, can we add a config to enable only specific driver?

@brettmc / @open-telemetry/php-maintainers Do you think it is a good idea to create a config to enable only specific driver?
e.g.
OTEL_PHP_SQLCOMMENTER_OPT_IN_DATABASE where default value = mysql and valid values are { mysql, postgresql }

For more information, the hardcoding logic https://github.com/jerrytfleung/opentelemetry-php-contrib/blob/service_name_propagator/src/Instrumentation/PDO/src/PDOInstrumentation.php#L135-L137 was approved in #406

I'd be happy to wait for somebody to ask for it, this is a 0.1 release

brettmc · 2025-10-03T05:25:32Z

plz add a .gitsplit.yaml entry for the new sql commenter package

jerrytfleung · 2025-10-03T16:16:38Z

plz add a .gitsplit.yaml entry for the new sql commenter package

@brettmc

  - prefix: "src/SqlCommenter"
    target: "https://${GH_TOKEN}@github.com/opentelemetry-php/contrib-sqlcommenter.git"

was added in .gitsplit.yml in this PR.
Is there any other entry I need to aware?

Is the PR looking good to merge?

jerrytfleung added 12 commits September 9, 2025 15:53

Initial

e612550

Added SqlCommenter and its tests

07d638d

Added context propagator and updated PDO

ad508aa

Fixed the dependencies

c7a25cc

Changed for MySqli

0b4418d

Changed for PostgreSql

812f019

nits

bad8159

Updated README.md

fe6cc73

Simplify PostgreSqlInstrumentation

eef0263

Considered the attributes in PostgreSql

ca93f88

Updated

e143523

Updated

aedc234

jerrytfleung requested a review from a team as a code owner September 17, 2025 01:31

Typo

0647e7d

jerrytfleung mentioned this pull request Sep 17, 2025

Add SQL commenter as context propagation for databases #406

Closed

brettmc reviewed Sep 17, 2025

View reviewed changes

cheempz reviewed Sep 18, 2025

View reviewed changes

src/Instrumentation/MySqli/src/MySqliInstrumentation.php Outdated Show resolved Hide resolved

cheempz reviewed Sep 18, 2025

View reviewed changes

Updated README.md(s)

06b510d

cheempz reviewed Sep 18, 2025

View reviewed changes

jerrytfleung added 7 commits September 22, 2025 10:42

Added comments

0c306dd

Remove the query command re-extraction

68ee5ac

Moved Context Propagator into SqlCommenter to make it per component

38dc8b9

Renamed env variable OTEL_PHP_SQLCOMMENTER_CONTEXT_PROPAGATORS

c2d5063

Renamed other environment variables

7e81de5

Moved logic to SqlCommenter

1bef669

nits

2b2be6c

jerrytfleung requested a review from brettmc September 26, 2025 00:07

jerrytfleung changed the title ~~feat: sqlcommenter for PDO, MySqli, PostgreSql~~ feat: sqlcommenter for PDO, MySqli, PostgreSql (pdo_mysql, pdo_pgsql) Sep 26, 2025

jerrytfleung requested a review from cheempz September 29, 2025 19:29

brettmc approved these changes Oct 3, 2025

View reviewed changes

brettmc merged commit f042bae into open-telemetry:main Oct 5, 2025
171 checks passed


		self::addTransactionLink($tracker, $span, $mysqli);

		if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query !== self::UNDEFINED) {

feat: sqlcommenter for PDO, MySqli, PostgreSql (pdo_mysql, pdo_pgsql) #442

feat: sqlcommenter for PDO, MySqli, PostgreSql (pdo_mysql, pdo_pgsql) #442

Uh oh!

Conversation

jerrytfleung commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Testing:

Unit Testing:

Manual Testing:

Uh oh!

codecov bot commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerrytfleung Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Ways of context propagation

Questions

Uh oh!

jerrytfleung Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheempz Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerrytfleung Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettmc commented Oct 3, 2025

Uh oh!

jerrytfleung commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerrytfleung commented Sep 17, 2025 •

edited

Loading

codecov bot commented Sep 17, 2025 •

edited

Loading

jerrytfleung Sep 18, 2025 •

edited

Loading

jerrytfleung Sep 26, 2025 •

edited

Loading

cheempz Sep 18, 2025 •

edited

Loading

jerrytfleung Sep 18, 2025 •

edited

Loading

jerrytfleung commented Oct 3, 2025 •

edited

Loading