Repository navigation
Conversation
Preserve upstream transfer events, nonblocking layer submission and dump partitioning. Observe batched dumps per native task and retain address buffers through completion.
Preserve SGLang I/O metrics while forwarding distributed backend load options. Synchronize the SGLang example configuration with the updated KV metrics and cover successful and failed distributed load submissions.
student-jhz
left a comment
student-jhz
left a comment
There was a problem hiding this comment.
整体结论:comment(建议修改后合入)。
理由:核心设计正确且工程质量高——复用既有 accumulator→dispatcher→PrometheusStatsLogger 导出链路,写入 SGLang 的 PROMETHEUS_MULTIPROC_DIR;采集(业务线程只累加)与导出(后台线程)解耦;禁用时零开销;观测失败绝不影响存储操作结果;submit/wait/check 失败语义精确;测试覆盖充分(新增 1061 行,含子进程隔离的真实 multiprocess 导出、ASGI /metrics 聚合、确定性时钟、跨 TP/PP worker 校验);CI 全绿(py-linter/cpp-linter/gtest/e2e-sglang-a2/双语文档检查)。无阻塞性缺陷,但有两个建议合入前解决的 [major]:HiCache 路径 close() 缺最终指标 flush(与 shutdown_metrics_logger 的设计意图不一致,尾部一个 log_interval 窗口的增量永久丢失);用户指南与指标参考双语文档未同步(新增配置面与约 40 个新指标目前只存在于 issue #1617)。
亮点(已逐项核实):
- 禁用态零开销且被测试锁定:disabled/空批次时不读时钟、不计算字节、不导入指标模块,有挂钩 perf_counter 的专测断言;
- 观测绝不改变存储结果:记录路径全异常保护,有 telemetry 抛异常时存储调用仍成功的专测;
- check 失败被 wait 收割时同时计入 wait errors 的双计数行为在指标 documentation 中明示;
- close() 幂等化并保证所有 store 都关闭后再抛首个错误;linker 初始化失败清理链(join 线程→关 adapter→停导出→抛出)完整,均有生命周期专测;
- 配置防呆:显式 metrics_config_path 无效时报错而非静默回退内置;consumers 强制 multiproc-only;重复注册有前置拦截;
- 内置配置与示例 YAML 由测试强制逐字段一致,且验证 DEFAULT_METRICS_CONFIG/vLLM 配置不被污染;
- 合并冲突处理可靠:develop 的 distributed backend load 选项保留有 4 组专测(含提交失败 drain 场景);
- PR 描述五要素齐全、诚实披露 6 个与 develop 基线相同的失败用例及复现 commit;
- 4 个 dashboard JSON 语法合法、引用的 ucm: 指标名全部存在于配置(本次独立核验)。
PR 级意见(无法锚定到具体行):
- [minor] 约束场景要素不完整:描述未说明 SGLang 侧需提供 HiCacheStorageConfig.enable_storage_metrics / CacheInitParams.enable_metrics 字段(依赖配套 SGLang 版本)、PROMETHEUS_MULTIPROC_DIR 需在导入 prometheus_client 前由 SGLang 设置、同进程仅支持一个 metrics-enabled 集成。建议补 Constraints 小节。
- [nit] commit message 采用 feat(sglang)/test(sglang) 风格,develop 主流为 [Feat]/[Fix] 方括号前缀(develop 亦有少量 conventional 先例,squash 后以带 [Feat] 的 PR 标题为准,仅提示)。
- PR 标题带 [Feat] 前缀、一句话意图清晰,Purpose/Modifications/Test/Documentation 四段齐全,单一主题无混入,符合规范,无需修改。
| logger.exception("Failed to close UCM store") | ||
| finally: | ||
| if self._metrics_logger is not None: | ||
| self._metrics_logger.shutdown() |
There was a problem hiding this comment.
[major] close() 仅调用 self._metrics_logger.shutdown()(停轮询线程),未做最终 drain:本 PR 专门实现的 shutdown_metrics_logger()(metrics.py:73,停线程后 drain_to_consumers() + update_stats() 导出最后增量)只被 Direct Linker 的 close() 使用,HiCache 路径的 close() 与 from_hicache 失败清理(第 237 行)都没有用。最后一个 log_interval(默认 5s)窗口内累加的计数/直方图增量不会写入 multiproc 目录,Prometheus counter 是累计值,最终抓取值会被永久低估(benchmark 结束读数的场景直接受影响)。建议两处均改为 shutdown_metrics_logger(self._metrics_logger)(ucm_connector.py 已 import 该模块,补充导入选中项即可)。
| @@ -0,0 +1,1095 @@ | |||
| # SGLang UCM metrics: shared by HiCache and direct Linker integrations. | |||
There was a problem hiding this comment.
[major] 文档同步缺失:本次新增了完整的用户侧配置面(--enable-metrics 前置、PROMETHEUS_MULTIPROC_DIR 由 SGLang 设置、enable_metrics/metrics_config_path 两个 UCM 键及其在 HiCache 与 Direct Linker 下不同的放置规则、同进程单 metrics 集成互斥、约 40 个 sglang_connector_/sglang_linker_ 新指标),但已核实 docs/docs-site/docs/{en,zh}/user-guide/observability/metrics.md 目前仅描述 vLLM 接入,metrics-reference.md 指标目录亦无 sglang_* 条目;这些内容目前只存在于 issue #1617,issue 不是长期维护的用户文档。建议:在 docs/docs-site/docs/{en,zh}/user-guide/observability/metrics.md 增补 SGLang 小节(启用方式、两条接入路径的键放置规则、标签与 ucm: 命名空间、前置条件与互斥约束、dashboard 使用),并在 metrics-reference.md(en/zh 成对,CI Docs·Bilingual files 会校验)增补 sglang_* 指标目录。
|
|
||
| return UnifiedCacheStoreConfig(module_path=module_path, name=name, config=cfg) | ||
| return UnifiedCacheStoreConfig( | ||
| module_path=module_path, name=name, config=cfg, launch_config=dict(kvc) |
There was a problem hiding this comment.
[minor] ucm_connectors 列表布局下文档级 metrics 键被丢弃,与 Direct Linker 行为不一致:load_from_config 在该布局用 connectors[0] 覆盖 kvc 后,launch_config=dict(kvc) 丢失 YAML 顶层的 enable_metrics/metrics_config_path(enable_metrics 缺省回退 True 导致显式关闭失效;metrics_config_path 被静默忽略)。而 UcmDirectStoreAdapter.from_config(ucm_direct_store_adapter.py:205-209)会显式合并文档级键——同一份 ucm_config.yaml 顶层放 enable_metrics: false 时 Linker 生效、HiCache 不生效。该差异仅在 issue #1617 中说明,仓内无测试覆盖(test_sglang_metrics.py 只测内联扁平布局,test_sglang_hicache_config.py 本 PR 未改)。建议:在 unwrap 前提取文档级 enable_metrics/metrics_config_path 并入 launch_config(条目内键优先),统一两条路径行为;若维持现状,至少在 test_sglang_hicache_config.py 补两种布局下 launch_config 的断言,并在用户指南显著说明差异。
| try: | ||
| close = getattr(store, "close", None) | ||
| if callable(close): | ||
| close() |
There was a problem hiding this comment.
[minor] 失败清理中 close() 抛异常会遮蔽原始构造异常:此处 close() 若抛出,新异常会替换原始的构造失败异常(原始异常仅作为 context 链在后面),排障根因不直观。建议清理动作单独兜底:try: close() except Exception: logger.warning("Failed to close store during cleanup", exc_info=True),确保随后的 raise 恢复原始异常。
| thread.join() | ||
| try: | ||
| if hasattr(self, "adapter"): | ||
| self.adapter.close() |
There was a problem hiding this comment.
[minor] 与 ucm_connector.py 的清理路径同型问题:except Exception 清理块中 self.adapter.close() 抛异常会替换原始初始化异常。建议对 adapter.close() 单独 try/except 记 warning,再走 finally: shutdown_metrics_logger(...) 与 raise,确保原始异常不被遮蔽。
| config = get_default_sglang_metrics_config() | ||
| if not get_metric_definitions(config): | ||
| raise ValueError(f"No UCM metrics defined in {config_path}") | ||
| if not consumer_enabled(config, "multiproc") or consumer_enabled( |
There was a problem hiding this comment.
[nit] consumer 名称使用字符串字面量:ucm/metrics_config.py:36-37 已定义 MULTIPROC_CONSUMER 与 VLLM_CONNECTOR_CONSUMER,observability.py 均使用常量,此处建议 from ucm.metrics_config import MULTIPROC_CONSUMER, VLLM_CONNECTOR_CONSUMER 并替换;下方 get_stats_and_clear("multiproc")(第 83 行)同理。
| ) | ||
| from prometheus_client import values | ||
|
|
||
| if not getattr(values.ValueClass, "_multiprocess", False): |
There was a problem hiding this comment.
[nit] 依赖 prometheus_client 私有属性 values.ValueClass._multiprocess 判定 multiprocess 模式,版本升级可能移除。已有 getattr 默认值兜底(误判时会给清晰报错),风险可控,建议至少加注释说明该属性出自 prometheus_client/values.py 的 MultiProcessValue 并写明验证过的版本号。
| raise RuntimeError( | ||
| "PROMETHEUS_MULTIPROC_DIR must be set before importing prometheus_client" | ||
| ) | ||
| from ucm.observability import PrometheusStatsLogger, _metric_mappings |
There was a problem hiding this comment.
[nit] 跨模块导入 ucm.observability 的私有符号 _metric_mappings,依赖实现细节(若 observability 将来重绑定该变量,检查会失效)。建议在 ucm/observability.py 增加公共判定函数,如 def metrics_registered() -> bool: return bool(_metric_mappings),SGLang 侧改用该函数。
| """Stop polling, then export the last deltas after all Store work has drained.""" | ||
| if metrics_logger is None: | ||
| return | ||
| metrics_logger.shutdown() |
There was a problem hiding this comment.
[nit] metrics_logger.shutdown() 位于 try 之外:该函数目标是尽力刷出尾部数据,但 shutdown()(含线程 join)一旦抛异常会跳过后续 drain,且异常会从 close() 的 finally 传播出去。建议将 shutdown() 一并移入 try,或在 try 前单独 try/except 记 warning。
|
|
||
|
|
||
| class DashboardTest(unittest.TestCase): | ||
| def test_dashboard_metric_names_and_panel_ids(self): |
There was a problem hiding this comment.
[nit] DashboardTest 只校验 sglang-linker-dashboard.json;sglang-ucm-store-dashboard.json 同样大量引用 ${ucm_prefix} 指标(本次已人工核验全部存在于配置),但无自动防护,后续指标改名会静默失效。建议将校验逻辑参数化,覆盖 sglang-ucm-store-dashboard.json(hicache dashboard 引用 SGLang 原生指标,可豁免)。
Purpose
Add UCM metrics support for SGLang's HiCache and Direct Linker integrations, exposing storage and scheduling metrics through SGLang's existing Prometheus
/metricsendpoint.Closes #1617.
Modifications
PROMETHEUS_MULTIPROC_DIR, using theucm:namespace withmodel_nameandworker_idlabels.Test
Hardware validation:
Automated validation:
a03985fa), covering configuration consistency, KV metric-count expectations and vLLM layerwise behavior.git diff --checkpassed. Black and isort checks passed for the conflict-resolved adapter and related test files.Documentation
Configuration, usage examples and metric semantics are described in #1617.
Added:
examples/metrics/metrics_configs_sglang.yamlexamples/metrics/sglang-dashboard.jsonexamples/metrics/sglang-hicache-dashboard.jsonexamples/metrics/sglang-linker-dashboard.jsonexamples/metrics/sglang-ucm-store-dashboard.jsonNo Markdown paths under the bilingual documentation trees are changed.