PDO auto instrumentation #1 #102

pdelewski · 2023-01-30T20:26:33Z

Initial commit of PDO auto instrumentation

codecov · 2023-01-31T11:58:20Z

Codecov Report

Merging #102 (5d1a298) into main (69e8c30) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff            @@
##               main     #102   +/-   ##
=========================================
  Coverage     64.42%   64.42%           
  Complexity      399      399           
=========================================
  Files            42       42           
  Lines          1380     1380           
=========================================
  Hits            889      889           
  Misses          491      491

Flag	Coverage Δ
7.4	`81.76% <ø> (ø)`
8.0	`64.24% <ø> (ø)`
8.1	`64.32% <ø> (ø)`
8.2	`64.32% <ø> (ø)`

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

Continue to review full report at Codecov.

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

brettmc · 2023-02-01T14:00:17Z

don't forget to add this to .gitsplit.yml. I can create (or show you how) a read-only repo for it to be pushed to. Then, we can create an entry in packagist for this.

pdelewski · 2023-02-01T14:01:43Z

don't forget to add this to .gitsplit.yml. I can create (or show you how) a read-only repo for it to be pushed to. Then, we can create an entry in packagist for this.

ok, thank you

brettmc · 2023-02-01T22:22:56Z

src/Instrumentation/PDO/composer.json

+  "require": {
+    "php": "^8.0",
+    "ext-otel_instrumentation": "*",
+    "open-telemetry/api": "*",


Suggested change

"open-telemetry/api": "*",

"open-telemetry/api": "^1",

brettmc · 2023-02-01T22:24:13Z

src/Instrumentation/PDO/src/PDOInstrumentation.php

+                    ->setAttribute(TraceAttributes::DB_CONNECTION_STRING, $params[0] ?? 'unknown')
+                    ->setAttribute(TraceAttributes::DB_USER, $params[1] ?? 'unknown')
+                    // parse connection string
+                    ->setAttribute(TraceAttributes::DB_SYSTEM, 'mysql');


can this be implemented to get real value from connection string?

yes, right. I forgot about this gap.

brettmc · 2023-02-01T22:25:21Z

src/Instrumentation/PDO/src/PDOInstrumentation.php

+            'fetchAll',
+            pre: static function (\PDOStatement $statement, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) {
+                /** @psalm-suppress ArgumentTypeCoercion */
+                $builder = self::makeBuilder($instrumentation, 'PDOStatement::fetch', $function, $class, $filename, $lineno)


Suggested change

$builder = self::makeBuilder($instrumentation, 'PDOStatement::fetch', $function, $class, $filename, $lineno)

$builder = self::makeBuilder($instrumentation, 'PDOStatement::fetchAll', $function, $class, $filename, $lineno)

brettmc · 2023-02-01T22:25:36Z

src/Instrumentation/PDO/src/PDOInstrumentation.php

+            'execute',
+            pre: static function (\PDOStatement $statement, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) {
+                /** @psalm-suppress ArgumentTypeCoercion */
+                $builder = self::makeBuilder($instrumentation, 'PDOStatement::fetch', $function, $class, $filename, $lineno)


Suggested change

$builder = self::makeBuilder($instrumentation, 'PDOStatement::fetch', $function, $class, $filename, $lineno)

$builder = self::makeBuilder($instrumentation, 'PDOStatement::execute', $function, $class, $filename, $lineno)

brettmc · 2023-02-02T07:15:35Z

src/Instrumentation/PDO/composer.json

+    "php": "^8.0",
+    "ext-otel_instrumentation": "*",
+    "open-telemetry/api": "^1",
+    "nyholm/psr7": "^1",


I think nyholm/* is not required for this instrumentation

Yes, right. Will remove it.

brettmc

Approved, merge when you're ready. I've created https://github.com/opentelemetry-php/contrib-auto-pdo so the git-split has a destination...

pdelewski · 2023-02-02T07:44:24Z

Approved, merge when you're ready. I've created https://github.com/opentelemetry-php/contrib-auto-pdo so the git-split has a destination...

ok, thank you

pdelewski · 2023-02-02T12:38:27Z

@brettmc For dsn parsing I added additional dependency https://github.com/Nyholm/dsn. If you have something better or you think that we should implement this by ourselves just let me know (will do that in following PR, however for me it would be reinventing the wheel).

pdelewski · 2023-02-02T14:34:57Z

@brettmc For dsn parsing I added additional dependency https://github.com/Nyholm/dsn. If you have something better or you think that we should implement this by ourselves just let me know (will do that in following PR, however for me it would be reinventing the wheel).

Hmm, I see that sdk contains dsn parser, however it seems that it's not general enough and does not fulfil PDO requirements as I'm getting InvalidArgumentException: An exporter DSN must have a exporter type and a scheme: type+scheme://host:port

Nevay · 2023-02-02T18:05:44Z

src/Instrumentation/PDO/src/PDOInstrumentation.php

+                    ->setSpanKind(SpanKind::KIND_CLIENT)
+                    ->setAttribute(TraceAttributes::DB_CONNECTION_STRING, $params[0] ?? 'unknown')
+                    ->setAttribute(TraceAttributes::DB_USER, $params[1] ?? 'unknown')
+                    ->setAttribute(TraceAttributes::DB_SYSTEM, self::getDB($params[0]));


You can use $pdo->getAttribute(PDO::ATTR_DRIVER_NAME) to set db.system in the post hook.

Nevay · 2023-02-02T18:07:06Z

src/Instrumentation/PDO/src/PDOInstrumentation.php

+                /** @psalm-suppress ArgumentTypeCoercion */
+                $builder = self::makeBuilder($instrumentation, 'PDO::__construct', $function, $class, $filename, $lineno)
+                    ->setSpanKind(SpanKind::KIND_CLIENT)
+                    ->setAttribute(TraceAttributes::DB_CONNECTION_STRING, $params[0] ?? 'unknown')


Should check that $class === PDO::class, subclasses might declare different parameters.

Nevay · 2023-02-02T18:11:05Z

src/Instrumentation/PDO/src/PDOInstrumentation.php

+
+        hook(
+            \PDOStatement::class,
+            'fetch',


IMO we shouldn't create a span per ::fetch() etc.; large result sets will lead to a lot of spans.

I was thinking about it and plan to change this behaviour in subsequent PR(s). BTW I was thinking about kind of grouping fetch operations mechanism, however don't know yet if that will work

pdelewski requested a review from a team January 30, 2023 20:26

pdelewski changed the title ~~PDO auto instrumentation WIP~~ PDO auto instrumentation #1 Feb 1, 2023

Przemek Delewski added 6 commits February 1, 2023 16:46

PDO auto instrumentation initial commit

7c77911

PDO initial integration test

912676f

adding readme

7736d4f

adding explicit return type to makeBuilder

67bbb43

add more operations

821dfdd

update .gitsplit.yml

29896a8

pdelewski force-pushed the PDO-auto-instrumentation branch from 9ff0a27 to 29896a8 Compare February 1, 2023 15:49

brettmc reviewed Feb 1, 2023

View reviewed changes

fixes

b3f89ff

pdelewski force-pushed the PDO-auto-instrumentation branch from 319dd6d to b3f89ff Compare February 2, 2023 07:07

brettmc reviewed Feb 2, 2023

View reviewed changes

brettmc approved these changes Feb 2, 2023

View reviewed changes

Przemek Delewski added 2 commits February 2, 2023 11:26

removing psr7 dependencies

5bd4d66

parsing dsn

5d1a298

pdelewski merged commit c2adbfb into open-telemetry:main Feb 2, 2023

Nevay reviewed Feb 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PDO auto instrumentation #1 #102

PDO auto instrumentation #1 #102

pdelewski commented Jan 30, 2023

codecov bot commented Jan 31, 2023 •

edited

Loading

brettmc commented Feb 1, 2023

pdelewski commented Feb 1, 2023

brettmc Feb 1, 2023

brettmc Feb 1, 2023

pdelewski Feb 2, 2023

brettmc Feb 1, 2023

brettmc Feb 1, 2023

brettmc Feb 2, 2023

pdelewski Feb 2, 2023

brettmc left a comment

pdelewski commented Feb 2, 2023

pdelewski commented Feb 2, 2023

pdelewski commented Feb 2, 2023 •

edited

Loading

Nevay Feb 2, 2023

pdelewski Feb 2, 2023

Nevay Feb 2, 2023

Nevay Feb 2, 2023

pdelewski Feb 2, 2023 •

edited

Loading

	$builder = self::makeBuilder($instrumentation, 'PDOStatement::fetch', $function, $class, $filename, $lineno)
	$builder = self::makeBuilder($instrumentation, 'PDOStatement::fetchAll', $function, $class, $filename, $lineno)

PDO auto instrumentation #1 #102

PDO auto instrumentation #1 #102

Conversation

pdelewski commented Jan 30, 2023

codecov bot commented Jan 31, 2023 • edited Loading

Codecov Report

brettmc commented Feb 1, 2023

pdelewski commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettmc left a comment

Choose a reason for hiding this comment

pdelewski commented Feb 2, 2023

pdelewski commented Feb 2, 2023

pdelewski commented Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdelewski Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 31, 2023 •

edited

Loading

pdelewski commented Feb 2, 2023 •

edited

Loading

pdelewski Feb 2, 2023 •

edited

Loading