Skip to content

feat(kv-router): stamp request identity on selector scoring rows - #12370

Merged
nv-yna merged 2 commits into
ai-dynamo:mainfrom
nv-yna:yna/kv-router-decision-id
Jul 29, 2026
Merged

feat(kv-router): stamp request identity on selector scoring rows#12370
nv-yna merged 2 commits into
ai-dynamo:mainfrom
nv-yna:yna/kv-router-decision-id

Conversation

@nv-yna

@nv-yna nv-yna commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Overview:

selector.rs emits one tracing::debug! per candidate carrying the whole cost breakdown — overlap_blocks, raw_prefill_blocks, overlap_credit_blocks, overlap_credit_decay, prefill_load_scale, adjusted_prefill_blocks, decode_blocks, and the resulting logit. It is the only place the router explains why it picked a worker. But the row carries no identity, so anything reading these logs can only group candidates into decisions by line adjacency.

This PR stamps request_id and worker_type on those rows, and request_id on the winner rows, making them self-joining. The selector change is +15/−0 and logging-only. The branch also repairs two pre-existing README links to the disaggregated-serving architecture page; their old redirect resolves to HTTP 404 and was failing the repository-wide docs-link check.

Details:

Adjacency is not sound here, for two structural reasons:

  1. The rows are outside the request span. They are emitted from the SchedulerQueueActor task, which scheduling/queue.rs:409 spawns via tokio::spawn(actor.run(admission_rx)) with no .instrument(...). The caller's request span is therefore not in scope, and the logging layer cannot attach x_request_id/trace_id. This is exactly why [ROUTING] Best — emitted on the caller's task at llm/src/kv_router/push_router/selection.rs:183 — does carry x_request_id while these rows do not.
  2. There is one actor per scheduler. A disaggregated deployment runs two (prefill and decode), so two concurrent tasks interleave into one log.

Measured on a 1.24 GB frontend-log sample from a 6-prefill + 1-decode deployment:

metric value
candidate→winner agreement, overall 72.46%
…on 24-candidate prefill decisions 99.82%
…on 8-candidate decode decisions 42.98%
winners with no preceding candidate rows 14.40%

Decode-pool analysis is not currently possible from these logs, and the smaller a pool's candidate set, the more adjacency corrupts it.

The two fields:

  • request_idrequest.mode.request_id() is already reachable at the emission site and returns the same value [ROUTING] Best logs. That makes the candidate row self-joining to the client record in one hop, rather than requiring a bridge through the winner row (which is absent for 14.40% of decisions).
  • worker_type — separates the two pools with no join at all. This matters beyond bookkeeping: the pools score differently. prefill_router/mod.rs's build_decode_router_override forces overlap_score_credit = 0.0 and track_prefill_tokens = false on the decode path, so decode candidates are load-only and have raw_prefill_blocks == 0 by construction. Aggregating the two pools mixes two different scoring regimes.

Alternatives considered:

  • A monotonic decision counter. Rejected — it is a surrogate for a natural key already in hand, it does not survive a process restart, and it collides across router replicas (router_replica_sync), so aggregated logs from two frontends would share ids. It also puts mutable global state in a component scheduling/CLAUDE.md declares side-effect free.
  • Propagating the caller's span across AdmissionCommand::Enqueue. This is the framework-correct fix — it would give x_request_id, trace_id, span_id and parent_id on every event inside selection, permanently. It is a larger, cross-crate change to the actor message type, so it belongs in its own PR. This one is the minimal change that makes the existing rows usable.

Cost: both fields are evaluated inside the macro, so they cost nothing when DEBUG is disabled. When enabled, request_id is a borrowed &str and worker_type a &'static str — nothing is allocated in selector.rs.

No scoring behaviour changes, and no function signatures change.

Where should the reviewer start?

lib/kv-router/src/scheduling/selector.rs contains the code change; README.md contains only the two URL corrections described above. Start with two places in selector.rs:

  1. worker_logit (the two tracing::debug! candidate rows) — confirm the added fields are inside the macro invocation, so they are not evaluated when DEBUG is off, and that request.mode.request_id() introduces no allocation.
  2. select_worker (the three tracing::info! winner rows: pinned, decode, general) — request_id is bound once and reused; confirm it matches what [ROUTING] Best logs so the two rows join.

Worth a skeptical eye: the unwrap_or("-") fallback. ScheduleMode::QueryOnly permits a None request id (prefill_router/query.rs passes None), so those rows are deliberately labelled rather than dropped — an explicit sentinel beats today's silent misattribution by adjacency, but say so if you'd prefer a different sentinel or an omitted field.

Related Issues

🚫 This PR is NOT linked to an issue:

  • Confirmed — no related issue

