-
Notifications
You must be signed in to change notification settings - Fork 117
feat: sqlcommenter for PDO, MySqli, PostgreSql (pdo_mysql, pdo_pgsql) #442
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
Changes from all commits
e612550
07d638d
ad508aa
c7a25cc
0b4418d
812f019
bad8159
fe6cc73
eef0263
ca93f88
e143523
aedc234
0647e7d
06b510d
0c306dd
68ee5ac
38dc8b9
c2d5063
7e81de5
1bef669
2b2be6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,12 +20,14 @@ | |
|
||
/** | ||
* @phan-file-suppress PhanParamTooFewUnpack | ||
* @phan-file-suppress PhanUndeclaredClassMethod | ||
*/ | ||
class MySqliInstrumentation | ||
{ | ||
use LogsMessagesTrait; | ||
|
||
public const NAME = 'mysqli'; | ||
private const UNDEFINED = 'undefined'; | ||
|
||
private const MYSQLI_CONNECT_ARG_OFFSET = 0; | ||
private const MYSQLI_REAL_CONNECT_ARG_OFFSET = 1; // The mysqli_real_connect function in procedural mode requires a mysqli object as its first argument. The remaining arguments are consistent with those used in other connection methods, such as connect or __construct | ||
|
@@ -98,7 +100,7 @@ public static function register(): void | |
null, | ||
'mysqli_query', | ||
pre: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPreHook('mysqli_query', $instrumentation, $tracker, ...$args); | ||
return self::queryPreHook('mysqli_query', $instrumentation, $tracker, ...$args); | ||
}, | ||
post: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPostHook($instrumentation, $tracker, ...$args); | ||
|
@@ -108,7 +110,7 @@ public static function register(): void | |
mysqli::class, | ||
'query', | ||
pre: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPreHook('mysqli::query', $instrumentation, $tracker, ...$args); | ||
return self::queryPreHook('mysqli::query', $instrumentation, $tracker, ...$args); | ||
}, | ||
post: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPostHook($instrumentation, $tracker, ...$args); | ||
|
@@ -119,7 +121,7 @@ public static function register(): void | |
null, | ||
'mysqli_real_query', | ||
pre: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPreHook('mysqli_real_query', $instrumentation, $tracker, ...$args); | ||
return self::queryPreHook('mysqli_real_query', $instrumentation, $tracker, ...$args); | ||
}, | ||
post: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPostHook($instrumentation, $tracker, ...$args); | ||
|
@@ -129,7 +131,7 @@ public static function register(): void | |
mysqli::class, | ||
'real_query', | ||
pre: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPreHook('mysqli::real_query', $instrumentation, $tracker, ...$args); | ||
return self::queryPreHook('mysqli::real_query', $instrumentation, $tracker, ...$args); | ||
}, | ||
post: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPostHook($instrumentation, $tracker, ...$args); | ||
|
@@ -161,7 +163,7 @@ public static function register(): void | |
null, | ||
'mysqli_multi_query', | ||
pre: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPreHook('mysqli_multi_query', $instrumentation, $tracker, ...$args); | ||
self::multiQueryPreHook('mysqli_multi_query', $instrumentation, $tracker, ...$args); | ||
}, | ||
post: static function (...$args) use ($instrumentation, $tracker) { | ||
self::multiQueryPostHook($instrumentation, $tracker, ...$args); | ||
|
@@ -171,7 +173,7 @@ public static function register(): void | |
mysqli::class, | ||
'multi_query', | ||
pre: static function (...$args) use ($instrumentation, $tracker) { | ||
self::queryPreHook('mysqli::multi_query', $instrumentation, $tracker, ...$args); | ||
self::multiQueryPreHook('mysqli::multi_query', $instrumentation, $tracker, ...$args); | ||
}, | ||
post: static function (...$args) use ($instrumentation, $tracker) { | ||
self::multiQueryPostHook($instrumentation, $tracker, ...$args); | ||
|
@@ -440,11 +442,46 @@ private static function constructPostHook(int $paramsOffset, CachedInstrumentati | |
} | ||
|
||
/** @param non-empty-string $spanName */ | ||
private static function queryPreHook(string $spanName, CachedInstrumentation $instrumentation, MySqliTracker $tracker, $obj, array $params, ?string $class, string $function, ?string $filename, ?int $lineno): void | ||
private static function queryPreHook(string $spanName, CachedInstrumentation $instrumentation, MySqliTracker $tracker, $obj, array $params, ?string $class, string $function, ?string $filename, ?int $lineno): array | ||
{ | ||
$span = self::startSpan($spanName, $instrumentation, $class, $function, $filename, $lineno, []); | ||
$mysqli = $obj ? $obj : $params[0]; | ||
$query = $obj ? $params[0] : $params[1]; | ||
$query = mb_convert_encoding($query ?? self::UNDEFINED, 'UTF-8'); | ||
if (!is_string($query)) { | ||
$query = self::UNDEFINED; | ||
} | ||
$span->setAttributes([ | ||
TraceAttributes::DB_QUERY_TEXT => $query, | ||
TraceAttributes::DB_OPERATION_NAME => self::extractQueryCommand($query), | ||
]); | ||
|
||
self::addTransactionLink($tracker, $span, $mysqli); | ||
|
||
if (class_exists('OpenTelemetry\Contrib\SqlCommenter\SqlCommenter') && $query !== self::UNDEFINED) { | ||
/** | ||
* @psalm-suppress UndefinedClass | ||
*/ | ||
$commenter = \OpenTelemetry\Contrib\SqlCommenter\SqlCommenter::getInstance(); | ||
$query = $commenter->inject($query); | ||
if ($commenter->isAttributeEnabled()) { | ||
$span->setAttributes([ | ||
TraceAttributes::DB_QUERY_TEXT => (string) $query, | ||
]); | ||
} | ||
if ($obj) { | ||
return [ | ||
0 => $query, | ||
]; | ||
} | ||
|
||
return [ | ||
1 => $query, | ||
]; | ||
|
||
} | ||
|
||
return []; | ||
} | ||
|
||
private static function queryPostHook(CachedInstrumentation $instrumentation, MySqliTracker $tracker, $obj, array $params, mixed $retVal, ?\Throwable $exception) | ||
|
@@ -454,9 +491,6 @@ private static function queryPostHook(CachedInstrumentation $instrumentation, My | |
|
||
$attributes = $tracker->getMySqliAttributes($mysqli); | ||
|
||
$attributes[TraceAttributes::DB_QUERY_TEXT] = mb_convert_encoding($query, 'UTF-8'); | ||
$attributes[TraceAttributes::DB_OPERATION_NAME] = self::extractQueryCommand($query); | ||
|
||
if ($retVal === false || $exception) { | ||
$attributes[TraceAttributes::DB_RESPONSE_STATUS_CODE] = $mysqli->errno; | ||
} | ||
|
@@ -466,6 +500,19 @@ private static function queryPostHook(CachedInstrumentation $instrumentation, My | |
|
||
} | ||
|
||
/** | ||
* multi_query can execute multiple queries in one call. We will create a span for the multi_query call, but we will also track the individual queries and their results, creating spans for each query in the multi_query call. | ||
* The individual query spans will be created in the next_result hook, which is called to fetch the results of each query in the multi_query call. | ||
* As QueryPreHook has database span context propagation logic, we need to create this multiQueryPrehook function for multi_query to keep the pre-hook function unchanged. | ||
*/ | ||
/** @param non-empty-string $spanName */ | ||
private static function multiQueryPreHook(string $spanName, CachedInstrumentation $instrumentation, MySqliTracker $tracker, $obj, array $params, ?string $class, string $function, ?string $filename, ?int $lineno): void | ||
{ | ||
$span = self::startSpan($spanName, $instrumentation, $class, $function, $filename, $lineno, []); | ||
$mysqli = $obj ? $obj : $params[0]; | ||
self::addTransactionLink($tracker, $span, $mysqli); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious why this prehook was added? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is for For There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still not seeing what this added code is doing for multi query, can you explain. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comments to the function to explain. In short, multi_query shared the same pre-hook function |
||
|
||
private static function multiQueryPostHook(CachedInstrumentation $instrumentation, MySqliTracker $tracker, $obj, array $params, mixed $retVal, ?\Throwable $exception) | ||
{ | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,4 +33,11 @@ otel.instrumentation.pdo.distribute_statement_to_linked_spans = true | |
or environment variable: | ||
```shell | ||
OTEL_PHP_INSTRUMENTATION_PDO_DISTRIBUTE_STATEMENT_TO_LINKED_SPANS=true | ||
``` | ||
|
||
## Database Context Propagation | ||
|
||
Enable context propagation for database queries by installing the following packages: | ||
```shell | ||
composer require open-telemetry/opentelemetry-sqlcommenter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now it would be enabled for both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @brettmc / @open-telemetry/php-maintainers Do you think it is a good idea to create a config to enable only specific driver? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be happy to wait for somebody to ask for it, this is a |
||
``` |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the opt-in is by installing the sqlcommenter & context-propagator.
Updated the README.md per each database instrumentation library.