[Store] feat: expose client metrics HTTP config to Python#2822
Conversation
Lin-z-w
requested review from
ShangmingCai,
XucSh,
YiXR,
stmatengss and
ykwd
as code owners
github-actions
Bot
added
documentation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces client-side HTTP health and metrics endpoints (/health, /metrics, and /metrics/summary) for Mooncake clients, updating the C++ core, Python bindings, configuration parsing, documentation, and tests. The feedback suggests enhancing the robustness of the configuration parsing in real_client.cpp by implementing a trim helper function to strip leading and trailing whitespace from boolean and integer configuration values, preventing parsing failures or unintended fallbacks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| inline bool get_config_bool(const ConfigDict &config, const std::string &key, | ||
| bool default_value) { | ||
| auto it = config.find(key); | ||
| if (it == config.end()) { | ||
| return default_value; | ||
| } | ||
|
|
||
| std::string value = it->second; | ||
| std::transform(value.begin(), value.end(), value.begin(), | ||
| [](unsigned char c) { return std::tolower(c); }); | ||
| if (value == "true" || value == "1" || value == "yes" || value == "on" || | ||
| value == "enable") { | ||
| return true; | ||
| } | ||
| if (value == "false" || value == "0" || value == "no" || value == "off" || | ||
| value == "disable") { | ||
| return false; | ||
| } | ||
|
|
||
| LOG(WARNING) << "Invalid boolean value for config key '" << key | ||
| << "': " << it->second << ", using default: " << default_value; | ||
| return default_value; | ||
| } |
There was a problem hiding this comment.
Configuration values parsed from environment variables or external files can often contain leading or trailing whitespace (e.g., "true " or " 1"). Since get_config_bool performs exact string matching, any leading or trailing whitespace will cause the check to fail and silently fall back to the default value. Introducing a helper function trim to strip whitespace before processing makes the configuration parsing much more robust.
inline std::string trim(const std::string &s) {
auto start = s.find_first_not_of(" \t\r\n");
if (start == std::string::npos) return "";
auto end = s.find_last_not_of(" \t\r\n");
return s.substr(start, end - start + 1);
}
inline bool get_config_bool(const ConfigDict &config, const std::string &key,
bool default_value) {
auto it = config.find(key);
if (it == config.end()) {
return default_value;
}
std::string value = trim(it->second);
std::transform(value.begin(), value.end(), value.begin(),
[](unsigned char c) { return std::tolower(c); });
if (value == "true" || value == "1" || value == "yes" || value == "on" ||
value == "enable") {
return true;
}
if (value == "false" || value == "0" || value == "no" || value == "off" ||
value == "disable") {
return false;
}
LOG(WARNING) << "Invalid boolean value for config key '" << key
<< "': " << it->second << ", using default: " << default_value;
return default_value;
}| inline std::optional<int> get_config_int(const ConfigDict &config, | ||
| const std::string &key, | ||
| int default_value) { | ||
| auto it = config.find(key); | ||
| if (it == config.end()) { | ||
| return default_value; | ||
| } | ||
|
|
||
| try { | ||
| size_t pos = 0; | ||
| int value = std::stoi(it->second, &pos); | ||
| if (pos != it->second.size()) { | ||
| LOG(ERROR) << "Invalid integer value for config key '" << key | ||
| << "': " << it->second; | ||
| return std::nullopt; | ||
| } | ||
| return value; | ||
| } catch (const std::exception &) { | ||
| LOG(ERROR) << "Invalid integer value for config key '" << key | ||
| << "': " << it->second; | ||
| return std::nullopt; | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the trim helper function to strip any leading or trailing whitespace from the integer string before passing it to std::stoi. This prevents parsing failures due to trailing spaces (which would otherwise fail the pos != it->second.size() check).
inline std::optional<int> get_config_int(const ConfigDict &config,
const std::string &key,
int default_value) {
auto it = config.find(key);
if (it == config.end()) {
return default_value;
}
try {
std::string trimmed = trim(it->second);
size_t pos = 0;
int value = std::stoi(trimmed, &pos);
if (pos != trimmed.size()) {
LOG(ERROR) << "Invalid integer value for config key '" << key
<< "': " << it->second;
return std::nullopt;
}
return value;
} catch (const std::exception &) {
LOG(ERROR) << "Invalid integer value for config key '" << key
<< "': " << it->second;
return std::nullopt;
}
}
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| return default_value; | ||
| } | ||
|
|
||
| inline std::optional<int> get_config_int(const ConfigDict &config, |
There was a problem hiding this comment.
Since we already have a ConfigDict type, would it make sense to turn it into a proper class? Then these helper functions could become public methods, rather than living in real_client.cpp.
There was a problem hiding this comment.
Thanks for the suggestion. ConfigDict is currently a public alias used by RealClient::setup_internal and existing C++ callers. Turning it into a class would broaden the scope of this PR and could introduce source and ABI compatibility concerns. Since these parsing rules are specific to client setup, I prefer to keep the helpers local in this PR. We can consider introducing a typed ClientConfig separately if we want to formalize the configuration API.
| try { | ||
| std::string value = trim(it->second); | ||
| size_t pos = 0; | ||
| int parsed_value = std::stoi(value, &pos); |
There was a problem hiding this comment.
Nit: std::from_chars is a more modern alternative and doesn’t throw exceptions.
Here's a some example.
auto [p, ec] = std::from_chars(s.data(), s.data() + s.size(), x);
if (ec == std::errc{}) {
}There was a problem hiding this comment.
Good suggestion. I updated get_config_int to use std::from_chars, checking both the error code and that the entire trimmed value was consumed. I also added regression coverage for trailing characters and out-of-range integer values in commit 72a2b0d.
4 participants
Description
Expose the Mooncake Store client-side health and metrics HTTP endpoint through the Python-facing client setup APIs and the packaged store service configuration.
This change migrates the client Python metrics exposure behavior from Mooncake-taas PR5 and includes the relevant PR9 compatibility fix. It adds:
enable_client_http_serverandclient_http_port.MooncakeConfigJSON/env support viaMOONCAKE_ENABLE_CLIENT_HTTP_SERVERandMOONCAKE_CLIENT_HTTP_PORT.MooncakeDistributedStore.setup./health,/metrics, and/metrics/summary, including disabled-metrics and port-conflict behavior.The client HTTP endpoint remains disabled by default, so existing Python and SGLang integrations keep their current behavior unless they opt in.
Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
git diff --check SKIP=mooncake-code-format pre-commit run --files docs/source/deployment/mooncake-store-deployment-guide.md docs/source/getting_started/observability.md mooncake-integration/store/store_py.cpp mooncake-store/include/client_metric.h mooncake-store/include/dummy_client.h mooncake-store/include/pyclient.h mooncake-store/include/real_client.h mooncake-store/include/types.h mooncake-store/src/real_client.cpp mooncake-store/src/real_client_main.cpp mooncake-store/tests/client_metrics_test.cpp mooncake-wheel/mooncake/mooncake_config.py mooncake-wheel/mooncake/mooncake_store_service.py mooncake-wheel/tests/test_mooncake_config.py mooncake-wheel/tests/test_mooncake_store_service_api.py cmake --build build --target client_metrics_test store build/mooncake-store/tests/client_metrics_test PYTHONPATH=/home/lzw/Mooncake/mooncake-wheel python3 -m unittest mooncake-wheel/tests/test_mooncake_config.py mooncake-wheel.tests.test_mooncake_store_service_api.StoreServiceApiTest.test_start_store_service_passes_tenant_id_to_setup mooncake-wheel.tests.test_mooncake_store_service_api.StoreServiceApiTest.test_cli_config_can_override_tenant_id PATH=/home/lzw/Mooncake/docs/.venv/bin:$PATH make htmlTest results:
Manual validation:
client_metrics_testpassed: 12 tests.make htmlpassed with only sandbox-network intersphinx inventory warnings.mooncake-code-formatwas run through the pre-commit hook and formatted the touched C++ files. It was skipped on the final targeted pre-commit/commit because the hook compares against localorigin/main, which was older than the rebasedupstream/mainand attempted to format unrelated upstream files.Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks passAI Assistance Disclosure
AI assistance: OpenAI Codex was used to inspect the codebase, port the changes, resolve rebase conflicts, run validation, and draft this PR description. I reviewed and edited the generated code before submission.