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

Redis::pipeline calls not being instrumented #664

Open
cyppe opened this issue Dec 15, 2023 · 3 comments
Open

Redis::pipeline calls not being instrumented #664

cyppe opened this issue Dec 15, 2023 · 3 comments

Comments

@cyppe
Copy link

cyppe commented Dec 15, 2023

Hello!

I noticed today that Laravel does not dispatch events when calling redis commands in Redis::pipeline(). So in my case where I use pipeline a lot to send GET commands in batch to Redis they are invisible for clockwork.

I logged an issue on laravel/framework repo:
Issue: laravel/framework#49395

But posting here just as information and maybe someone here is interested enough and skilled to help solve it. As I don't know where to start looking for adding additional triggers for Illuminate\Redis\Events\CommandExecuted event.

@itsgoingd
Copy link
Owner

Hey, thanks for the heads up.

Note, the following is based only on reading the Laravel code, I haven't tested anything.

Looks like for pipelines, they pass the PhpRedis client instance itself to the closure, instead of the Laravel's Redis connection, which dispatches the command-executed events.

Redis::pipeline(function ($pipe) {
    // This is PhpRedis instance, so Laravel has no idea what commands I'm calling.
    $pipe->get('foo');
});

I think they could literally just replace the PhpRedis instance with Laravel's Redis connection and it should "just work", since it passes all calls to the same PhpRedis instance internally.

This means simply replacing $pipeline with $this on this line:
https://github.com/laravel/framework/blob/10.x/src/Illuminate/Redis/Connections/PhpRedisConnection.php#L405

Redis::pipeline(function ($pipe) {
    // This is now the same as calling Redis::get, so Laravel dispatches command-executed events.
    $pipe->get('foo');
    // Or you can literally just do this, without any changes to Laravel code and it should do the same thing
    Redis::get('foo');
});

I don't feel like working on this PR myself, but if anyone else wants to, I'm down to help.

@cyppe
Copy link
Author

cyppe commented Dec 17, 2023

That is amazing.

I can confirm that using Redis::get('foo'); inside the pipeline closure really works. And it still uses the pipeline method. I was afraid it would go back to sending them one by one, but it really works.

Then if someone wants to do a PR to Laravel it would be even better as it will fix any code. But as I am not comfortable enough to do it I will change to using Redis::get in my code for now.

@unseen703
Copy link

@itsgoingd I am familiar with Laravel, PHP and I want to contribute to this, can you help me in how to start with raising a PR.

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

No branches or pull requests

3 participants