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

Fix broken Serializer when using Laravel Illuminate\Queue\LuaScripts #309

Merged
merged 2 commits into from
Oct 27, 2024

Conversation

gdaszuta
Copy link
Contributor

Since introduction of RedisCommandWatcher, auto-instrumentation for Laravel fails when using Lua scripts for Queue management, since command argument array can contains arrays.

I don't have the time right now to prepare failing PoC, problematic payload is created in Illuminate\Queue\RedisQueue, it seems that it fails when multiple jobs are passed into argument as an object.

Since introduction of RedisCommandWatcher, auto-instrumentation for Laravel fails when using Lua scripts for Queue management, since command argument array can contains arrays.
@gdaszuta gdaszuta requested a review from a team as a code owner October 22, 2024 14:58
Copy link

welcome bot commented Oct 22, 2024

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.

Copy link

linux-foundation-easycla bot commented Oct 22, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@brettmc
Copy link
Collaborator

brettmc commented Oct 23, 2024

Looks sensible to me - can you add (or modify) a test for this?

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.29%. Comparing base (78a04ce) to head (11c4078).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #309      +/-   ##
============================================
+ Coverage     80.33%   83.29%   +2.95%     
+ Complexity     1026      896     -130     
============================================
  Files            98       83      -15     
  Lines          4114     3585     -529     
============================================
- Hits           3305     2986     -319     
+ Misses          809      599     -210     
Flag Coverage Δ
Aws 85.55% <ø> (+0.03%) ⬆️
Context/Swoole ?
Instrumentation/CakePHP ?
Instrumentation/CodeIgniter 73.77% <ø> (-0.18%) ⬇️
Instrumentation/ExtAmqp 89.26% <ø> (-0.33%) ⬇️
Instrumentation/Guzzle 69.51% <ø> (-0.23%) ⬇️
Instrumentation/HttpAsyncClient ?
Instrumentation/IO ?
Instrumentation/MongoDB 76.31% <ø> (-1.02%) ⬇️
Instrumentation/OpenAIPHP 87.31% <ø> (+0.49%) ⬆️
Instrumentation/PDO ?
Instrumentation/Psr14 77.14% <ø> (-0.99%) ⬇️
Instrumentation/Psr15 ?
Instrumentation/Psr16 97.56% <ø> (+0.06%) ⬆️
Instrumentation/Psr18 81.15% <ø> (-0.94%) ⬇️
Instrumentation/Psr3 59.49% <ø> (-0.77%) ⬇️
Instrumentation/Psr6 97.67% <ø> (+0.05%) ⬆️
Instrumentation/Slim 86.89% <ø> (-0.06%) ⬇️
Instrumentation/Symfony 88.70% <ø> (-0.37%) ⬇️
Instrumentation/Yii 77.68% <ø> (-0.10%) ⬇️
Logs/Monolog 100.00% <ø> (ø)
Propagation/ServerTiming 100.00% <ø> (ø)
Propagation/TraceResponse 100.00% <ø> (ø)
ResourceDetectors/Container 93.02% <ø> (ø)
Sampler/RuleBased 33.51% <ø> (+1.37%) ⬆️
Shims/OpenTracing 92.45% <ø> (-0.55%) ⬇️
Symfony 87.94% <ø> (-0.27%) ⬇️

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

see 56 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 78a04ce...11c4078. Read the comment docs.

@gdaszuta gdaszuta force-pushed the laravel-queue-redis branch from a1b7979 to 11c4078 Compare October 23, 2024 08:22
@gdaszuta
Copy link
Contributor Author

@brettmc added a simple test for the case, is this enough?

@ChrisLightfootWild
Copy link
Contributor

@MilesChou would you be happy with these changes?

Copy link
Contributor

@MilesChou MilesChou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good for me 👍

@brettmc brettmc merged commit 182ddb0 into open-telemetry:main Oct 27, 2024
110 of 120 checks passed
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.

4 participants