Capture TensorRT-LLM AgentX server metrics - #2625
Conversation
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
|
Claude finished @cquil11's task in 4m 53s —— View job PR Review
SummaryNo blocking issues found. One 🟡 warning posted inline: the per-endpoint hit rate in What I verified:
Not verifiable in this sandbox (relying on the PR's stated validation): I couldn't execute pytest/ruff (command approval denied), and I couldn't fetch NVIDIA/TensorRT-LLM#12545 to confirm the upstream |
cquil11
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| hit_rate = normalize_fraction( | ||
| gauge_stat( | ||
| metrics, | ||
| "trtllm_kv_cache_hit_rate", | ||
| combine="avg", | ||
| series_filter=series_filter, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🟡 WARNING: Per-endpoint hit rate uses the series max stat while the aggregate uses avg.
Why it matters: gauge_stat's default preferred_keys is ("max", "avg", "total"), so this per-source computation picks each series' peak sampled hit rate whenever aiperf exports both max and avg stats (the aggregate in populate() explicitly passes preferred_keys=("avg", "max", "total")). Per-endpoint prefix_cache_hit_rate in sources will then be systematically higher than, and inconsistent with, the top-level server_gpu_cache_hit_rate. The new test only exports an avg stat, so it doesn't catch this.
Fix:
| hit_rate = normalize_fraction( | |
| gauge_stat( | |
| metrics, | |
| "trtllm_kv_cache_hit_rate", | |
| combine="avg", | |
| series_filter=series_filter, | |
| ) | |
| ) | |
| hit_rate = normalize_fraction( | |
| gauge_stat( | |
| metrics, | |
| "trtllm_kv_cache_hit_rate", | |
| preferred_keys=("avg", "max", "total"), | |
| combine="avg", | |
| series_filter=series_filter, | |
| ) | |
| ) |
There was a problem hiding this comment.
This automated review found only nits (a duplicated helper function and two latent metric-computation edge cases that don't affect the recipes shipped in this PR). Given this introduces a brand-new TensorRT-LLM metrics backend and modifies 6 production recipe configs, a human look is still worthwhile before merging.
Extended reasoning...
Overview
This PR adds a new TrtllmBackend server-metrics adapter (utils/agentic/aggregation/backends/trtllm.py, 244 lines, new file) that parses TensorRT-LLM Prometheus metrics into the shared agentic aggregation schema, registers it in the backend list, updates the drain-wait script to also poll trtllm_num_requests_running/waiting, and flips publish_events_and_metrics/return_perf_metrics to true plus adds AIPERF_REQUIRED_SERVER_METRIC_PREFIX: trtllm_ across all 6 GB300 disaggregated recipe YAMLs. A new unit test (test_processor_normalizes_trtllm_server_metrics) exercises the adapter end-to-end.
Security risks
None identified. This is benchmark tooling parsing Prometheus metrics exports and YAML recipe configs — no user input, auth, or crypto surface is touched.
Level of scrutiny
Medium. The recipe YAML changes are mechanical (flag flips, one new env var, uniform across files) and low risk. The new trtllm.py adapter is genuinely new logic computing several derived metrics (cache hit rate, KV capacity, offload bandwidth) that feed into reported benchmark numbers — correctness bugs here would be silent (wrong numbers, not crashes) and only surface downstream in dashboards. The bug hunting pass found three nits: a duplicated helper (pure cleanup), a block-size/block-count pairing bug in gpu_kv_capacity_tokens that's mathematically latent today because all 6 shipped recipes use uniform tokens_per_block: 64 for both roles, and an inverted primary/fallback precedence for server_gpu_cache_hit_rate that will actually affect these specific imbalanced disagg recipes (1p7d, 3p1d, etc.) since they all enable publish_events_and_metrics. None of these crash or corrupt benchmark request handling, but the cache-hit-rate one does undermine the PR's own stated goal of capturing an accurate cache-hit metric for the topologies it ships.
Other factors
The new test fixture cleverly can't distinguish the two capacity-calculation approaches (both use tokens_per_block: 64 for prefill and decode), so it doesn't catch the block-size pairing issue, and its cache-hit-rate assertion (prefill 0.4, decode 0.6 → 0.5) directly demonstrates the unweighted-average behavior without flagging it as unexpected. No CODEOWNER-restricted paths appear to be touched beyond the standard recipe-reminder bot comment. Given the metric-correctness nits found, a maintainer familiar with the intended weighting semantics should weigh in before merge.
| def _first_counter_total( | ||
| metrics: dict[str, dict[str, Any]], | ||
| metric_names: list[str], | ||
| ) -> float | None: | ||
| for metric_name in metric_names: | ||
| value = sum_stat( | ||
| metrics, | ||
| metric_name, | ||
| preferred_keys=("total", "sum", "max", "avg"), | ||
| ) | ||
| if value is not None: | ||
| return value | ||
| return None |
There was a problem hiding this comment.
🟡 trtllm.py's private _first_counter_total (lines 152-164) is byte-for-byte identical to the already-exported first_counter_total in vllm.py (lines 237-249), which dynamo_vllm.py already imports and reuses. trtllm.py should import the shared helper instead of re-implementing it, so the logic doesn't need to be kept in sync in two places.
Extended reasoning...
utils/agentic/aggregation/backends/trtllm.py (new in this PR) defines a private module-level function _first_counter_total(metrics, metric_names) at lines 152-164. Its body loops over metric_names, calls sum_stat(metrics, metric_name, preferred_keys=("total", "sum", "max", "avg")) for each, and returns the first non-None result.
That is byte-for-byte identical, including the exact preferred_keys tuple order, to the already-exported first_counter_total(metrics, metric_names) defined in utils/agentic/aggregation/backends/vllm.py (lines 237-249). This isn't a coincidental similarity — it's the same helper re-implemented under a private name in a second file.
The reuse path is already established and proven in this codebase: dynamo_vllm.py line 8 does from .vllm import VllmBackend, first_counter_total and calls the shared helper at lines 25 and 29 rather than writing its own copy. trtllm.py already imports from ..aggregation_common and .base in the same style, so from .vllm import first_counter_total would be a drop-in replacement — no signature or behavior changes needed. In TrtllmBackend.populate, both call sites (_first_counter_total(metrics, ["dynamo_frontend_input_sequence_tokens", "trtllm_prompt_tokens_total"]) and the generation-tokens equivalent) would just call the imported first_counter_total instead.
Concretely: today, if someone needs to change the tie-breaking order (e.g. add a "count" fallback key) in vllm.py's first_counter_total, they'd have to remember to also update trtllm.py's private copy, or the two backends will silently diverge in how they resolve ambiguous counter stats — a bug that would only surface as a subtle metric discrepancy between vLLM-based and TRT-LLM-based aggregation runs, not a test failure, since both copies currently behave identically and the existing tests exercise them independently.
Fix: delete _first_counter_total from trtllm.py and replace from .base import ServerMetricsBackend, counter_int with an additional from .vllm import first_counter_total, then update the two call sites in populate to use first_counter_total instead of _first_counter_total.
This is a pure code-reuse/de-duplication cleanup with no behavioral difference — the two implementations already do exactly the same thing, and the PR's own test (test_processor_normalizes_trtllm_server_metrics) would still pass unchanged after the fix.
| def gpu_kv_capacity_tokens( | ||
| self, | ||
| metrics: dict[str, dict[str, Any]], | ||
| server_log_paths: list[Path], | ||
| ) -> int | None: | ||
| del server_log_paths | ||
| max_blocks = sum_stat( | ||
| metrics, | ||
| "trtllm_kv_cache_max_blocks", | ||
| preferred_keys=("max", "avg", "total", "sum"), | ||
| ) | ||
| tokens_per_block = gauge_stat( | ||
| metrics, | ||
| "trtllm_kv_cache_tokens_per_block", | ||
| preferred_keys=("max", "avg", "total"), | ||
| combine="max", | ||
| ) | ||
| if max_blocks is None or tokens_per_block is None: | ||
| return None | ||
| return counter_int(max_blocks * tokens_per_block) |
There was a problem hiding this comment.
🟡 gpu_kv_capacity_tokens sums trtllm_kv_cache_max_blocks across all prefill+decode series but multiplies by only the single largest trtllm_kv_cache_tokens_per_block value, instead of pairing each series' own block count with its own block size. Since tokens_per_block is independently configurable per role in TRT-LLM's kv_cache_config, this overstates capacity whenever prefill and decode use different block sizes (e.g. 100032 + 2000128 = 288000 correct vs sum(3000)*max(128) = 384000, ~33% over); it's currently latent because all 6 shipped recipes use tokens_per_block: 64 for both roles. Fix by summing max_blocks * tokens_per_block per series (as VllmBackend does per-engine) rather than combining sum and max independently.
Extended reasoning...
The bug: TrtllmBackend.gpu_kv_capacity_tokens (utils/agentic/aggregation/backends/trtllm.py:130-149) computes total GPU KV cache token capacity as:
max_blocks = sum_stat(metrics, "trtllm_kv_cache_max_blocks", ...)
tokens_per_block = gauge_stat(metrics, "trtllm_kv_cache_tokens_per_block", combine="max", ...)
return counter_int(max_blocks * tokens_per_block)sum_stat sums the metric value across every series that matches (i.e. across all prefill and decode worker endpoints combined), while gauge_stat(..., combine="max") collapses tokens_per_block down to the single largest value seen across any series. The result is sum(blocks_i) * max(tpb_i), which is only mathematically equivalent to the correct sum(blocks_i * tpb_i) when every series shares the same tokens_per_block.
Why this can be wrong: tokens_per_block lives under trtllm_config.<role>.kv_cache_config.tokens_per_block and is independently configurable per role — this very PR's recipe YAMLs set it separately for prefill and decode (both happen to be 64 in all six shipped recipes). Nothing in the code or schema enforces that prefill and decode use the same block size; a future recipe that tunes them differently (e.g. a smaller block size for prefill's chunked-prefill KV pool vs. a larger one for decode) will silently get a wrong gpu_total_tokens capacity figure.
Concrete proof: Suppose prefill has max_blocks=1000, tokens_per_block=32, and decode has max_blocks=2000, tokens_per_block=128.
- Correct capacity:
1000*32 + 2000*128 = 32000 + 256000 = 288000tokens. - What the code computes:
sum_statgives1000+2000=3000;gauge_stat(combine="max")givesmax(32,128)=128; product =3000*128 = 384000tokens — a ~33% overstatement.
Why nothing catches this today: The new unit test (test_processor_normalizes_trtllm_server_metrics) sets tokens_per_block: 64 for both the prefill and decode series in its fixture, so sum(blocks)*max(64) happens to equal sum(blocks_i*64) — the test can't distinguish correct pairing from the sum/max mixing. All six recipe YAMLs shipped in this PR likewise set tokens_per_block: 64 for both roles, so the bug is latent: today's output is numerically correct, it just isn't computed the way the field names imply, and any future heterogeneous-block-size recipe will get a wrong number without any test or schema check catching it.
Contrast with VllmBackend: VllmBackend.gpu_kv_capacity_tokens avoids this class of bug entirely by summing per-engine token counts (server_log_metrics) that are already the product of blocks and block size per engine, rather than combining an aggregate sum with an aggregate max across engines.
Suggested fix: iterate per-series (e.g. group by endpoint_url similar to _trtllm_sources), compute max_blocks * tokens_per_block for each series using its own series_filter, then sum those per-series products — mirroring the per-engine approach already used by VllmBackend.
Severity: This only affects a reported/telemetry field (gpu_kv_cache.gpu_total_tokens), never benchmark correctness or request handling, and every configuration this PR actually ships uses uniform tokens_per_block: 64, so no incorrect output is produced today. It's worth fixing for robustness before a heterogeneous-block-size recipe lands, but it does not block this merge.
| flat["server_gpu_cache_hit_rate"] = normalize_fraction( | ||
| gauge_stat( | ||
| metrics, | ||
| "trtllm_kv_cache_hit_rate", | ||
| preferred_keys=("avg", "max", "total"), | ||
| combine="avg", | ||
| ) | ||
| ) | ||
| if flat["server_gpu_cache_hit_rate"] is None: | ||
| flat["server_gpu_cache_hit_rate"] = rate(cached_tokens, prompt_total) |
There was a problem hiding this comment.
🟡 In TrtllmBackend.populate (utils/agentic/aggregation/backends/trtllm.py:50-59), server_gpu_cache_hit_rate uses the unweighted gauge_stat(..., combine="avg") across all prefill/decode worker series as the PRIMARY path, only falling back to the token-weighted rate(cached_tokens, prompt_total) when that gauge is absent. This inverts the pattern in vllm.py/sglang.py (token-weighted rate primary, native gauge as fallback), and since these recipes enable publish_events_and_metrics/return_perf_metrics, the gauge will be present in practice — letting the many decode workers (block_reuse disabled) dilute the meaningful prefill signal (block_reuse enabled) in the imbalanced disagg topologies this PR ships (e.g. 1p7d, 3p1d). The same combine="avg" bias is duplicated in _trtllm_sources (~line 210).
Extended reasoning...
The bug: TrtllmBackend.populate computes server_gpu_cache_hit_rate primarily from gauge_stat(metrics, "trtllm_kv_cache_hit_rate", combine="avg"), an unweighted arithmetic mean across every per-endpoint series for that gauge (one series per prefill/decode worker). Only when that gauge is None does the code fall back to the token-weighted rate(cached_tokens, prompt_total) at line 59.
Why this inverts the established pattern: the sibling backends do the opposite. vllm.py computes rate(prefix_hits, prefix_queries) — a sum-of-hits-over-sum-of-queries token-weighted rate — as the primary path, with no unweighted-gauge fallback for this metric. sglang.py computes rate(device_hits, prompt_total) (also token-weighted) as primary, and only falls back to gauge_stat("sglang:cache_hit_rate", combine="avg") when that weighted rate is None. trtllm.py flips this precedence, discarding the more accurate weighted computation whenever the native gauge exists.
Why it manifests in this PR specifically: all 6 new recipes set publish_events_and_metrics: true and return_perf_metrics: true for both prefill and decode engines, which is exactly what causes TensorRT-LLM to emit the native trtllm_kv_cache_hit_rate gauge. That means in production the gauge will never be None, so the unweighted-average path is always taken — the token-weighted fallback is effectively dead code for these recipes. Compounding this, every recipe is a deliberately imbalanced disaggregated topology (1p7d, 3p1d, 2p3d, 3p2d, etc.) where only the prefill engine sets kv_cache_config.enable_block_reuse: true (decode sets it to false). Prefill is where the meaningful cache-hit signal lives, but an unweighted average over per-worker series gives each worker's local hit-rate equal weight regardless of the token volume it handled.
Step-by-step proof (1p7d recipe): suppose the single prefill worker reports trtllm_kv_cache_hit_rate = 0.6 (meaningful, block_reuse enabled) and each of the 7 decode workers reports ~0.05 (block_reuse disabled, so hits are incidental). gauge_stat(..., combine="avg") averages across all 8 series: (0.6 + 7*0.05) / 8 = 0.95/8 ≈ 0.119. The reported server_gpu_cache_hit_rate is ~0.12, even though the actual meaningful signal (prefill) was 0.6 — a 5x understatement driven entirely by how many decode workers happen to exist, not by actual cache behavior. The already-present token-weighted fallback (rate(cached_tokens, prompt_total), summed across all series) would instead reflect the true proportion of tokens served from cache, unaffected by worker count. The test added in this PR (trtllm_kv_cache_hit_rate: prefill avg 0.4, decode avg 0.6 → asserted result 0.5) directly demonstrates the unweighted-mean behavior in the harness.
The fix: swap precedence in populate (and in _trtllm_sources, ~line 210, though there it's scoped to a single endpoint so it doesn't compound across workers) so the token-weighted rate(cached_tokens, prompt_total) is tried first, falling back to the combine="avg" gauge only when token totals are unavailable — matching vllm.py/sglang.py.
Impact: this only skews a reported/benchmark metric (server_gpu_cache_hit_rate), it does not crash, corrupt request data, or affect other benchmark correctness, so it's a nit rather than a merge-blocker — but it does defeat the stated purpose of this PR (capturing an accurate cache-hit metric) for the exact imbalanced topologies it introduces.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31969289712 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=32042452736 |
1 similar comment
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=32042452736 |
|
Closing because the project scope is now limited to porting active, nondeprecated AMD multi-node configurations to srt-slurm. This PR is outside that scope. |
|
Reopening: this PR predates the recent out-of-scope configuration-porting detour and was closed by mistake during an overly broad cleanup. Its prior state is being restored. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Enable native metrics on every Qwen3.5 TRT-LLM prefill/decode worker, require exported trtllm_ series, and normalize token, cache, KV, and offload metrics. 中文：为 Qwen3.5 TRT-LLM 的所有预填充和解码 worker 启用原生指标，要求导出 trtllm_ 序列，并统一归一化 token、缓存、KV 与卸载指标。
cquil11
added
agentx-fast
067677f to
5713868
Compare
cquil11
added
agentx-fast
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5713868. Configure here.
| ) | ||
| if flat["server_gpu_cache_hit_rate"] is None: | ||
| flat["server_gpu_cache_hit_rate"] = rate(cached_tokens, prompt_total) | ||
| flat["server_overall_cache_hit_rate"] = flat["server_gpu_cache_hit_rate"] |
There was a problem hiding this comment.
Unweighted P/D cache hit average
High Severity
Cluster-wide GPU and overall cache hit rates take an unweighted mean of trtllm_kv_cache_hit_rate across every prefill and decode endpoint. Decode workers in these recipes disable block reuse, so decode gauges are not prefix-cache hits; topologies like 1P7D then dilute the real prefill hit rate by the decode count. The token-weighted fallback from cached vs prompt tokens only runs when that gauge is missing, so production AgentX cache metrics come out systematically wrong.
Reviewed by Cursor Bugbot for commit 5713868. Configure here.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33107651693 |
1 participant
Summary
Follow-up to #2612.
trtllm_server-metric exports while retaining srt-slurm v1.0.50’s one-list endpoint injection for disaggregated topologies.Upstream metric surface: NVIDIA/TensorRT-LLM#12545.
Validation
trtllm_./metricsendpoints but emits zerotrtllm_series whilepublish_events_and_metricsis disabled.Note
Low Risk
Changes are limited to benchmark recipes, metrics aggregation, and drain polling; they do not alter inference serving logic beyond enabling metric publication in configs.
Overview
Enables end-to-end TensorRT-LLM Prometheus metrics for GB300 disaggregated AgentX benchmarks and folds them into the existing agentic aggregation schema.
Six Qwen3.5 FP4
agentxsrt-slurm recipes now turn onpublish_events_and_metrics, setreturn_perf_metrics: trueon prefill/decode workers, and require non-empty exports viaAIPERF_REQUIRED_SERVER_METRIC_PREFIX: trtllm_. Post-profile drain inagentic_srt.shalso countstrtllm_num_requests_running/trtllm_num_requests_waitingso idle detection works for TRT-LLM workers, not only vLLM.A new
TrtllmBackendadapter (registered ahead of other backends) mapstrtllm_*and Dynamo frontend token counters into normalized cache/KV/offload/token fields, GPU KV capacity, and per-endpoint prefill/decode sources. Coverage is backed by a processor test and a perf-changelog entry forqwen3.5-fp4-gb300-dynamo-trt-agentic-disagg.Reviewed by Cursor Bugbot for commit 5713868. Bugbot is set up for automated code reviews on this repo. Configure here.