Be less restrictive when autoloading instrumentation libraries #133
Conversation
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
|
|
Emit a warning through trigger_error instead of using an assert. This will allow one to use the libraries in environments where the extension might not be available. However in case of issues, it will be clear through logs why code might not be instrumented
xvilo
force-pushed
the
improvement/emit-warning
branch
from
44668c3
to
368098c
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
=========================================
Coverage 52.72% 52.72%
Complexity 447 447
=========================================
Files 48 48
Lines 1686 1686
=========================================
Hits 889 889
Misses 797 797
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
xvilo
force-pushed
the
improvement/emit-warning
branch
from
af11042
to
8981132
Compare
|
This PR reminded me that we also have the capability to turn off auto-instrumentations by config, since open-telemetry/opentelemetry-php#909, but it has not been added to the . I think it should be an extra check right where you have added the various _register scripts. How do you feel about adding that here? |
xvilo
force-pushed
the
improvement/emit-warning
branch
from
8981132
to
ea80c0e
Compare
|
That would be a great idea @brettmc, I didn't know that was already there. However, I do feel personally a discussion is in place about certain things and would like to discuss this first in an issue. So for now, I'd like this PR scoped to just replacing the Furthermore, I have fixed the code style issues for some sub-libraries. We might want to stream-line those as well, as not all projects have php-cs-fixer in them, but that's also something for later. |
|
I forgot about this capability open-telemetry/opentelemetry-php#909, but I agree that this is right place to discuss such things |
|
created open-telemetry/opentelemetry-php#947 for somebody to implement disabling via config |
Emit a warning through PHP's
trigger_errorinstead of using anassertfunction call. This allows one to use the libraries in environments where the extension might not be available. However, in case of issues, it will be clear through logs why code might not be instrumented.The following approach was shortly discussed in the CNCF Slack, which should be fine for the use case at hand. The project can still run in environments that are not compatible or that are misconfigured.
I would personally also add a check for the
OTEL_PHP_AUTOLOAD_ENABLEDto let these autoload/configure. Through that, one could use the code in the libraries but not autoload it, or just let it autoload when needed. This can be discussed at a later point in time, though.As a side note, I noticed that only the WordPress auto-instrumenter had an import for the Instrumentation class, so I took the liberty to add imports in all the files. Now they are more the same.