Skip to content

feat: add context token delta metrics - #1462

Open
jhuangxue wants to merge 1 commit into
alibaba:mainfrom
jhuangxue:codex/context-token-deltas
Open

jhuangxue wants to merge 1 commit into
alibaba:mainfrom
jhuangxue:codex/context-token-deltas

Conversation

@jhuangxue

Copy link
Copy Markdown

Existing context TPS gauges divide token counts by time, while total_tps can 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:

  • Built the metrics library and ran //rtp_llm/cpp/metrics:rtp_llm_token_ps_metrics_collector_test with --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.
  • Verified that all three PR files exactly match the files used in that build and test run.
  • git diff --check passed.

netaddi
netaddi previously approved these changes Sep 24, 2026
Comment thread rtp_llm/cpp/metrics/RtpLLMMetrics.h
Comment thread rtp_llm/cpp/metrics/RtpLLMMetrics.cc Outdated
Comment thread rtp_llm/cpp/metrics/RtpLLMMetrics.h
@rtp-llm-review-bot

rtp-llm-review-bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

PR #1462 评审:未发现阻塞项,等待人工审核

  • 标题:feat: add context token delta metrics(@jhuangxue,open)
  • 评审版本:f373dcab165a

无阻塞项

非阻塞发现

已确认修复

  • [P2] 新增 delta 字段的 report 后重置逻辑未在补丁中体现,若 reset 未同步更新会导致增量指标跨窗口累积、SUM 聚合下重复计数 — reportLoop 在每次上报后通过 collector_ = CollectType()(第719、727行)整体重建 collector,delta 字段 context_tokens_delta_/context_tokens_with_cache_delta_ 随对象重建清零,不存在逐字段遗漏导致的跨窗口累积,发现所担心的 SUM 重复计数不成立。
  • [P2] 新增测试断言了补丁外 reporter 的 delta 重置行为,但重置实现未随补丁更新 — RtpLLMMetrics.h 第 719/727 行 reportLoop 用 collector_ = CollectType(); 整体重建 collector(非逐字段清零),新增 delta 字段随默认构造一并重置,作者主张成立。
  • [P3] expectSnapshot 硬编码 kmonitor GAUGE 序列化格式,测试与 kmonitor 内部实现强耦合 — 改动块(当前文件第53–58行)用 kmonitor::MinMaxCalculator 的 Add/ToString 替换了原先硬编码 std::replace(...,'_',' ') + istringstream 解析五字段的逻辑,测试不再自行复制 kmonitor 的序列化格式。

自动代码评审 · head f373dcab165a · 未发现阻塞项,等待人工审核

@CLAassistant

CLAassistant commented Sep 26, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread rtp_llm/cpp/metrics/RtpLLMMetrics.cc
Comment thread rtp_llm/cpp/metrics/RtpLLMTokenPSMetricsCollectorTest.cc
Comment thread rtp_llm/cpp/metrics/RtpLLMTokenPSMetricsCollectorTest.cc Outdated
Comment thread rtp_llm/cpp/metrics/RtpLLMTokenPSMetricsCollectorTest.cc
@jhuangxue
jhuangxue force-pushed the codex/context-token-deltas branch from 064773d to f373dca Compare September 26, 2026 12:23
};

RtpLLMTokenPSMetricsCollector first;
first.addTokenSize(400, 600, 0, 400, 1000000);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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_);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants