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
Add responsetime/uptime/connection-count to memcache + redis checks #57
Conversation
If this change is acceptable, I'd also like to add these stats to the PDOCheck class. |
There was a problem hiding this 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 👍
$serviceData = [ | ||
"responseTime" => $responseTime, | ||
"connections" => (int) $stats[$authority]['curr_connections'], | ||
"uptime" => (int) $stats[$authority]['uptime'], | ||
]; |
There was a problem hiding this comment.
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? 🤔
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>
There was a problem hiding this 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!
Signed-off-by: Bram Leeda bram@123inkt.nl
Description
Added more CheckResult data values:
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.