Skip to content

fix(metrics): make per-request context gauges opt-in via CLI - #2836

Merged
valarLip merged 2 commits into
mainfrom
lirzhang/fix-metrics-request-cardinality
Oct 30, 2026
Merged

valarLip merged 2 commits into
mainfrom
lirzhang/fix-metrics-request-cardinality

Conversation

@ZhangLirong-amd

@ZhangLirong-amd ZhangLirong-amd commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Long-running serving and PD benchmarks accumulate Prometheus time series in two per-request context gauges:

  • atom:prefill_request_context_tokens
  • atom:decode_request_context_tokens

Their labels include request_id, sequence_id, and started_at, so each newly observed request sequence creates a distinct series for that phase. Samples are intentionally retained after completion so periodic scrapes can capture short requests and reports can show exact per-request context lengths. There is no automatic expiry. As a result, series count grows with request history rather than active concurrency; in native multiprocess mode, the retained samples also grow the Prometheus mmap files and scrape payloads.

This change makes both diagnostic gauges disabled by default for standalone serving, Prefill services, and Decode services. When disabled, the gauges are not registered and their per-request samples are not allocated or written.

To collect these diagnostics, add the server CLI flag:

--enable-request-context-metrics

For PD deployments, pass the flag separately to each P or D service whose diagnostics are needed. Python callers can set enable_request_context_metrics=True on LLM(...). The setting is propagated through EngineArgs and Config to the schedulers; it does not introduce an environment-variable switch.

Aggregate metrics remain enabled, including queue and batch metrics, context and latency histograms, and cache counters. These retain the observability needed to diagnose queueing, latency, and cache behavior without allocating a series per request. The independent GPU timing option is unchanged.

Opting in preserves the existing retention behavior, so use it for short diagnostic captures rather than sustained throughput runs. Disabling the option does not purge samples already stored in an existing metrics directory; restart with a fresh metrics directory when a clean capture is needed, without deleting storage used by live workers.

Validation on main 124c3b0 plus this change:

  • 89 focused tests passed in a CPU-only container, including 10,000-request checks of series identity and actual multiprocess mmap file size, default and explicit disabled settings, opt-in observations, CLI-to-engine propagation, and standalone/Prefill/Decode scheduler configuration.
  • Black, Ruff, and git diff --check passed.

Signed-off-by: ZhangLirong <219599949+ZhangLirong-amd@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 30, 2026 03:19
@github-actions

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every eligible PR before approval:

  • ✅ Pre Checkin: Black, Ruff, catalog schema validation, non-GPU unit tests

Heavy model tests:

  • ✅ Run after the PR is approved and Pre Checkin passes
  • ✅ Run immediately when an approval review is submitted
  • ✅ Can be requested before approval with labels
Label Tests
ci:full Run all heavy PR model tests: native ATOM, vLLM, and SGLang
ci:atom Run native ATOM model accuracy tests
ci:vllm Run ATOM vLLM OOT model accuracy tests
ci:sglang Run ATOM SGLang model accuracy tests

Heavy jobs are skipped when the PR is not approved and no matching ci:* label is present.
Add labels via the sidebar or gh pr edit 2836 --add-label <label>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues remain for the change making per-request context gauges opt-in.

No open issues 🎉

What changed in this PR

Makes ATOM’s per-request context gauges opt-in, preventing request-history growth in default Prometheus series and native multiprocess storage while preserving aggregate metrics.

Changes:

  • Adds ATOM_ENABLE_METRICS_REQUEST_CONTEXT, disabled by default, to gate gauge registration and per-request observations.
  • Adds 10,000-request cardinality and storage regression checks; explicitly enables gauges in existing diagnostic tests.
  • Documents bounded diagnostic use and safe cleanup of retained samples.
File Description
tests/​test_scheduler_metrics.py Enables request-context gauges for existing observation tests.
tests/​test_scheduler_metrics_cardinality.py Checks stable series and mmap storage with gauges disabled.
tests/​test_multiprocess_metrics.py Enables diagnostic gauges in multiprocess export tests.
docs/​metrics_guide.md Explains opt-in behavior, retention, and cleanup.
docs/​environment_variables.md Documents the new flag and its default.
atom/​utils/​envs.py Registers the request-context metrics flag.
atom/​metrics/​scheduler.py Gates gauge creation and skips disabled observations.

Review effort: Balanced

Signed-off-by: ZhangLirong <219599949+ZhangLirong-amd@users.noreply.github.com>
@ZhangLirong-amd ZhangLirong-amd changed the title fix(metrics): make per-request context gauges opt-in fix(metrics): make per-request context gauges opt-in via CLI Oct 30, 2026
Copilot AI balanced review requested due to automatic review settings October 30, 2026 03:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The metrics guide refers callers to the unavailable LLM(...) API instead of the public LLMEngine(...) entry point needed to enable the option.

1 open issue

Review effort: Balanced

Comment thread docs/metrics_guide.md
capture, pass `--enable-request-context-metrics` when starting the server.
For PD deployments, pass it separately to each P/D service to be diagnosed.
The option is disabled by default for both roles and standalone serving.
Python callers can set `enable_request_context_metrics=True` on `LLM(...)`.

@Jasen2201 Jasen2201 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Making the two per-request context gauges opt-in is appropriate: it bounds default metric cardinality while preserving aggregate observations. I found no blocking issue in the scheduler configuration propagation or recording paths.

Locally verified all four 10,000-request cardinality probes (ordinary/mmap storage, default/explicitly disabled), opt-in first-dispatch observations with unchanged aggregate values, and three related report tests. GPU end-to-end tests were not run locally.

Non-blocking documentation nits: update the Agentic report instructions to explain how to opt in for request-context panels, and use LLMEngine(...) rather than LLM(...) in the Python example, as already noted in the existing review comment.

@valarLip
valarLip merged commit 124684b into main Oct 30, 2026
95 of 97 checks passed
@valarLip
valarLip deleted the lirzhang/fix-metrics-request-cardinality branch October 30, 2026 08:06
ChengYao-amd added a commit to ChengYao-amd/ATOM that referenced this pull request Nov 4, 2026
Under AgentX load, both API processes of an ATOM PD deployment spent ~45% of
the wall clock in stop-the-world gen-2 GC passes, and each pass grew with the
requests served (~0.65 s per pass after an hour). A pass freezes the event
loop, so admission, the prefill hand-back and SSE delivery all wait on it.

CoreManager.outputs_queue grew with every completed request. The output
thread put every batch of finished sequences on it, but only
LLMEngine.step()/generate() and asyncio mode read it; the OpenAI server and
the atomesh standalone service deliver through the per-sequence stream
callbacks. So every finished Sequence stayed reachable for the life of the
process, as RequestOutputs did on stream_outputs_queue before.

Whether the queue has a reader is now declared: consumes_outputs_queue
starts True in _init_shared_state, both callback-only entrypoints clear it
as they wire up, and the output thread queues only while it is set.
get_output()/get_output_async() raise when it is cleared instead of waiting
on a queue nothing fills. _asyncio_mode is set only in _init_shared_state,
and both managers derive async_output_queue from it.

The per-request context gauges were the other source of growth; ROCm#2836 made
them opt-in with --enable-request-context-metrics. The metrics guide's table
rows for them now say so.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants