search

open-telemetry / opentelemetry-php

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

[opentelemetry-php-contrib] Wrong php requirements on pdo autoinstrumentation #1360

Closed AllPDFfillersGoToHeaven closed 1 month ago

AllPDFfillersGoToHeaven commented 2 months ago

I would like to use https://github.com/opentelemetry-php/contrib-auto-pdo in my project to auto-instrument PDO. Composer.json explicitly says (after 0.0.1 version) that it requires php 8.2 and it's not true. I've located the commit that made this adjustment and it seems that developer made a mistake of assuming php below 8.2 didn't have observability support(https://github.com/open-telemetry/opentelemetry-php-contrib/commit/3c52850b8e30db6e404dc08480f25feae4291f80). I ignored this requirements in dev and PDO is being auto-instrumented on php

please fix this as it breaks my composer

What is the expected behavior? Composer being able to install in prod php 8.0

What is the actual behavior? Pdo autoinstrumentation lib blocked composer installation

brettmc commented 2 months ago

Hi @AllPDFfillersGoToHeaven The requirement for 8.2 is due to the observer API, which our auto-instrumentation extension is built upon. It only got the ability to observe "internal" functions (provided by PHP itself, and extensions) from php 8.2. That includes PDO.

I'm surprised to hear that it works for you in php8.0 - I don't understand how. Can you double-check that auto-instrumentation is really working against 8.0 on PDO?

AllPDFfillersGoToHeaven commented 2 months ago

Hi @AllPDFfillersGoToHeaven The requirement for 8.2 is due to the observer API, which our auto-instrumentation extension is built upon. It only got the ability to observe "internal" functions (provided by PHP itself, and extensions) from php 8.2. That includes PDO.

I'm surprised to hear that it works for you in php8.0 - I don't understand how. Can you double-check that auto-instrumentation is really working against 8.0 on PDO?

yes, I will double-check, maybe I made a mistake. am I correct in understanding that other auto-instrumentation libraries (for example https://packagist.org/packages/open-telemetry/opentelemetry-auto-guzzle) do NOT require php 8.2 because PDO::class is considered to be "internal" and Guzzle(in this case) is considered to be "external"?

brettmc commented 2 months ago

do NOT require php 8.2 because PDO::class is considered to be "internal" and Guzzle(in this case) is considered to be "external"?

Yes, that's right. AMQP and RdKafka are the same - provided by extensions and so require 8.2