[TRTLLM-14511][feat] refactor per-request perf metrics for multi-process trtllm-server#16668
[TRTLLM-14511][feat] refactor per-request perf metrics for multi-process trtllm-server#16668reasonsolo wants to merge 8 commits into
Conversation
c0b42bf to
598991a
Compare
402269a to
5d92a7d
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughReplaces the HTTP ChangesServer-side JSONL performance metrics pipeline
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAIClient
participant PerfMetricsMiddleware
participant PerfMetricsJsonlWriter
OpenAIClient->>PerfMetricsMiddleware: send metrics-enabled request
PerfMetricsMiddleware->>OpenAIClient: return headers or SSE metrics event
PerfMetricsMiddleware->>PerfMetricsJsonlWriter: submit completed record
PerfMetricsJsonlWriter->>PerfMetricsJsonlWriter: write JSONL
sequenceDiagram
participant OpenAIHttpClient
participant RawRequestResponseHooks
participant OpenAIDisaggServer
OpenAIHttpClient->>RawRequestResponseHooks: on_perf_metrics(server, role, metrics)
RawRequestResponseHooks->>RawRequestResponseHooks: combine_disagg_metrics
RawRequestResponseHooks->>OpenAIDisaggServer: store perf_metrics_record
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #60916 [ run ] triggered by Bot. Commit: |
|
PR_Github #60916 [ run ] completed with state
|
There was a problem hiding this comment.
Removing /perf_metrics is a breaking change and the PR title should reflect that. Older benchmark_serving.py or run_benchmark_aiperf.sh would now fail I believe.
There was a problem hiding this comment.
I have removed /perf_metrics fetching from benchmark script, and added jsonl file parsing.
There was a problem hiding this comment.
I added save-breakdown feature back to benchmark_serving.py, but it's requiring an extra jsonl dump dir now.
There was a problem hiding this comment.
I understand we kept the functionality but since we removed the /perf_metrics endpoint, it could break clients outside TRT-LLM that are still using that endpoint. Therefore I would prefer if we mark it as a breaking change so it at least gets captured in the release notes.
|
/bot run --disable-fail-fast |
|
PR_Github #61165 [ run ] triggered by Bot. Commit: |
|
PR_Github #61165 [ 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>
d9ff361 to
4f9117c
Compare
Signed-off-by: Lizhi Zhou <1432185+reasonsolo@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #61270 [ run ] triggered by Bot. Commit: |
|
PR_Github #61270 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61508 [ run ] triggered by Bot. Commit: |
Doc: TRTLLM-Serving Performance Metrics Design
Dev Engineer Review
/perf_metricspolling/endpoint usage and replaced it with:return_perf_metrics=trueandX-TRTLLM-return-metrics: 1.[DONE]metrics event (SSE event is suppressed from user-visible streaming output).perf_metrics_output_dirindependent of header return.DisaggServerConfigextended withreturn_perf_metricsandperf_metrics_output_dir;extract_disagg_cfg(...)now propagates/forces downstreamcontext_serversandgeneration_serversto return internal metrics when either gating/JSONL options are enabled.kwargs, and switched to explicit keyword instantiation.tensorrt_llm/serve/_perf_metrics_schema.pywithTypedDictrecords supporting aggregated + disaggregated phases and a unifiedPerfMetricsRecordunion.tensorrt_llm/serve/perf_metrics.pynow owns building metrics records, generating metric headers, SSE emission, and JSONL persistence (PerfMetricsJsonlWriter), plus disagg record combination (combine_disagg_metrics)./perf_metricsimplementation; metrics records are now attached toraw_request.state.perf_metrics_recordand emitted at the correct time (explicitly awaited at end of streaming/non-streaming flows).OpenAIHttpClientnow sets the return-metrics header whenrequest_perf_metricsis enabled, extracts metrics from response headers/SSE payloads, and emits via hooks; removed the old abstract/concretecollect_metrics()plumbing.disagg_request_idhook invocation.fix: allow large disagg metrics headersupdates implementation to support larger disaggregated metrics headers (important given the move to header-carried metrics)./perf_metrics.[DONE]terminates SSE iteration immediately (return), preventing accidental extra chunk processing.CI failures noted in context: Two CI runs failed after the refactor/endpoint removal:
#60916(pipeline#49181, commit5e0ac04)#61165(pipeline#49414, commitd9ff361)Recommendation for investigation: prioritize any tests asserting
/perf_metricsbehavior, then validate header gating correctness and SSE post-[DONE]emission/suppression, and finally confirm JSONL writing/reading expectations (including large-header handling) across aggregated + disaggregated flows.QA Engineer Review
Test code/config changes detected (under
tests/)Integration / config updates
tests/integration/defs/accuracy/test_disaggregated_serving.pyperf_metrics_output_dir(derived from temp dir) instead ofperf_metrics_max_requests./perf_metricspolling helper.tests/integration/defs/disaggregated/test_disaggregated.pyperf_metrics_output_dir, injectreturn_perf_metrics, and validate via JSONL artifacts instead of HTTP/perf_metrics.tests/integration/defs/disaggregated/test_configs/*.yamldisagg_config_conditional_deepseek_v3_v2.yaml: set top-levelreturn_perf_metrics: true(kept/updated rationale comment).disagg_config_metrics.yaml: set top-levelreturn_perf_metrics: true.Test utilities
tests/test_common/perf_metrics_utils.pyperf_metrics-*.jsonlfromoutput_dir.read_perf_metrics_jsonl,wait_for_perf_metrics_jsonl, updatedget_timing_metrics(...).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).tests/unittest/llmapi/apps/test_disagg_perf_metrics_collector.pytests/unittest/llmapi/apps/test_disagg_serving_perf_metrics.pytest_return_perf_metrics_and_jsonl_dump; threadedperf_metrics_output_dirthrough fixtures and server config.tests/unittest/llmapi/apps/test_request_metrics.pytests/unittest/others/test_time_breakdown.pytest_parse_disaggregated_time_breakdowntest_parse_jsonl_file_test_disagg_serving_multi_nodes.pyand_test_disagg_serving_multi_nodes_service_discovery.pyremove perf/timing-metrics plumbing and assertions.API stability reference
tests/unittest/api_stability/references/llm.yamlperf_metrics_output_dirperf_metrics_max_requestsas deprecated.Coverage mapping vs test lists (CI/manual)
tests/integration/test_lists/are mentioned in the provided change list; however, the repo diff couldn’t be validated in this environment. Treat test-list coverage verification as not confirmed.Verdict
needs follow-up — the change is substantial (removing
/perf_metricsendpoint behavior and switching to header/SSE/JSONL), and the context includes two failing CI runs likely tied to metrics gating/emission/JSONL expectations and/or the disaggregated large-header fix.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.