Reconsider extension check in opentelemetry-auto-pdo

[SnakingSappyGoat]

The open-telemetry/opentelemetry-auto-pdo package triggers a PHP warning if the extension is not loaded:

https://github.com/opentelemetry-php/contrib-auto-pdo/blob/38910f71b9cf75da989bd06711339ff5459442d1/_register.php#L13

This is a bit of a nuisance for local development, because it for example makes PHPStan integration in PHPStorm constantly spew errors.

I know that I can set OTEL_PHP_DISABLED_INSTRUMENTATIONS=pdo in the environment variables, but that is inconvenient to ask of developers that don't interact with opentelemetry components at all.

Since the extension is already declared in composer as a requirement, I'm not sure the warning adds anything, so perhaps we can get rid of it?

[xvilo]

I would like to add that this issue is the same for all auto-instrumentation packages, and not just the auto-pdo package. We've talked about this internally and also thought about using the OTEL_SDK_DISABLED instead. However, that states the following:

Boolean value. If “true”, a no-op SDK implementation will be used for all telemetry signals. Any other value or absence of the variable will have no effect and the SDK will remain enabled. This setting has no effect on propagators configured through the OTEL_PROPAGATORS variable.

And we weren't sure if that's a proper mechanism to disable the auto instrumentation. That could result in the following diff:

-if (class_exists(Sdk::class) && Sdk::isInstrumentationDisabled(PDOInstrumentation::NAME) === true) {
+if (
+    (class_exists(Sdk::class) && Sdk::isInstrumentationDisabled(PDOInstrumentation::NAME) === true) ||
+    Sdk::isDisabled()
+) {
    return;
}

This warning was added through open-telemetry/opentelemetry-php-contrib#133 as the assert() was even more intrusive by fulling stopping the code instead of a graceful warning.

---

Additionally, this situation exists because we do some composer trickery by overriding platform.config with the extension. If we hadn't had this, @SnakingSappyGoat wouldn't have been able to install the dependencies, as the dependency is not available in their installation of PHP.

[brettmc]

Can you elaborate on your use case - why do you have the package installed without the extension? And, by avoiding that particular error, aren't other static analysis tools going to complain just as loudly about missing stuff?

[SnakingSappyGoat]

The package is required as composer dependency on a project I work on (with a couple dozen of other people).

On my local machine I have no use for the actual operation of the OpenTelemetry integration, because the local code is only there for static analysis, IDE completions, and unit test runs.

Any contributors to our project with the same setup would have to manually install the extension with PECL or set up some other workaround, which creates unnecessary development friction.

Static analysis tools only check the project code. So unless we are doing direct calls to the extension functions, I'm not sure how they would complain about missing stuff? Even if they do, they will just include the issues in the report instead of directly pushing the PHP warnings to stderr, which is why the IDE integration keeps popping up errors about it. One could argue that this is a problem with the static analysis tools or IDE integration, but this is the first package I have come across to cause something like this.

The only issue I can imagine with silently ignoring the missing extension, is that the OpenTelemetry integration might be implemented in a path that is hit by unit tests. A possible solution could be that this SDK would "mock" extension-exposed functions as no-ops if the extension is not available. Having said that, you could also leave the responsibility of creating those stubs to the project (but it might still be nice if this SDK would include a function that can be called from the test bootstrapper to do that).

[brettmc]

OK, that makes sense. So the issue is not so much that it complains about missing extension, but the way it complains.

From memory, the main reason that the check is there is that whilst the composer requirement should be enough, we were concerned that people would bypass that and then be surprised when it doesn't work.

As a test, is your IDE happier if trigger_error is replaced with error_log?

[brettmc]

@SnakingSappyGoat @xvilo - we've recently added the ability to set OTEL_PHP_DISABLED_INSTRUMENTATIONS=all, does that help at all? If not, please report back on whether error_log helps per my last comment.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.