Conversation
PR #1462 评审:未发现阻塞项,等待人工审核
无阻塞项非阻塞发现
已确认修复
自动代码评审 · head |
2f12934 to
064773d
Compare
064773d to
f373dca
Compare
| }; | ||
|
|
||
| RtpLLMTokenPSMetricsCollector first; | ||
| first.addTokenSize(400, 600, 0, 400, 1000000); |
There was a problem hiding this comment.
[P2] reset 验证测试依赖补丁外的 reporter reset 实现,跨文件契约未在补丁内闭环
checkReporterResetsTokenDeltas 中 first.addTokenSize(400, 600, 0, 400, 1000000) 累计了未打标签的 context delta 400/600,但 expect_window 仅断言 priority 30/50 的 {100,150}/{300,450}(测试第 149 行),未打标签的 400/600 从未出现在期望里。报告循环 RtpLLMMetrics.h:709-717 在 priority_collectors_ 非空时只遍历各 priority 桶、跳过未打标签的 collector_,因此 rtp_llm_context_tokens_delta/rtp_llm_context_tokens_with_cache_delta 的 priority="0" 序列在存在优先级时根本不会上报。生产侧 NormalExecutor.cc:460/465 与 MtpExecutor.cc:1262/1269 分别用 addTokenSize(未打标签,取自 contextExecuteTokenSize)和 addTokenSizeByPriority(各优先级,取自 token_counts_by_priority)独立累计,二者相等这一不变量未被任何代码强制;一旦未打标签总量与各优先级之和出现偏差,偏差部分会被静默丢失。
建议:在同一 PR 内补充 reporter 的 reset 实现(或确认其整体重建 collector 的机制),使新增 delta 字段的 reset 与测试在补丁内闭环。
评审版本:f373dcab165a
| snapshot_time_ms_ += 10000; | ||
| monitor_->GetMetrics(&snapshot, {kmonitor::NORMAL}, snapshot_time_ms_); | ||
| std::map<std::string, std::string> actual; | ||
| for (const auto* record : snapshot.GetRecords().getRecords()) { |
There was a problem hiding this comment.
[P3] expectSnapshot 的 snapshot_time_ms_ 从 0 起每次 +10000,作为 GetMetrics 时间参数可能与墙钟时间戳错位
新增测试夹具成员 int64_t snapshot_time_ms_ = 0;,expectSnapshot 内 snapshot_time_ms_ += 10000; 后调用 monitor_->GetMetrics(&snapshot, {kmonitor::NORMAL}, snapshot_time_ms_)。首次快照传入的时间为 10000(ms,即 1970 年附近)。若 kmonitor 的 GetMetrics 第三参数是时间戳阈值/快照时刻,而指标上报使用系统/steady 时钟(当前墙钟约 1.7e12 ms),则首次快照会因 10000 远小于上报时间戳而取不到任何指标,使 ASSERT_EQ(actual.size(), expected_samples.size()) 恒失败或快照空跑。kmonitor 头文件不在本仓库,无法核实 GetMetrics 语义,故降级为 P3。
建议:核实 kmonitor GetMetrics 第三参数的语义:若为时间戳阈值,应以当前墙钟/单调时钟为基准递增,而非从 0 开始;或改用不依赖时间参数的 drain 型快照 API,避免时间基准错位导致测试空跑或恒失败。
评审版本:f373dcab165a
| })) { | ||
| return false; | ||
| } | ||
| reports.swap(data_reports_); |
There was a problem hiding this comment.
[P2] waitForIdleAfter 的 last_report_was_idle_ 标志在 swap 后未复位,后续窗口的等待可能被陈旧 idle 标志提前满足
waitForIdleAfter 的谓词为 return data_reports_.size() >= count && last_report_was_idle_;,返回前仅执行 reports.swap(data_reports_); idle_report = idle_report_;,last_report_was_idle_ 从不复位。该标志在每次 idle report 时置 true(report() 中 last_report_was_idle_ = collector->reportZeroTPS();)。因此第一个 expect_window 返回后,第二个及后续 expect_window 的等待只需 data_reports_.size() >= count 即可被上窗口遗留的 true 标志满足,不再真正等待当前窗口的 idle 上报。后果:注释声称的 'idle report after the data proves that the real reporting loop drained its collector' 这一 drain 证明对后续窗口失效;若 reporter 在首窗口之后停止发 idle,后续 expect_window 仍会返回 true,掩盖该回归;同时 idle_report_ 可能读到上一窗口的陈旧 idle 值(当前断言恰好因各 idle 内容相同而通过)。
建议:在 reports.swap(data_reports_) 后复位 last_report_was_idle_ = false;(并考虑清空 idle_report_),或引入窗口代次/序号,使谓词要求当前窗口的 idle 必须晚于 count 个 data report 到达。
评审版本:f373dcab165a
| struct TokenDeltaReport { | ||
| std::string priority; | ||
| int64_t context_tokens; | ||
| int64_t context_tokens_with_cache; |
There was a problem hiding this comment.
[P3] RecordingTokenDeltaMetrics::report 对 FindTag 返回值未做空指针防护即构造 std::string
TokenDeltaReport report{tags->FindTag("priority"), collector->contextTokensDelta(), ...} 直接把 FindTag 的返回值用于初始化 std::string 成员。若 reporter 存在不带 priority tag 的调用路径(例如某些 idle 或默认上报路径),FindTag 返回 nullptr 时 std::string(nullptr) 属未定义行为,测试进程会崩溃而非给出可读失败。同文件 expectSnapshot 中 EXPECT_EQ(record->Tags()->FindTag("priority"), priority) 对缺失 tag 只是断言失败不崩溃,但此处构造路径不同。
建议:构造前判空或提供默认值,例如 const auto* p = tags->FindTag("priority"); std::string priority = p ? *p : ""; 再用于初始化 TokenDeltaReport。
评审版本:f373dcab165a
| REPORT_MUTABLE_METRIC(generate_tps_metric, 0.0); | ||
| REPORT_MUTABLE_METRIC(total_tps_metric, 0.0); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[P3] 新增 gauge 依赖后端按 SUM 聚合,需确认指标后端配置已同步
report 中 delta 指标仅在 hasContextTokensDelta()/hasContextTokensWithCacheDelta() 为真时才上报(RtpLLMMetrics.cc:468-473),而这两个条件由 context_token_num>0 决定。decode 稳态窗口 context_token_num==0(如 MtpExecutor.cc:2434 的 addTokenSize(0,0,accepted,accepted,decode_time)),此时既不上报 delta 也不上报 context TPS,两个 gauge 均保留上一 prefill 窗口的旧值。这是台账 P3(仅含 delta 的窗口不清零 TPS)的对称方向:新引入的 delta gauge 在长时间 decode 阶段会持续显示陈旧的 context token 计数,与 'delta'(本窗口变化量)的语义相矛盾。
建议:确认指标后端(kmonitor)对 rtp_llm_context_tokens_delta / rtp_llm_context_tokens_with_cache_delta 已配置 SUM 聚合,与 total_tps 保持一致。
评审版本:f373dcab165a
Existing context TPS gauges divide token counts by time, while
total_tpscan also include decode work. Add two gauges for aggregating prefill token increments directly:rtp_llm_context_tokens_delta: context tokens excluding cached tokens.rtp_llm_context_tokens_with_cache_delta: context tokens including cached tokens, using the existing context-with-cache accounting.Both metrics use the existing priority buckets and reporting/reset lifecycle, including idle zero reports. Positive token increments are retained even when execution timing is zero or negative; the existing TPS numerators and time normalization remain unchanged.
Validation:
//rtp_llm/cpp/metrics:rtp_llm_token_ps_metrics_collector_testwith--config=cuda13: all 19 tests passed, including five new boundary tests. The test harness used container Python for the GPU-lock wrapper and a local GoogleTest archive at the repository-pinned commit after a GitHub fetch timeout.git diff --checkpassed.