👷 Laravel instrumentation 1.1.x #269
Conversation
|
|
||
| return $root | ||
| ->canBeDisabled() | ||
| ->addDefaultsIfNotSet() |
There was a problem hiding this comment.
I'd like to add a hooks array here with configuration of each hook, unless there's a better alternative?
There was a problem hiding this comment.
I think you can do that, the laravel configuration schema can accept any shape you can define with symfony/config. For example, create an array for each component that is instrumented, which can individually be turned on/off, configure other values, ...
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #269 +/- ##
============================================
- Coverage 79.85% 77.99% -1.86%
+ Complexity 887 609 -278
============================================
Files 85 66 -19
Lines 3495 2577 -918
============================================
- Hits 2791 2010 -781
+ Misses 704 567 -137
Flags with carried forward coverage won't be shown. Click here to find out more. see 37 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
… constants from sem-conv 1.26.
| LoggerInterface $logger, | ||
| MeterInterface $meter, | ||
| TracerInterface $tracer, |
There was a problem hiding this comment.
Should I actually be passing the *ProviderInterfaces around?
My current understanding would be that I'd have to duplicate the instantiation or provide a Factory to give me back the implementations I want.
There was a problem hiding this comment.
Yes, I think you should. I also think you should just accept the InstrumentationContext here, and retrieve the providers you need for this particular hook (less rework if we add new providers in future). Also, I think the Tracer/Logger/etc for this hook should have a unique name that includes the component being instrumented, eg io.opentelemetry.contrib.php.laravel.kernel - the reason for this is that we can use scope configuration to turn off parts of auto-instrumentation based on that name.
There was a problem hiding this comment.
Thanks @brettmc, all of that sounds sensible. I will look to make the switch.
I will have to have a tinker with the scope configuration here and see how that goes!
…nstead of Globals.
ChrisLightfootWild
force-pushed
the
spi/laravel
branch
from
5d3210d
to
a3cf950
Compare
ChrisLightfootWild
force-pushed
the
spi/laravel
branch
from
1a1daa0
to
504796e
Compare
ChrisLightfootWild
changed the title
| "ext-json": "*", | ||
| "ext-opentelemetry": "*", |
There was a problem hiding this comment.
Changed based on similar issue raised for PDO instrumentation.
WIP: current test approach needs tweaking.
Using new functionality provided via open-telemetry/opentelemetry-php#1304.
This should probably form the basis of a v1.1 package, so depends on a 1.0 release of the current Laravel instrumentation.