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

Add responsetime/uptime/connection-count to memcache + redis checks #57

Merged
merged 3 commits into from Nov 11, 2022
Merged

Add responsetime/uptime/connection-count to memcache + redis checks #57

merged 3 commits into from Nov 11, 2022

Conversation

bram123
Copy link
Contributor

@bram123 bram123 commented Nov 10, 2022

Signed-off-by: Bram Leeda bram@123inkt.nl

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

Description

Added more CheckResult data values:

  • responsetime
  • uptime
  • connections

Added as a keyed array following DirWritable, or do you prefer a dataclass for this for structured data?
There were no unittests for these classes unfortunately, to test this I'd need to be able to mock the Client instance.

@bram123
Copy link
Contributor Author

bram123 commented Nov 10, 2022

If this change is acceptable, I'd also like to add these stats to the PDOCheck class.
Problem here is that in order to do this, I'd need to execute a query. And I know the syntax for MySQL, but I don't know a generic method to fetch this.
Is a mysql specific implementation for this allowed (and empty result-data for other drivers)?

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Proposed some suggestions: expanding on the success data is very viable, but let's try to improve on the API with this 👍

src/Check/Memcache.php Outdated Show resolved Hide resolved
src/Check/Memcache.php Show resolved Hide resolved
Comment on lines +93 to +97
$serviceData = [
"responseTime" => $responseTime,
"connections" => (int) $stats[$authority]['curr_connections'],
"uptime" => (int) $stats[$authority]['uptime'],
];
Copy link
Member

Choose a reason for hiding this comment

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

Can we perhaps make the type more precise, by having a ServiceData<{responseTime: float, connections: int, uptime: int}> or such? 🤔

src/Check/Memcached.php Outdated Show resolved Hide resolved
src/Check/Memcached.php Show resolved Hide resolved
Signed-off-by: Bram Leeda <bram@123inkt.nl>
Added unittest for Memcache(d) constructor's host/port validation

Had to update psalm-baseline because we are testing invalid input for $host

Signed-off-by: Bram Leeda <bram@123inkt.nl>
…r locally installing the extensions

Signed-off-by: Bram Leeda <bram@123inkt.nl>
@Ocramius Ocramius added this to the 1.19.0 milestone Nov 11, 2022
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM here.

🚢 meanwhile, good improvement!

src/Check/Memcache.php Show resolved Hide resolved
@Ocramius Ocramius changed the title Add responsetime/uptime/connection-count to memcache + redis Add responsetime/uptime/connection-count to memcache + redis checks Nov 11, 2022
@Ocramius Ocramius self-assigned this Nov 11, 2022
@Ocramius Ocramius merged commit 2d19d23 into laminas:1.19.x Nov 11, 2022
@bram123 bram123 deleted the add-redismemcache-server-status branch November 11, 2022 13:50
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.

None yet

2 participants