Skip to content

[WIP] docs: add coordinator metrics specification - #2277

Open
revit13 wants to merge 10 commits into
llm-d:mainfrom
revit13:coord-metrices
Open

[WIP] docs: add coordinator metrics specification#2277
revit13 wants to merge 10 commits into
llm-d:mainfrom
revit13:coord-metrices

Conversation

@revit13

@revit13 revit13 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind documentation
/kind feature

PR Description

What this PR does / why we need it:
This PR adds docs/metrics.coord.md as the first step towards instrumenting the coordinator. It defines the metrics that the coordinator will expose, including request latency, pipeline step durations, and upstream sub-requests.

By establishing the metric naming conventions, label sets, and cardinality bounds up front, we can align the design with EPP's existing telemetry before opening implementation PRs.

Which issue(s) this PR fixes:

Part of #2276

@revit13
revit13 requested a review from a team as a code owner August 5, 2026 04:48
@revit13
revit13 requested review from ahg-g and elevran August 5, 2026 04:48
@revit13
revit13 marked this pull request as draft August 5, 2026 04:48
@github-actions github-actions Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 5, 2026
revit13 added 2 commits August 5, 2026 08:18
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
revit13 added 2 commits August 5, 2026 14:31
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
@roytman
roytman requested a review from gyliu513 August 5, 2026 13:42
@roytman

roytman commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Hi Guangya Liu @guygir, can you take a look from an observability point of view?

@revit13
revit13 marked this pull request as ready for review August 5, 2026 13:48
@revit13 revit13 changed the title docs: add coordinator metrics specification [WIP] docs: add coordinator metrics specification Aug 5, 2026
Comment thread docs/metrics.coord.md Outdated

## Labels

Two labels use the words `encode`, `prefill`, and `decode`. They are not interchangeable.

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.

nits: Three labels

@revit13 revit13 Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reworded to name the pair instead of counting: "The step and phase labels share
the values encode, prefill, decode, and conditional-decode, but are not
interchangeable: a step is a pipeline stage, a phase is a single backend call." Thanks

Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Comment thread docs/metrics.coord.md Outdated

| Prefix | Scope |
|---|---|
| `llm_d_coordinator_` | Every coordinator metric: request, pipeline step, upstream phase, disaggregation decision, and decode cache. |

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.

Maybe use llm-d-coordinator_ to keep a single _ as the separator between the subsystem and the name?

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.

we should be in line with llm_d_epp_ and llm_d_router_epp_ metrics:

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.

maybe instead of
"Every coordinator metric: request, pipeline step, upstream phase, disaggregation decision, and decode cache. "
"Canonical, coordinator-wide: " request, response, pipeline step/latency, and other metrics"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hyphens aren't valid in classic Prometheus metric names. Even with Subsystem: "llm-d-coordinator", the text exposition escapes them back, so the scraped name is llm_d_coordinator_request_total either way. Keeping it also matches the EPP subsystem llm_d_epp in this repo, which the coordinator naming mirrors. Thanks

Comment thread docs/metrics.coord.md
| `request_error_total` | Counter | Failed requests; adds label `error_code`. |
| `request_duration_seconds` | Histogram | End-to-end request latency; `generalLatencyBuckets` (5ms to 1h). |
| `request_size_bytes` | Histogram | Request body length; powers-of-2 buckets. |
| `response_size_bytes` | Histogram | Bytes streamed to the client, measured by a counting `ResponseWriter` wrapper in the handler rather than by parsing the body. |

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.

nit: maybe define buckets here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

docs/metrics.md doesn't document buckets for EPP histograms, so I dropped the bucket details here to keep the two docs aligned. Can add exact boundaries to both if you prefer. Thanks

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.

I added this comment because in other place the buckets boundaries were explained, not critical(request_duration_seconds | Histogram | End-to-end request latency; generalLatencyBuckets (5ms to 1h).)

Comment thread docs/metrics.coord.md Outdated
| Name | Type | Notes |
|---|---|---|
| `upstream_request_total` | Counter | Gateway sub-requests, one per call. (e.g. encode contributes one per image). |
| `upstream_request_duration_seconds` | Histogram | Latency of one encode call (encode only). |

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.

nit: maybe define buckets here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

docs/metrics.md doesn't document buckets for EPP histograms, so I dropped the bucket details here to keep the two docs aligned. Can add exact boundaries to both if you prefer. Thanks

Comment thread docs/metrics.coord.md Outdated
Comment on lines +142 to +144
| Request family (`request_total`, etc.) | `llm_d_epp_*` (same names) | Coordinator counts single client requests at entry; EPP counts every sub-request reaching the gateway. EPP adds flow-control labels (`fairness_id`, `priority`). |
| `disagg_decision_total` | `llm_d_epp_disagg_decision_total` | Coordinator counts phases *executed*; EPP counts routing decisions *made* and adds plugin labels. |
| `pipeline_step_*`, `upstream_request_*`, `decode_cache_lookups_total` | None | Unique to coordinator. |

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.

nit: maybe add a prefix to the coordinator’s metrics in the first column to match the second column?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, the comparison table now uses full names in both columns. The catalog above still omits the prefix, since it states it once at the top. Thanks

Comment thread docs/metrics.coord.md Outdated

| Prefix | Scope |
|---|---|
| `llm_d_coordinator_` | Every coordinator metric: request, pipeline step, upstream phase, disaggregation decision, and decode cache. |

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.

we should be in line with llm_d_epp_ and llm_d_router_epp_ metrics:

Comment thread docs/metrics.coord.md Outdated

| Prefix | Scope |
|---|---|
| `llm_d_coordinator_` | Every coordinator metric: request, pipeline step, upstream phase, disaggregation decision, and decode cache. |

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.

maybe instead of
"Every coordinator metric: request, pipeline step, upstream phase, disaggregation decision, and decode cache. "
"Canonical, coordinator-wide: " request, response, pipeline step/latency, and other metrics"

Comment thread docs/metrics.coord.md

### Coordinator metrics endpoint

Every metric on this page is exposed on a single `/metrics` endpoint served by the coordinator

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.

on the metrics port (default 9090, configurable with --metrics-port)

Comment thread docs/metrics.coord.md Outdated
process, alongside the inference paths and `/healthz` and `/readyz`. EPP's metrics endpoint is
separate, served from its own process on the controller-runtime registry.

TBD: the registry the endpoint serves, its address, and whether it is authenticated. EPP authenticates

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.

EPP ??

Comment thread docs/metrics.coord.md Outdated
### Other scrape targets

All coordinator metrics are self-instrumented: the coordinator counts and times its own work
in-process, and scrapes nothing. Two series record a signal that originates upstream but is still

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.

"It is not clear about two series record"
I don't think that decode_cache_lookups_total is a good name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reworded, "two series record a signal that originates upstream" was doing too much work. It now names them directly.
Agreed on the name too. The coordinator doesn't look anything up in a cache; it sends a conditional-decode probe and records the answer. Renamed decode_cache_lookups_total to conditional_decode_probes_total, labels unchanged (result=hit|miss).

Comment thread docs/metrics.coord.md Outdated

#### `decode_cache_lookups_total` (Counter)
* **Labels:** `result` (`hit` or `miss`)
* **Description:** Records the outcome of the conditional-decode probe.

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.

Conditional decode can forward the request to a Decode node (doesn't return 412) in two cases: cache hit, or the input prompt is too short. Coordinator cannot know why the request was sent to a Decode node.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, the labels asserted a reason the coordinator can't see. Changed hit/miss to served/deferred, with served noted as "cached or too short to disaggregate, we can't tell which". Also fixed the error-class bullet that called the 412 a cache miss.

The code has the same assumption (errCacheMiss in decode_proxy.go); renaming it is a code change, so I left it out unless you want it here.

Comment thread docs/metrics.coord.md Outdated
| `disagg_decision_total` | `llm_d_epp_disagg_decision_total` | Coordinator counts phases *executed*; EPP counts routing decisions *made* and adds plugin labels. |
| `pipeline_step_*`, `upstream_request_*`, `decode_cache_lookups_total` | None | Unique to coordinator. |

**EPP-only metrics:** EPP exposes token counts, latencies (TTFT/TPOT/ITL), and metrics for scheduling, flow control, and pool aggregates. The coordinator does not measure these as they are outside its scope.

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.

I think Coordinator can measure it, furthermore, in a deployment with Coordinator, these metrics from EPP are not correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please see my response above on the same topic. Thanks

Comment thread docs/metrics.coord.md Outdated

### Token counts

The coordinator emits no token-count metrics. EPP already parses the vLLM `usage` block to emit `request_input_tokens`, `request_output_tokens`, and `request_cached_tokens`, and the coordinator avoids duplicating this effort. Furthermore, extracting output tokens would require the coordinator to parse the streamed SSE response, defeating the purpose of a pure byte proxy (which is why we use `response_size_bytes`).

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.

request_input_tokens are part of the Renderer response

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please see my response above on the same topic. Thanks

Comment thread docs/metrics.coord.md
* `upstream_request_duration_seconds` is encode-only because it tracks per-image latency. Prefill and decode already have a 1:1 mapping between call latency and step duration, so their timings are tracked solely in `pipeline_step_duration_seconds`.
* There is no `upstream_request_error_total`. A phase-call failure aborts its step and is already counted by `pipeline_step_errors_total`.
* **Conditional-decode:** The probe gets its own phase rather than counting as `decode`, keeping fan-out ratios accurate.

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.

if we work with the "inference/v1/generate" interface, we can add more metrics, especially about multimedia tensors.

Comment thread docs/metrics.coord.md Outdated

### Per-request image visibility

Neither the coordinator nor EPP tracks per-request image count or size. Currently, image count is only visible in aggregate via `upstream_request_total{phase="encode"}`. Image byte size is unreliable because URL images are fetched by `replace-media-urls` and do not traverse the coordinator.

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.

yes, but we have base64 size

@roytman

roytman commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Thank you @revit13 for organising everything together; eventually, this document can be merged with docs/metrics.md

Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Signed-off-by: Revital Sur <eres@il.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants