Add methods to query health endpoints #29
Conversation
michalismeng
force-pushed
the
health-endpoints
branch
from
7df2b3b to
3d0b074
Compare
There was a problem hiding this comment.
Thanks @michalismeng I added a set of comments/recommendations I think we need to change, but this is a great effort so far. If you're OK with checking out the feedback and doing some updates I think this is definitely something that will help the SDK users.
🙏🏼
|
@michalismeng I should add: please feel free to discuss things on the PR comments, tag me for re-reviews, ask for help etc. I'd be happy to help contribute to your PR/branch if you haven't got the bandwidth at the moment, as this is good and super helpful work! 😄 just let me know. |
michalismeng
force-pushed
the
health-endpoints
branch
from
3d0b074 to
752c7ae
Compare
|
@peteski22 Thanks for your detailed comments and review! Indeed I was a bit busy last week. I think I've addressed most of your comments. Regarding
My initial thought of introducing the Let me know if you'd like any more changes and also feel free to make any improvements as you see fit! |
Thanks again @michalismeng, much appreciated. I’ve forked your fork and opened a PR with a few small tweaks so we don’t end up in long review loops while you’re busy. If it looks good, we can merge it into your repo, then this PR will be ready to go. |
* Move assertions outside pytest.raises() context to ensure execution * Fix assertion to check exception type instead of ExceptionInfo instance * Add length check to ensure test covers all cacheable exceptions * Make patching consistent with rest of file using @patch.object decorator * Add mock reset between test iterations
* Add _SERVER_HEALTH_CACHE_MAXSIZE constant for maintainability * Add proper docstrings for both class constants * Add test to verify constant value * Follows same pattern as _CACHEABLE_EXCEPTIONS
* Add comprehensive test coverage for is_transient() method * Test both transient (timeout, unknown) and non-transient (ok, unreachable) states * Also improve test coverage for is_healthy() method * Add usage example in server_health() docstring showing retry logic pattern
* Add complete docstring for _raise_for_server_health() with Args and Raises sections * Enhance _get_server_health() docs with reference to public API * Improve _get_tool_definitions() with comprehensive exception documentation * Helps developers working on public APIs understand private method contracts
* Only catch ServerUnhealthyError and ServerNotFoundError as 'not healthy' * Let connection/auth/timeout errors propagate to caller * Update docstring to document which exceptions can be raised * Update test to verify new error propagation behavior This improves error visibility - infrastructure issues (connection, auth) are now distinguishable from server health status issues.
* Add threading.RLock to protect cache access * Pass lock to cached decorator for synchronized operations * Wrap cache.clear() and cache.pop() operations with lock * Document thread safety in class docstring The performance impact is negligible since network I/O dominates execution time. Cache stampede risk is minimal due to 10-second TTL and typically small number of MCP servers.
|
Thank you as well @peteski22! Everything looks good, I just added an additional test case for |
What's Changed
Notes
cachetoolslibrary for the caching mechanism (see https://github.com/tkem/cachetools/). The library is actively maintained, MIT-licensed and requires Python 3.9+, so it seems ok for use with the mcpd SDK.Refs #11