-
Notifications
You must be signed in to change notification settings - Fork 214
Disable SDK from autoloading during composer execution. #1727
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
9ec2a8c
c9f43e4
e3005fb
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,4 +2,17 @@ | |||||
|
|
||||||
| declare(strict_types=1); | ||||||
|
|
||||||
| \OpenTelemetry\SDK\SdkAutoloader::autoload(); | ||||||
| /** | ||||||
| * If OTEL_PHP_AUTOLOAD_ENABLED=true, there may be compatability issues | ||||||
| * if the version of PSR-3 installed in ./vendor conflicts with that of the | ||||||
| * packaged composer PSR-3 library. | ||||||
| * | ||||||
| * If COMPOSER_DEV_MODE is present, then we can assume that a composer script | ||||||
| * is running, and we can prevent the PSR-3 compatability issues by disabling | ||||||
| * the SDK from activating. | ||||||
| * | ||||||
| * @see https://github.com/open-telemetry/opentelemetry-php/issues/1673 | ||||||
| */ | ||||||
| if (getenv('COMPOSER_DEV_MODE') === false) { | ||||||
|
||||||
| if (getenv('COMPOSER_DEV_MODE') === false) { | |
| if (getenv('COMPOSER_BINARY') === false) { |
COMPOSER_DEV_MODE is only set by the run-script command, running scripts directly still fails:
OTEL_PHP_AUTOLOAD_ENABLED=true composer test-psr3Edit: checking solely for COMPOSER_BINARY will be too strict as this environment variable is also propagated to @php scripts.
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.
@Nevay Ah, that is unfortunate - thanks for highlighting. Realistically, there may be no safe way to do this then? 🤔
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.
Why not start with COMPOSER_DEV_MODE, since that covers some failure cases? It could be broken out into its own class+function such as ComposerHandler::isRunning() to allow for future expansion of methods to identify whether composer is running?
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.
I've made the suggested change to move to a ComposerHandler::isRunning() check.
I have also added an additional rudimentary check to try to infer whether or not the composer executable has been called directly.
This does fix the problem with running this way:
OTEL_PHP_AUTOLOAD_ENABLED=true composer test-psr3
Are there any foreseen problems in doing these additional checks?
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.
I also noted a discussion started here which links to this issue.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace OpenTelemetry\Tests\Integration\Composer; | ||
|
|
||
| use Composer\Script\Event; | ||
|
|
||
| final class Psr3Compatability | ||
| { | ||
| public static function run(Event $event): void | ||
| { | ||
| require_once $event->getComposer()->getConfig()->get('vendor-dir') . '/autoload.php'; | ||
| } | ||
| } |
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.
What would happen if you were starting your application through a composer script? eg something like
composer run-my-application?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.
Previously, if you were using a compatible version of
psr/logthen the SDK would bootstrap.If you were using an incompatible version, it would cause a fatal exception.
With the proposed changes, the SDK is essentially disabled.
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.
I was really wondering whether we're breaking any legitimate use-cases...eg do folks use composer scripts to start and run an application? And if so, will they be surprised when instrumentation stops working? I'm happy to deal with that separately if it comes up.