Validation

  • cargo test -p dynamo-kv-router --lib849 passed, 0 failed, 2 ignored
  • cargo clippy -p dynamo-kv-router --no-deps --all-targets -- -D warnings — clean
  • cargo fmt -p dynamo-kv-router — applied
  • Lychee v0.24.2 with the workflow flags against README.md102 links checked, 0 errors
  • pre-commit run --all-files — all hooks passed
  • No selector test changes were needed, which is itself the behaviour-preservation check: the selector change is additive logging fields only.

@nv-yna
nv-yna requested a review from a team as a code owner July 29, 2026 17:50
@nv-yna
nv-yna temporarily deployed to external_collaborator July 29, 2026 17:50 — with GitHub Actions Inactive
@nv-yna
nv-yna temporarily deployed to external_collaborator July 29, 2026 17:50 — with GitHub Actions Inactive

@devin-ai-integration devin-ai-integration Bot 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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@github-actions github-actions Bot added the feat label Jul 29, 2026
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot added the external-contribution Pull request is from an external contributor label Jul 29, 2026
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Selector logging correlation

Layer / File(s) Summary
Decision identifier and scoring logs
lib/kv-router/src/scheduling/selector.rs
Adds a monotonic per-decision counter and propagates decision_id into worker_logit debug tracing.
Selection log wiring and test updates
lib/kv-router/src/scheduling/selector.rs
Adds decision_id and request_id to pinned and final selection logs, updates scoring calls, and adjusts unit tests for the expanded helper signature.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and matches the main logging change in selector scoring rows.
Description check ✅ Passed The description includes the required sections and clearly states that no related issue is linked.

Comment @coderabbitai help to get the list of available commands.

@datadog-official

datadog-official Bot commented Jul 29, 2026

Copy link
Copy Markdown

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 44.41% (+6.48%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: f19f5d3 | Docs | Datadog PR Page | Give us feedback!

selector.rs emits one `tracing::debug!` per candidate carrying the whole
cost breakdown, but the row has no identity, so offline tooling can only
group candidates into decisions by line adjacency.

Adjacency is not sound here. These rows are emitted from the
`SchedulerQueueActor` task, which scheduling/queue.rs:409 spawns via
`tokio::spawn(actor.run(admission_rx))` with no `.instrument(...)`, so
the caller's request span is not in scope and the logging layer cannot
attach `x_request_id`/`trace_id`. That is why `[ROUTING] Best`, emitted
on the caller task at llm/src/kv_router/push_router/selection.rs:183,
carries `x_request_id` while these rows do not. There is also one actor
per scheduler, and a disaggregated deployment runs two, so prefill-pool
and decode-pool decisions interleave into the same log.

Measured on a 1.24 GB frontend-log sample from a 6 prefill + 1 decode
deployment: candidate-to-winner agreement 72.46% overall, 99.82% on
24-candidate prefill decisions but 42.98% on 8-candidate decode
decisions, and 14.40% of winners with no preceding candidate rows.
Decode-pool analysis is not currently possible from these logs.

`request.mode.request_id()` is already reachable at the emission site and
returns the same value `[ROUTING] Best` logs as `request_id`, so it makes
the row self-joining to the client record in one hop, with no scheduler
state and no new identifier scheme. `worker_type` separates the two pools
without a join, which matters because they score differently: the decode
path runs with `overlap_score_credit = 0` and `track_prefill_tokens =
false` (prefill_router/mod.rs build_decode_router_override).

Both fields are evaluated inside the macro, so they cost nothing when
DEBUG is disabled. When enabled, `request_id` is a borrowed `&str` and
`worker_type` a `&'static str`; nothing is allocated in selector.rs.

No scoring behaviour changes, and no function signatures change.

Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
@nv-yna
nv-yna force-pushed the yna/kv-router-decision-id branch from ebb8bfd to f92280d Compare July 29, 2026 19:04
@nv-yna
nv-yna temporarily deployed to external_collaborator July 29, 2026 19:04 — with GitHub Actions Inactive
@dynamo-ops

Copy link
Copy Markdown
Contributor

/ok to test f92280d

@nv-yna nv-yna changed the title feat(kv-router): stamp a decision id on selector scoring rows feat(kv-router): stamp request identity on selector scoring rows Jul 29, 2026
Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
@nv-yna
nv-yna requested a review from a team as a code owner July 29, 2026 20:38
@nv-yna
nv-yna temporarily deployed to external_collaborator July 29, 2026 20:38 — with GitHub Actions Inactive
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Jul 29, 2026
@dynamo-ops

Copy link
Copy Markdown
Contributor

/ok to test f19f5d3

@nv-yna
nv-yna enabled auto-merge (squash) July 29, 2026 21:22
@nv-yna
nv-yna merged commit ee9687a into ai-dynamo:main Jul 29, 2026
105 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation external-contribution Pull request is from an external contributor feat size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants