itsgoingd / clockwork

Clockwork - php dev tools in your browser - server-side component
https://underground.works/clockwork
MIT License
5.63k stars 323 forks source link

Redis::pipeline calls not being instrumented #664

Open cyppe opened 9 months ago

cyppe commented 9 months ago

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: https://github.com/laravel/framework/issues/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 commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

@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.