Skip to content

Allow for Redis info arrays which use Clients and Server keys#60

Open
Sacrome wants to merge 7 commits intolaminas:1.25.xfrom
Sacrome:bc-fix-redis-check
Open

Allow for Redis info arrays which use Clients and Server keys#60
Sacrome wants to merge 7 commits intolaminas:1.25.xfrom
Sacrome:bc-fix-redis-check

Conversation

@Sacrome
Copy link

@Sacrome Sacrome commented Nov 15, 2022

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

The Redis Check has been extended by including response time, number of connected clients and uptime.
(https://github.com/laminas/laminas-diagnostics/pull/57/files#diff-ae274e5d76cba73799bb44059a715045e3721dbe71f125ad128df5d2152c2a52)

However, the number of connected clients and uptime may need to be retrieved in slightly different ways, as the array structure returned by Predis\ClientInterface::info() can have different data structures.

The previous behaviour only allowed directly retrieving connected_clients and uptime_in_seconds from the array returned by Predis\ClientInterface::info(). The suggested changes allow for those keys residing in $array['Clients'] and $array['Server'] respectively, as was the case in my setup.

In my testing I used Redis V6.0.6 and Predis V1.1.10.

NB: The suggested change allows for both data structures of the info array.

Signed-off-by: Tim Visser <tvisser@sacrome.com>
Signed-off-by: Tim Visser <tvisser@sacrome.com>
Signed-off-by: Tim Visser <tvisser@sacrome.com>
@Sacrome Sacrome force-pushed the bc-fix-redis-check branch from ea008dd to e66b6b1 Compare January 17, 2023 14:09
Signed-off-by: Tim Visser <tvisser@sacrome.com>
@Sacrome Sacrome force-pushed the bc-fix-redis-check branch from 0879aae to 4be10bd Compare January 17, 2023 14:29
@Sacrome Sacrome changed the base branch from 1.20.x to 1.24.x January 17, 2023 14:30
Signed-off-by: Tim Visser <tvisser@sacrome.com>
@Sacrome Sacrome force-pushed the bc-fix-redis-check branch from 4be10bd to fddceab Compare January 17, 2023 14:31
… information

Signed-off-by: Tim Visser <tvisser@sacrome.com>
Signed-off-by: Tim Visser <tvisser@sacrome.com>
@Sacrome
Copy link
Author

Sacrome commented Jan 17, 2023

@Ocramius I've reworked the changes a bit - they now include some missing checks/guards as Psalm helpfully commented on and the change now targets the 1.24.x branch which I've merged into my branch.

Wrt the 'needs unit test' label - the only way I could test this is by mocking Redis/ClientInterface which are obviously both not part of laminas-diagnostics. So that only provides false sense of security as they could change their API at any moment and the test would be none the wiser. Wdyt?

@Sacrome
Copy link
Author

Sacrome commented Mar 22, 2023

@Ocramius Could you have a look at this?

@Ocramius Ocramius changed the base branch from 1.24.x to 1.25.x March 22, 2023 11:55
@Ocramius
Copy link
Member

About testing, I wonder if we can perhaps really spin up a redis instance? 🤔

@mremi
Copy link

mremi commented Dec 5, 2023

Hi,
I have the same issue, could it be merged?
Thanks

@lonely-pilot
Copy link

Is it possible to finally fix this issue after more than a year, please? 🙏🏻

@Sacrome
Copy link
Author

Sacrome commented Feb 10, 2024

Setting up a testing scenario for specific Redis versions is not something I’m able to do unfortunately 🤷‍♂️

Also I’m currently not actively working on PHP projects anymore so I’m affraid I won’t be actively following this PR (especially after it has been up here for more than a year 😅)

@mvhirsch
Copy link

I just ran into this. I can confirm that this fixes the bug throwing an error:

PHP WARNING: Undefined array key "connected_clients"

I'm running a Redis 6 setup using Redis extension locally for development and Predis on production. I copied these changes and it just works. Can we please merge this and provide an interface and/or tests in a follow up PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants