[opentelemetry-php-contrib] Auto-instrumentation packages are loaded even when OTEL_PHP_AUTOLOAD_ENABLED=false #1206
Comments
|
What method are you using to disable the |
|
I'm removing the extension .ini file from PHP config paths. I've pushed a small reproducer here: https://github.com/joaojacome/otel-auto-reproducer You can reproduce it with |
|
That env var only controls SDK autoloading (setting up an SDK from environment variables). That said, SDK autoloading is a requirement for auto-instrumentation to work (or if not that, then some other thing that configures and registers an SDK as part of composer's autoloader). |
|
I imagined so, but yeah, for me it makes sense to disable the auto instrumentation packages too.
I've found this issue when I tried running phpunit with the extension enabled and the PSR-18 auto instrumentation package installed. Due to a 'ClientInterface' mock the tests were hanging. To my surprise, disabling the auto instrumentation autoload didn't work.
If you guys think that the auto instrumentation packages should also be disabled if the SDK autoload is disabled, I can prepare a PR with those changes.
|
|
I've pushed a PR with the SDK changes - the idea is to expose the Example for the PSR18 package: @@ -1,18 +1,21 @@
<?php
declare(strict_types=1);
use OpenTelemetry\Contrib\Instrumentation\Psr18\Psr18Instrumentation;
use OpenTelemetry\SDK\Sdk;
-if (class_exists(Sdk::class) && Sdk::isInstrumentationDisabled(Psr18Instrumentation::NAME) === true) {
+if (class_exists(Sdk::class) && (
+ Sdk::isInstrumentationDisabled(Psr18Instrumentation::NAME) === true
+ || Sdk::isAutoloadEnabled() === false
+)) {
return;
}
if (extension_loaded('opentelemetry') === false) {
trigger_error('The opentelemetry extension must be loaded in order to autoload the OpenTelemetry PSR-18 auto-instrumentation', E_USER_WARNING);
return;
}
Psr18Instrumentation::register();Another approach would be to have an If the PR is approved, I'll follow up with a PR for updating the auto-instrumentation packages. |
|
I think this is the same issue as #1191 Thinking about it more, I don't think disabling the package based on whether autoload is enabled or not is correct; they're not the same thing and while it's true today that they're strongly coupled, I can imagine somebody writing their own SDK loader that runs via composer's I asked in the linked issue what would happen if we made the error less dramatic ( |
|
It looks like its the same, yes. The reason I opened this wasn't really the warnings themselves, but rather that I'm facing some segmantation faults when running tests with the opentelemetry extension enabled. When mocking a PSR18 Sample strace (collapsed)```Somehow that issue is gone if I update Given that there's no reason (at least for me) to have auto-instrumentation enabled on unit tests, I could disable the extension (and get the warnings), or set the Of course, that's not a big deal, but if we had an easy way of "fully disable open-telemetry auto-instrumentation", I'd rather use that. |
How about |
I've seen something similar this week, and it was due to the type-hint for pre/post hook being incompatible with the actual type. Just as a test, you could remove the type-hints and see if your error goes away. If so, it could be that the mocked object's type isn't quite what the pre or post hook expects. There may be a bug in the extension we need to understand and fix, there's certainly a semi-related issue logged... |
That would be perfect. I'll give the implementation a go here.
It seems to be the case. I've removed a few type-hints on |
I think it could be fixed by open-telemetry/opentelemetry-php-instrumentation#118 |
|
@joaojacome Added a test which I think confirms that your bug fails nicely rather than hang with the just-merged fix: open-telemetry/opentelemetry-php-instrumentation#119
Can you create a new issue for this? |
#1220 PR adding the option I'll follow up with a PR for the docs too.
Is that necessary even with the hook exception handling fix? |
Maybe not. I wasn't clear on whether you thought there was an actual bug in opentelemetry-auto-psr18 or not. If the only fix required is in the extension, then nothing further needed except to verify. |
Given that the psr18 auto-instrumentation package requires About the Thanks! |
|
I couldn't reproduce (what I think is) your issue with a test, using psr/http-message 1.1 and extension 1.0.1beta1 Drop this into public function test_send_mock_request(): void
{
$request = $this->createMock(RequestInterface::class);
$uri = $this->createMock(UriInterface::class);
$request->method('getUri')->willReturn($uri);
$request->method('withoutHeader')->willReturnSelf();
$request->method('withAddedHeader')->willReturnSelf();
$this->client->sendRequest($request);
}It doesn't fail, but did whilst I worked out those |
|
Update: with the just-released extension 1.0.1beta2, now if I break the above test it fails as expected and more helpfully: Could you please re-test, @joaojacome ? |
|
Closing, as the original issue is resolved, and I'm pretty confident the extra issue (hanging whilst mocking) is also fixed in the latest version of the extension. |
|
All working good now :) Thanks for the help! |
Describe your environment
Steps to reproduce
opentelemetryextensionecho "require('vendor/autoload.php');" | OTEL_PHP_AUTOLOAD_ENABLED=false php -aWhat is the expected behavior?
I'd expect the auto instrumentation packages to be ignored.
What is the actual behavior?
The auto instrumentation packages are loaded, and you'll see a PHP warning:
The text was updated successfully, but these errors were encountered: