Skip to content
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

Laravel: handle possible LogWatcher TypeError's #311

Merged

Conversation

ChrisLightfootWild
Copy link
Contributor

Based on reported issues in #306 (with thanks to @flc1125 for raising it).

Given we are close to moving to package 1.1.x and dropping support of Laravel 6-9, then it may be preferable to restore the previous behaviour and allow the log messages through. This would keep the package viable for the older Laravel versions.

Unfortunately, there's no log level filtering at that point but I think it's better to handle this gracefully.

The approach in #306 could also provide a stable v1, while we look to tackle this more elegantly in future.

@ChrisLightfootWild ChrisLightfootWild requested a review from a team as a code owner October 26, 2024 20:17
Copy link

codecov bot commented Oct 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.11%. Comparing base (2d2c37b) to head (2166ec5).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #311      +/-   ##
============================================
+ Coverage     80.17%   86.11%   +5.93%     
+ Complexity     1028       73     -955     
============================================
  Files            98        6      -92     
  Lines          4268      360    -3908     
============================================
- Hits           3422      310    -3112     
+ Misses          846       50     -796     
Flag Coverage Δ
Aws ?
Context/Swoole ?
Instrumentation/CakePHP ?
Instrumentation/CodeIgniter ?
Instrumentation/ExtAmqp 89.26% <ø> (ø)
Instrumentation/Guzzle ?
Instrumentation/HttpAsyncClient ?
Instrumentation/IO 70.68% <ø> (ø)
Instrumentation/MongoDB ?
Instrumentation/OpenAIPHP ?
Instrumentation/PDO ?
Instrumentation/Psr14 ?
Instrumentation/Psr15 ?
Instrumentation/Psr16 97.56% <ø> (ø)
Instrumentation/Psr18 81.15% <ø> (ø)
Instrumentation/Psr3 ?
Instrumentation/Psr6 ?
Instrumentation/Slim ?
Instrumentation/Symfony ?
Instrumentation/Yii ?
Logs/Monolog ?
Propagation/ServerTiming ?
Propagation/TraceResponse ?
ResourceDetectors/Container 93.02% <ø> (ø)
Sampler/RuleBased ?
Shims/OpenTracing ?
Symfony ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 92 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d2c37b...2166ec5. Read the comment docs.

@flc1125
Copy link
Member

flc1125 commented Oct 27, 2024

If we plan to only support Laravel versions 10 + and above.

I think the Laravel dependency in composer.json can also be evaluated if it needs to be adjusted.

"laravel/framework": "^6.0 || ^7.0 || ^8.0 || ^9.0 || ^10.0 || ^11.0",

Another message: Aside from the fix this time, I'm currently running fine on version 9.x.

@ChrisLightfootWild
Copy link
Contributor Author

If we plan to only support Laravel versions 10 + and above.

I think the Laravel dependency in composer.json can also be evaluated if it needs to be adjusted.

"laravel/framework": "^6.0 || ^7.0 || ^8.0 || ^9.0 || ^10.0 || ^11.0",

Another message: Aside from the fix this time, I'm currently running fine on version 9.x.

Yes to v1.1 moving to support 10 + 11 👍

This package should still work down to Laravel 6; it is just hard to make progress when having to test so many end-of-life versions!

@flc1125
Copy link
Member

flc1125 commented Oct 27, 2024

If we plan to only support Laravel versions 10 + and above.
I think the Laravel dependency in composer.json can also be evaluated if it needs to be adjusted.

"laravel/framework": "^6.0 || ^7.0 || ^8.0 || ^9.0 || ^10.0 || ^11.0",

Another message: Aside from the fix this time, I'm currently running fine on version 9.x.

Yes to v1.1 moving to support 10 + 11 👍

This package should still work down to Laravel 6; it is just hard to make progress when having to test so many end-of-life versions!

Very good plan. If multi-version compatibility needs to be considered, it is a challenge.

Currently, accelerating the advancement of Laravel in OTEL may be a more priority task to do. 💪🏻

@brettmc brettmc merged commit 0a41216 into open-telemetry:main Oct 27, 2024
111 of 120 checks passed
@ChrisLightfootWild ChrisLightfootWild deleted the fix/laravel/logwatcher branch November 17, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants