[TRTLLM-14511][feat] BREAKING: refactor per-request perf metrics for multi-process trtllm-server - #16668
[TRTLLM-14511][feat] BREAKING: refactor per-request perf metrics for multi-process trtllm-server#16668reasonsolo wants to merge 10 commits into
Conversation
c0b42bf to
598991a
Compare
402269a to
5d92a7d
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces the HTTP ChangesServer-side JSONL performance metrics pipeline
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAIHttpClient
participant OpenAIDisaggServer
participant PerfMetricsMiddleware
participant PerfMetricsJsonlWriter
OpenAIHttpClient->>OpenAIDisaggServer: send metrics-enabled request
OpenAIDisaggServer->>PerfMetricsMiddleware: collect completed request records
PerfMetricsMiddleware->>OpenAIHttpClient: return headers or SSE metrics event
PerfMetricsMiddleware->>PerfMetricsJsonlWriter: submit JSONL record
PerfMetricsJsonlWriter->>PerfMetricsJsonlWriter: write per-process JSONL
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #61892 [ run ] triggered by Bot. Commit: |
|
PR_Github #61892 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61902 [ run ] triggered by Bot. Commit: |
|
PR_Github #61902 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #62043 [ run ] triggered by Bot. Commit: |
|
PR_Github #62043 [ run ] completed with state
|
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
228a883 to
6602b50
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #62355 [ run ] triggered by Bot. Commit: |
|
PR_Github #62355 [ run ] completed with state
|
Doc: TRTLLM-Serving Performance Metrics Design
Dev Engineer Review
/perf_metricspolling/endpoint behavior and moved per-request metrics delivery to:return_perf_metrics=trueandX-TRTLLM-return-metrics: 1).[DONE]metrics event (suppressed from user-visible streaming output).perf_metrics_output_dir.DisaggServerConfigaddedreturn_perf_metricsandperf_metrics_output_dir.extract_disagg_cfg(...)now propagates options intocontext_serversandgeneration_serversand forces downstream returning when eitherreturn_perf_metricsis enabled orperf_metrics_output_diris set.kwargsdirectly; switched to explicit keyword construction forDisaggServerConfig._perf_metrics_schema.pywithTypedDictrecord types supporting aggregated + disaggregated phases and a unifiedPerfMetricsRecordunion.tensorrt_llm/serve/perf_metrics.py(build_request_metrics_record,combine_disagg_metrics, header formatting, SSE event construction).openai_server.pyremoved the legacy in-memory/perf_metricsimplementation and deque/lock buffering; metrics are now built into per-request state and emitted at the correct time (explicitly awaited for both non-streaming and streaming completion).openai_disagg_server.pyreplaced prior collector behavior with queue-latency-aware hooks + conditional collection/persistence, and removed the in-process/perf_metricsroute registration.PerfMetricsMiddlewareandPerfMetricsJsonlWriter, with middleware opting-in header/SSE exposure based onX-TRTLLM-return-metrics: 1and server config.OpenAIHttpClientnow sets return-metrics headers whenrequest_perf_metricsis enabled, extracts metrics from response headers and/or the SSE post-[DONE]metrics payload, and emits via hooks.disagg_request_idvia hooks when regenerated.collect_metrics()plumbing fromOpenAIClient/OpenAIHttpClient.perf_metrics_output_dirinstead of fetching/perf_metrics.[DONE]handling in SSE consumption now exits immediately (return) to prevent extra-chunk processing.CI failures noted in context: Four reported failures—prioritize validating the new gating/emission/JSONL expectations and any removed
/perf_metricsbehaviors:#60916, pipeline#49181, commit5e0ac04#61165, pipeline#49414, commitd9ff361#61270, pipeline#49505, commitdbce2f7#61508, pipeline#49725, commitdbce2f7Recommendation for follow-up investigation: focus on test failures around (1) header/SSE gating conditions, (2) disaggregated record merging and
disagg_request_idpropagation, and (3) JSONL writer output completeness/ordering (including time-breakdown fields), since the/perf_metricsendpoint and its client/scripting usage were removed.QA Engineer Review
Test code/config changes detected (under
tests/)Integration / config updates
tests/integration/defs/accuracy/test_disaggregated_serving.pyperf_metrics_output_dir(temp-derived) instead ofperf_metrics_max_requests./perf_metricsHTTP polling helper and invocation.tests/integration/defs/disaggregated/test_disaggregated.pyperf_metrics_output_dir, validate via local JSONL artifacts, and remove prior/perf_metricsfetch/recording logic.wait_for_perf_metrics_jsonl/JSONL readers rather than the HTTP endpoint.tests/integration/defs/disaggregated/test_configs/disagg_config_conditional_deepseek_v3_v2.yamlreturn_perf_metrics: true.tests/integration/defs/disaggregated/test_configs/disagg_config_metrics.yamlreturn_perf_metrics: true.Test utilities
tests/test_common/perf_metrics_utils.pyperf_metrics-*.jsonlfrom an output directory.read_perf_metrics_jsonl,wait_for_perf_metrics_jsonl.get_timing_metrics(...)to acceptoutput_dir.Unittest coverage
tests/unittest/disaggregated/test_disagg_utils.pytest_extract_disagg_metrics_controls.tests/unittest/llmapi/apps/_test_openai_perf_metrics.pytest_metrics_endpointtest_return_perf_metrics_and_jsonl_dumptest_streaming_metrics_require_request_opt_intests/unittest/llmapi/apps/_test_openai_chat_harmony_perf_metrics.py/perf_metricsdrain/count).kv_cache_time_output_dir.tests/unittest/llmapi/apps/test_disagg_serving_perf_metrics.pyperf_metrics_output_dirtest_return_perf_metrics_and_jsonl_dumpperf_metrics_output_dir.tests/unittest/llmapi/apps/test_request_metrics.pytests/unittest/others/test_time_breakdown.pytest_parse_disaggregated_time_breakdowntest_parse_jsonl_filetests/unittest/disaggregated/test_disagg_openai_client.pytest_internal_client_accepts_perf_metrics_header_sizetest_malformed_streaming_metrics_do_not_fail_requesttests/unittest/llmapi/apps/_test_disagg_serving_multi_nodes.py: removed perf-metrics/timing-metrics plumbing.tests/unittest/llmapi/apps/_test_disagg_serving_multi_nodes_service_discovery.py: removed perf-metrics/timing-metrics plumbing.tests/unittest/llmapi/apps/test_disagg_perf_metrics_collector.py: entire module removed (collector serialization test deleted).Coverage mapping vs test lists (CI/manual)
tests/integration/test_lists/are confirmed from the available context (repo diff inspection returned no discoverable file changes in this environment)./perf_metricsto header/SSE/JSONL), so CI failures may require targeted test-list validation and/or additional waivers.Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.