feat(laravel-auto-instrumentation): use logger provider in logwatcher #286
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. |
|
cc:\ @ChrisLightfootWild |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
============================================
+ Coverage 82.50% 82.52% +0.02%
Complexity 1083 1083
============================================
Files 105 105
Lines 4578 4578
============================================
+ Hits 3777 3778 +1
+ Misses 801 800 -1 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
This has come up as an issue at least once before: https://cloud-native.slack.com/archives/C01NFPCV44V/p1715862811690339 so I think this change is probably the right way to go. Our Laravel maintainer is on leave currently, so a review might take a week or 2. |
|
No rush, I'm glad the change is sensible in the first place 👍 |
|
@hendrikheil seems sensible to me to make this switch - can you please fix up the build failures around ordered imports? 👍 |
|
@hendrikheil thanks for sorting the import order; there's a few psalm errors thrown up now. When you get time, would you be able to take a look please? You can run the Laravel pipeline locally with: make all PHP_VERSION=8.0 PROJECT=Instrumentation/Laravel |
|
@ChrisLightfootWild Thanks for sharing that command, that definitely made tracking down all issues much easier! I adjusted the failing tests for the new log data in the storage and resolved some deprecation warnings for PHP 8.4. Let me know if you'd rather me send a dedicated PR for the deprecation warnings, then I'll undo those changes. |
I fixed a bunch of those in the last few days, so rebasing off main might resolve them. If not, happy for them to be included in this PR. |
|
Thanks so much for doing this! OpenTelemetry noob here, and I wasted a week of debugging my configuration, dependencies, and endpoints wondering why I wasn't getting any logs 🤦🏻♂️ Looking forward to this getting merged! |
I am by no means an expert in the OpenTelemetry spec, but I noticed that other auto instrumentations, like the ones available for python, record logs with the logger provider APIs.
Currently the Laravel auto instrumentation stores logs it encounters as events on the span. This PR changes this behavior to use the logger provider API instead.