Repository navigation
Conversation
|
👋 Hi sbak5! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
2dd4532 to
4618a7c
Compare
37b8d8c to
62170d1
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughAdds lifecycle telemetry across ingress, preprocessing, response streaming, router queueing and selection, and request dispatch. Spans record stage outcomes and, in investigation mode, additional checkpoints, counts, and selection details. ChangesLifecycle telemetry
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to
|
jh-nv
left a comment
There was a problem hiding this comment.
Tested bfd2be4 locally (CPU only, OTEL collector with file export; mocker, dynamo.common.backend.sample_main, and a small Python echo worker; KV and round-robin routers; agg and disagg; core, investigation, and disabled modes).
Works as described:
- Queue:
deferred=true,reason=prefill_capacity, thendequeued. A client leaving while queued givescancelled/request_lifecycle_ended_before_admission. - Selection: candidate/filter counts, selected/best/score/margin, and investigation
candidates.top_k(4 rows with cache/load inputs across 2 workers x 2 DP ranks). - Dispatch and admission: accepted with
tcpand payload bytes. A response-plane mismatch (QUIC worker) gives admissionrejected, dispatchfailed, rootrejected. - Preprocessing rejection, mid-stream client abort (
response.streaming=abandoned, rootcancelled), both disagg legs, core-mode field trimming, and disabled mode all behaved as expected. - Unit tests: kv-router lifecycle 16/16, runtime 25, llm 334 (filtered to the touched areas) pass.
The main problems are trace linkage (inline): the router spans are not part of the request's trace, and request.dispatch never finds the request's LifecycleTrace because migration drops it, so it creates a new identity and can emit spans with no root.
| .request_id() | ||
| .map_or_else(tracing::Span::none, |id| { | ||
| LifecycleTrace::frontend_request_without_session(id) | ||
| .start(LifecycleStage::RouterQueue) |
There was a problem hiding this comment.
router.queue and router.selection end up as separate traces.
These spans are started inside the queue actor task (tokio::spawn(actor.run(...)) at L665), where no request span is current, with a fresh frontend_request_without_session identity. In a local OTEL run (KV router, investigation mode), every router.queue span here and router.selection span at L1718 was exported as a parentless one-span trace with its own dynamo.operation.id. They can only be joined to request.lifecycle by dynamo.request.id, so they don't show up in the request's trace in Tempo/Jaeger.
Suggested fix: in SchedulerQueue::enqueue (still on the request task), capture tracing::Span::current() and the request's LifecycleTrace, carry both in QueuedRequest, and start both spans from that trace inside parent.in_scope(...).
There was a problem hiding this comment.
Good catch, thanks. Fixed in the next push. enqueue* and select_without_admission now capture the request's LifecycleTrace together with Span::current() while still on the request task, and carry both through the admission command into QueuedRequest. The actor runs enqueue handling, queued admission, and advisory selection under that span and trace, so router.queue and router.selection are children of the caller's kv_router.schedule span and share the root's request and operation IDs.
The trace reaches the kv-router through LifecycleTrace::scope, a task-local scoped around the selection future in kv.rs, so ScheduleRequest/SchedulingRequest stay unchanged. lifecycle_concurrent_queue_outcomes now asserts the parent for queued, cancelled, and expired requests, including a selection made in a later actor turn.
In a 2P+2D GH200 run (AgentX, concurrency 64, 900 s), all 598 router.queue and 598 router.selection spans were in their request's trace under kv_router.schedule, with the root's operation ID.
| .flatten() | ||
| .map(|trace| trace.as_ref().clone()) | ||
| .unwrap_or_else(|| { | ||
| LifecycleTrace::frontend_request_without_session(context_id.clone()) |
There was a problem hiding this comment.
This lookup misses on the normal chat path, so request.dispatch always gets a new identity.
Migration sits between the preprocessor and the router, and migration.rs:516 rebuilds the request with Context::with_id_and_metadata(...), which starts an empty registry. It re-inserts route trace and session affinity but not LIFECYCLE_TRACE_CONTEXT_KEY. The prefill leg has the same problem in independent_prefill_context (prefill_router/mod.rs:583). Locally, every request.dispatch (agg KV, disagg KV, round-robin) had a different dynamo.operation.id from its request.lifecycle root.
The fallback also creates orphan spans for endpoints that never open a root: one /v1/completions request produced router.queue, router.selection and request.dispatch with no request.lifecycle. That goes against the preprocessor's rule at preprocessor.rs:7082 ("Never invent an orphan capture").
Suggested fix: carry the trace through migration the same way as SESSION_AFFINITY_CONTEXT_KEY (and in independent_prefill_context), and fall back to LifecycleTrace::new(false) here and in builtin.rs:375. The queue.rs router spans should be gated on that same trace rather than on request_id().
There was a problem hiding this comment.
Fixed in the next push. Migration now keeps the trace next to session affinity and re-inserts it into each attempt's context, and independent_prefill_context copies it to the prefill leg. The dispatch fallback is gone: request.dispatch starts only from the request's trace, and the router spans are gated on that same trace instead of request_id(). A /v1/completions request now emits no router or dispatch spans (checked in a mock run).
In the GH200 run, all 598 request.dispatch spans (299 prefill + 299 decode) are children of request.lifecycle with the root's operation ID, and every lifecycle span (4445) sits in a trace with a root.
| .ok() | ||
| .flatten() | ||
| .map(|trace| trace.as_ref().clone()) | ||
| .unwrap_or_else(|| LifecycleTrace::frontend_request_without_session(context_id)); |
There was a problem hiding this comment.
Same as the comment in kv.rs: the trace is dropped by migration before it gets here, so this always invents a new identity. Falling back to LifecycleTrace::new(false) avoids orphan spans.
There was a problem hiding this comment.
Fixed with the same change: builtin dispatch uses the shared helper, so it no longer invents an identity.
| metadata.target() == dynamo_runtime::telemetry::LIFECYCLE_TARGET | ||
| && metadata.name() == "router.selection" | ||
| }); | ||
| (env_is_truthy(DYN_LIFECYCLE_TRACE_ENABLED) && is_router_selection).then(|| { |
There was a problem hiding this comment.
Env vars are read on every selection, even with tracing off.
env_is_truthy(DYN_LIFECYCLE_TRACE_ENABLED) runs before the cheap is_router_selection check, and L360 calls std::env::var again. Each read takes the process env lock and allocates a String on the routing hot path. It can also disagree with the lifecycle config that dynamo_runtime::telemetry caches once per process (LIFECYCLE_ENABLED / LIFECYCLE_MODE).
Suggested fix: check is_router_selection first, then use the cached lifecycle_tracing_enabled() / lifecycle_mode() (make them pub; they are currently pub(crate) / private).
There was a problem hiding this comment.
Fixed in the next push. The router.selection metadata check now runs first, and there are no env reads. Investigation detail uses the cached telemetry::lifecycle_mode() (now pub). An enabled check isn't needed any more, since router.selection is only opened for enabled traces.
| ); | ||
| self.span | ||
| .record("dynamo.router.selected.score", selected.cost); | ||
| if let Some(best) = candidates.iter().min_by(|left, right| { |
There was a problem hiding this comment.
Minor: on equal scores, best is the lowest worker key, while the selector can pick a different worker among the ties. So best.worker.id can differ from selected.worker.id with best.margin = 0 (seen locally in a core-mode run). An analysis could read that as the router skipping the best worker. Consider preferring selected as best when its cost equals the minimum.
There was a problem hiding this comment.
Agreed, fixed in the next push. When the selected candidate's cost equals the minimum, it's reported as best (margin 0). I added a tie case to lifecycle_selection_evidence. In the GH200 run, all 102 zero-margin selections report best == selected.
| .unwrap_or_else(|| { | ||
| LifecycleTrace::frontend_request_without_session(context_id.clone()) | ||
| }); | ||
| let request_dispatch = lifecycle.start(LifecycleStage::RequestDispatch); |
There was a problem hiding this comment.
Minor: in disagg, the prefill and decode request.dispatch spans carry no phase, so only the worker ID tells them apart. phase_label is already in scope (L538); adding a phase field to the span and recording it here would fix that.
There was a problem hiding this comment.
Added dynamo.dispatch.phase (prefill/decode/aggregated) to request.dispatch, recorded on both the KV and builtin paths. With it, each disaggregated dispatch can be paired with the selection from the same pool.
| .get() | ||
| .expect("segment not set") | ||
| .generate(request) | ||
| .instrument(lifecycle.start(LifecycleStage::RequestDispatch)), |
There was a problem hiding this comment.
This removes the worker-side request.dispatch span, and RequestDispatch moves to the frontend component in telemetry.rs. That changes the span layout that dashboards or trace analysis may key on, but the PR description doesn't mention it. Worth calling out there.
There was a problem hiding this comment.
Agreed. I'll call this out in the PR description. On main, the worker emits request.dispatch as a child of worker.operation.* around the backend generate() call. This PR removes it, and request.dispatch becomes a frontend span per dispatch attempt (one per phase leg in disagg). The worker-side interval stays covered by worker.operation.*, and ingress acceptance by worker.admission.
bfd2be4 to
0e64155
Compare
External PR triage (2026-10-13)Blocker: trusted CI has not run on the current head Next action (maintainer): @ai-dynamo/dynamo-router-codeowners @ai-dynamo/dynamo-runtime-codeowners @ai-dynamo/dynamo-frontend-codeowners @ai-dynamo/dynamo-ops-codeowners (cc @jh-nv, who reviewed and whose comments @sbak5 answered in Supply-chain audit (head Note for @sbak5: Automated triage of external contributions; this comment is edited in place on each run. Written by Devin |
|
@jh-nv @dmitry-tokarev-nv thanks again for your approvals. Since then the branch has changed, so could you take another look at the latest head (949861e)?
On this head: GH200 2P+2D run, 274 roots matching AIPerf, zero invariant violations, and every prefill and decode dispatch carries its attempt. CPU mocks (incl. a mid-stream migration) are clean, and the pre-merge mirror shows only known environment failures. Details are in the description's Validation section. |
|
@jh-nv @dmitry-tokarev-nv one more ask: once CI on 949861e finishes green, could one of you merge the PR? GitHub still shows it waiting for a required code-owner approval, so that will need to land first. Thanks! |
|
@dmitry-tokarev-nv @rmccorm4 the full PR CI hasn't started on the rebased head yet. Could one of you post |
biswapanda
left a comment
There was a problem hiding this comment.
One remaining issue in dispatch outcome classification; see the inline comment.
| request_dispatch.record( | ||
| "dynamo.dispatch.result", | ||
| match &dispatch_result { | ||
| Ok(_) => "accepted", |
There was a problem hiding this comment.
A cancelled response handshake can reach this Ok(_) arm: AddressedRouter intentionally returns Ok(empty_stream) when the prologue fails after the context is stopped or killed, and cancel_on_stop polls the operation before the cancellation branch. This records dynamo.dispatch.result=accepted for a cancelled handshake, depending on which result becomes ready first. The same issue exists at request_dispatch.finish("accepted") in routing_host/builtin.rs.
Please account for the request context's stopped/killed state when classifying successful returns, as Migration::new_stream already does, while preserving the existing cleanup behavior. A regression test that cancels during the handshake and checks the dispatch result on both routing paths would cover this.
There was a problem hiding this comment.
Good catch, fixed in 11e440d. On both routing paths, a dispatch that returns a stream is now recorded as cancelled when the request context is stopped or killed at that point, the same check Migration::new_stream uses (completed_dispatch_result in routing_host.rs). Only the recorded result changes; the guard, migration, and cleanup paths are unchanged.
dispatch_cancelled_during_handshake_is_not_accepted drives generate() on the KV and built-in (round-robin) paths with a dispatch that stops the request and returns an empty stream, as AddressedRouter does. It expects accepted without the stop and cancelled with it. Without the fix it fails with accepted.
One consequence: a stopped decode leg with staged KV, which is still dispatched so the worker can release the blocks, now records cancelled as well. That matches the migration outcome for the same request.
|
/ok to test 949861e |
|
Hi @sbak5, I’ve triggered CI and will follow up on your comments and fixes. Given the scope of this change, would you mind adding a GitHub issue or expanding an existing one with a summary of the problem, the proposed solution, and the before/after behavior? Any relevant technical design details, such as architecture or sequence diagrams and data model changes, would also help reviewers/agents understand the approach/decisions made. Thanks! |
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
No P0 or P1 found. This review read head 949861ec90e701523799b3fbcd217a6a98fe58f3 and found two new P3 items. The four P3 items of the review on 2026-10-21 are fixed on this head.
- [P3] A request that backend admission control rejects gets
dynamo.worker.admission.result=accepted(inline,push_handler.rs:721). - [P3] No test covers the nesting of the KV route span under
request.dispatch(inline,kv.rs:577).
Open items from other reviewers:
- The thread from @biswapanda on
kv.rs:619(a cancelled handshake recorded asaccepted) is open. That review read this same head, so theOk(_) => "accepted"arm inkv.rsandfinish("accepted")inbuiltin.rsare unchanged. - The P2 from @jh-nv on
migration.rs:291is fixed on this head, as the first table below shows. The thread is still unresolved. - The other 16 threads from jh-nv and KrishnanPrash are unresolved too, and
mainrequires resolved threads.
Earlier items, one line each.
| Item | State on this head | Evidence |
|---|---|---|
candidate.eligible.count equals candidate.count |
Fixed | filters.eligible is recorded. A break that puts candidate_count back fails lifecycle_selection_evidence. In the CPU Mocker run, 14 of 14 selections have eligible = candidate + filtered.policy. |
No test for the DP-rank rule of DispatchSpan |
Fixed | dispatch_span_records_only_the_last_target_dp_rank passes. A break that renames the field that Drop records fails it. |
| Worker spans in different places on KV and built-in routes | Fixed | 14 of 14 kv_router.route_request spans and 4 of 4 router.route_request spans are children of request.dispatch. |
| Description does not name the investigation-only fields | Fixed | The description now names them. In core mode, none of the 32 preprocessing and streaming spans has dynamo.stage.outcome, dynamo.stage.checkpoint or dynamo.stream.events. |
| [P2] from @jh-nv: a mid-stream retry leaves the request span | Fixed, thread open | A break that removes .instrument(request_span) fails test_mid_stream_migration_retry_stays_under_the_request_span. In the CPU Mocker run, both migrated requests have a second router.queue, router.selection and request.dispatch (dynamo.request.attempt=1) under request.lifecycle in the same trace. |
Rebase: only the push_handler.rs conflict changed the earlier patches.
git range-diff 662cb22df4..0a22d27e1c f7deb693dd..949861ec90 shows commits 2 to 5 as identical patches, including 40772c53d9 as 4302ee4ac6 and 0a22d27e1c as 949861ec90. Commit 1 differs only in context: a moved mod server; line in http/service.rs and the push_handler.rs hunk. The added and removed line counts match for 17 of 18 files. push_handler.rs goes from +143/-24 to +142/-23.
The push_handler.rs resolution keeps both changes. #14369 on main removed the admission_gate::global().admit(...) wrapper, and this file on the head has no admission_gate reference. The removal of the worker-side request.dispatch instrument from this PR is still in place.
Tests on Linux, base and head, with deliberate breaks.
Command: cargo test --locked --offline --no-default-features -p dynamo-runtime -p dynamo-kv-router -p dynamo-llm --lib. It ran in the dynamo-runtime-test CI image of this head, with Rust 1.96.1 and no network.
| Tree | dynamo-kv-router | dynamo-llm | dynamo-runtime |
|---|---|---|---|
merge base f7deb693dd |
1360 passed | 3384 passed | 908 passed |
| head | 1363 passed | 3388 passed | 911 passed |
No test failed on either tree. The new tests of 4302ee4ac6 and 949861ec90 ran and passed. The admission test ran its tcp and quic child processes.
On macOS, eleven deliberate breaks of the new code each fail the test that guards that code:
deferredalways true, the per-command turn count removed, the capacity mark removed, anddeferredrecorded on every blocked pass. Each failslifecycle_queue_deferral_reflects_waiting.pool.rolenot recorded, andeligible.countset back tocandidate_count. Each failslifecycle_selection_evidence.- The dispatch attempt not recorded, and the prefill attempt key not read. Each fails
request_dispatch_records_phase_and_attempt. - The prefill leg copies attempt 0. This fails
client_cancellation_does_not_cancel_prefill_context. - The rank field that
Droprecords renamed. This failsdispatch_span_records_only_the_last_target_dp_rank. - The request span removed from the mid-stream retry. This fails
test_mid_stream_migration_retry_stays_under_the_request_span.
One break changes no test result, on macOS or on Linux: the KV route span created outside request.dispatch (inline, kv.rs:577).
cargo fmt --all --check passes. On Linux, cargo clippy --all-targets --no-default-features -- -D warnings passes for the three crates. The rust-clippy (.) job of Pre Merge also passed.
CPU Mocker runs: five request types, all spans in the request trace.
Setup: two Mocker workers, the frontend with the KV router, file discovery and an OTLP HTTP receiver. All ran in the CI image of this head with no network. The router policy sets prefill_busy_threshold: 256 and request_queue_limit_per_worker: 2. One run used investigation mode, and one used core mode.
| Request | Result |
|---|---|
| Normal | router.queue dequeued with deferred=false and no reason, request.dispatch accepted with attempt 0, root success |
| Queued | 4 requests: router.queue dequeued, deferred=true, reason prefill_capacity |
| Rejected | 8 requests: HTTP 529, router.queue rejected, deferred=false, reason request_limit, no selection or dispatch span |
| Cancelled stream | Root cancelled, response.streaming abandoned with 3 events |
| Migration retry | A worker killed mid-stream. 2 requests have a second queue, selection and dispatch span (attempt 1) in the same trace. |
Each mode had 50 router and dispatch spans, and none was outside the request.lifecycle subtree. No router.queue span has a repeated attribute, and the largest has 27 attributes. The run of 81326653c9 on 2026-10-21 differs in three ways. Its KV route spans were under request.lifecycle, its rejected requests recorded deferred=true, and it used the policy_ordering reason.
New and changed span fields in 4302ee4 and 949861e: none carries caller text.
router.selection,dynamo.router.candidate.eligible.count: the number of worker DP ranks that pass the host checks.router.selection,dynamo.router.pool.role: a static string, now recorded before selection.router.queue,dynamo.router.queue.deferred(a boolean) anddynamo.router.queue.reason(a fixed set): now recorded once, at the end of the span.request.dispatch,dynamo.request.attempt: au32from the migration attempt counter, or the copy that the prefill leg carries.
With lifecycle tracing off, the KV route span is created inside Span::none().in_scope(...), which keeps its old parent. The queue actor adds one u64 increment per event, and each queued request carries one more u64 and one AtomicBool.
Cost with lifecycle tracing off.
worker_selection bench of dynamo-kv-router, release build on macOS. Base and head used the same bench file, with two runs each.
| Case | Base f7deb693dd |
Head |
|---|---|---|
| 2 workers, no subscriber | 72.6 / 74.4 ns | 82.3 / 81.7 ns |
| 2 workers, registry subscriber | 87.1 / 87.8 ns | 99.9 / 98.3 ns |
| 32 workers, no subscriber | 671 / 685 ns | 650 / 672 ns |
| 32 workers, registry subscriber | 836 / 805 ns | 902 / 814 ns |
| 10,000 workers, composed policy, keep-all filter | 287 / 285 µs | 284 / 287 µs |
With two workers, the head adds 7 to 13 ns per selection. With 32 and 10,000 workers, the difference is within the run-to-run spread.
CI and merge with main.
PRrun 39847082175 passed: 44 jobs passed and 33 skipped. The skipped jobs are the deploy, snapshot, operator, Helm, planner, Power Agent, SGLang N-2 and Allure report lanes. The dev, local-dev and EFA image builds were also skipped.- Pre Merge run 39841356411 passed, with
rust-tests (.)andrust-clippy (.). It skipped 7 docs, operator, recipe and planner jobs.PR-XPUandFern Docspassed. Docs link checkrun 39841356156 failed inlycheeonhttps://github.com/Spycsh(404) indynamo-v1-5-0.mdx. This PR does not change that file, and themainpush run 39856184239 ata7ed39ca91fails on the same link.- A merge of this head with
mainf8e69c912dconflicts in one file,lib/llm/src/preprocessor.rs. Both sides change the lines after thepreprocess_request_with_optionscall: this PR adds.inspect_err(...)for the stage outcome, and #13187 adds theRequestMetrics::from_contextinput-token count. A resolution needs both changes.
| let _ = publisher.finish().await; | ||
| return Err(PipelineError::Generic(message)); | ||
| } | ||
| worker_admission.record("dynamo.worker.admission.result", "accepted"); |
There was a problem hiding this comment.
[P3] A request that backend admission control rejects still gets dynamo.worker.admission.result=accepted. lib/runtime/src/pipeline/network/ingress/push_handler.rs:721. On a main with #14369, the worker's admission gate decides inside generate(), after this span closes, and request.dispatch then records failed. Please record the gate's rejection on a worker span, or say in the description that worker.admission ends before backend admission control.
Measured in a CPU run of this head, with the gate at one request and no queue.
Setup: one dynamo.common.backend.sample_main worker with --delay 0.2, DYN_BACKEND_ADMISSION_ENGINE_REQUEST_LIMIT=1, DYN_BACKEND_ADMISSION_ENGINE_REQUEST_WAITING_LIMIT=1 and DYN_BACKEND_ADMISSION_REQUEST_QUEUE_LIMIT=0. The frontend used round-robin routing, and lifecycle tracing was on in investigation mode. The client sent four streaming requests at the same time.
| Request | HTTP | worker.admission result |
request.dispatch result |
Root outcome |
|---|---|---|---|---|
| 1 | 200 | accepted |
accepted |
success |
| 2, 3, 4 | 529 | accepted |
failed |
rejected |
For requests 2 to 4, the worker log has Worker at capacity (an engine limit and the queue are both full), rejecting request from dynamo_llm::admission_gate, then Generate Error: CapacityExhausted: Server overloaded: worker at capacity. lib/backend-common/src/worker.rs wraps the engine in AdmissionEngine, so the gate runs inside generate(), after line 722 drops worker_admission.
| ); | ||
| // Under request.dispatch, as on the built-in path, so worker spans nest | ||
| // the same way on both. Without a lifecycle root nothing is entered. | ||
| let route_span = request_dispatch.in_scope(|| { |
There was a problem hiding this comment.
[P3] No test covers the new nesting of the KV route span under request.dispatch. lib/llm/src/kv_router/routing_host/kv.rs:577. If this line calls in_scope on tracing::Span::none(), the route span leaves request.dispatch, and all 501 kv_router:: and migration:: tests of dynamo-llm still pass. Please add a test, for example the one below, in routing_host/tests.rs.
@@ -1834,6 +1834,70 @@ async fn kv_cancellation_after_admission_still_stops_the_aggregated_dispatch() {
runtime.shutdown();
}
+/// The KV route span nests under `request.dispatch`, as on the built-in path, so
+/// the worker spans sit in the same place on both routes.
+#[tokio::test]
+#[serial_test::serial]
+async fn kv_route_span_nests_under_request_dispatch() {
+ use tracing_subscriber::prelude::*;
+
+ /// Parent name of each `kv_router.route_request` span.
+ #[derive(Clone, Default)]
+ struct RouteParents(Arc<Mutex<Vec<Option<&'static str>>>>);
+
+ impl<S> tracing_subscriber::Layer<S> for RouteParents
+ where
+ S: tracing::Subscriber + for<'a> tracing_subscriber::registry::LookupSpan<'a>,
+ {
+ fn on_new_span(
+ &self,
+ _: &tracing::span::Attributes<'_>,
+ id: &tracing::Id,
+ ctx: tracing_subscriber::layer::Context<'_, S>,
+ ) {
+ let span = ctx.span(id).unwrap();
+ if span.name() == "kv_router.route_request" {
+ self.0
+ .lock()
+ .unwrap()
+ .push(span.parent().map(|parent| parent.name()));
+ }
+ }
+ }
+
+ let parents = RouteParents::default();
+ let _guard =
+ tracing::subscriber::set_default(tracing_subscriber::registry().with(parents.clone()));
+ let (router, dispatch, _worker_id, runtime) =
+ router_with_recorded_dispatch("kv-route-span-nesting").await;
+ let mut request = Context::with_id_and_metadata(
+ request(),
+ "kv-route-span-nesting".to_string(),
+ Default::default(),
+ );
+ request.insert(LIFECYCLE_TRACE_CONTEXT_KEY, LifecycleTrace::new(true));
+ let phase = RequestPhase::Aggregated;
+ let budget = CleanupBudget::default();
+ let (mut selection, _) = router
+ .select_with_affinity(&request, phase, false, &budget)
+ .await
+ .unwrap();
+ let guard = router
+ .track_selection(&request, &mut selection, phase, false, &budget)
+ .await
+ .unwrap();
+ let stream = router
+ .dispatch_selection(request, selection, guard, &budget)
+ .await
+ .unwrap();
+ drop(stream);
+ assert_eq!(dispatch.worker_ids.lock().unwrap().len(), 1);
+ assert_eq!(*parents.0.lock().unwrap(), [Some("request.dispatch")]);
+
+ drop(router);
+ runtime.shutdown();
+}
+
#[derive(Debug, PartialEq, Eq)]
enum ClassifierObservation {
Completed(usize),The test passes on this head. Without the nesting, it fails.
All runs used the CI image of this head on Linux, with --no-default-features.
- On this head,
kv_route_span_nests_under_request_dispatchpasses. - With line 577 changed to
tracing::Span::none().in_scope(|| {, it fails withleft: [None]andright: [Some("request.dispatch")]. rustfmt --checkpasses. With the test added,cargo clippy --all-targets -- -D warningspasses fordynamo-runtime,dynamo-kv-routeranddynamo-llm.
|
@biswapanda thanks for triggering CI and for the review.
Validation on this head: quick check (fmt, Clippy, focused suites), CPU mocks with zero invariant violations, and a GH200 2P+2D run. In that run, 288 request roots match AIPerf (225 success, 63 cancelled at the cutoff, 0 errors), with zero violations across 4,265 lifecycle spans. Since the head moved, could you post |
|
/ok to test 11e440d |
11e440d to
16576b5
Compare
|
@biswapanda @rmccorm4 thanks for running CI on 11e440d; it passed. Main moved again and the PR had a merge conflict, so I rebased it onto main Could one of you post The |
|
/ok to test 16576b5 |
|
@biswapanda @jh-nv any pending issue for merge? |
|
/ok to test 16576b5 |
I am good. launched test for my final check |
|
@sbak5 , please resolve the conflict |
16576b5 to
d06df9d
Compare
|
@biswapanda @rmccorm4 the PR had a merge conflict with main again, so I rebased it onto main The only conflict was in Could one of you post |
|
@jh-nv could you post |
|
/ok to test d06df9d |
Add bounded state evidence to the frontend lifecycle spans for request preprocessing, worker admission, router queueing and selection, dispatch and response streaming, join router and dispatch spans to the request trace, keep migration retries under the request span, record queue deferral once, pool role and the dispatch attempt, and classify handshake-cancelled dispatches as cancelled. Signed-off-by: Seonmyeong Bak <sbak@nvidia.com>
d06df9d to
3b2bdfb
Compare
|
@jh-nv I resolved the conflicts with main ( |
Overview
Add bounded state evidence to the existing frontend lifecycle spans for request
preprocessing, worker admission, router queueing and selection, dispatch, and
response streaming. Core mode keeps the stable decision summary and timing
spans; investigation mode adds the preprocessing and streaming state below and
up to four scored router candidates with their available cache/load inputs. Request identity and terminal semantics are
unchanged; no engine events or operation-ID propagation are added.
Details
In investigation mode, preprocessing records its last checkpoint and an
outcome classified with the request terminal policy, so invalid arguments are
rejected, matching the request outcome.Worker admission records payload size, transport, and accepted/rejected/failed;
a response-plane mismatch is recorded as rejected, never accepted.
Dispatch records route, destination worker, phase, attempt, result, and the DP
rank of the last attempted target (a retry to a worker without a rank does not
inherit one).
Router queue records class/policy, depth snapshots, and the exit outcome/reason.
deferredis recorded once at exit: true when the request left the queue in alater router turn than it entered, with reason
prefill_capacityorclass_backlog; router selection records candidate/filter accounting and theselected/best scores.
candidate.eligible.countis the host-eligible countbefore the policy filter, so
eligible = candidate + filtered.policy.In investigation mode, streaming counts SSE items yielded by the frontend, not
tokens received by the client, and a dropped stream records
abandoned; therequest terminal stays authoritative. Router evidence describes routing inputs and decisions, not
observed cache reuse or transfer results.
Span layout change. On main, the worker ingress emits
request.dispatch(
dynamo.component=worker) as a child ofworker.operation.*around thebackend
generate()call. This PR removes that worker span;request.dispatchis now a frontend span per dispatch attempt (one per phase leg in
disaggregated serving, labelled by
dynamo.dispatch.phase). The worker-sideinterval stays covered by
worker.operation.*, and ingress acceptance byworker.admission. Routerrouter.queueandrouter.selectionspans arechildren of the request's selection span in the same trace. On both the KV
and built-in paths, the route span (
kv_router.route_requestorrouter.route_request) and the worker spans nest underrequest.dispatch;without a lifecycle root the route span keeps its existing parent.
Where should the reviewer start?
lib/runtime/src/telemetry.rs: stage observation and new span fields.lib/kv-router/src/scheduling/selector/mod.rs,queue.rs: router evidence.lib/runtime/src/pipeline/network/ingress/push_handler.rs: worker admission.lib/llm/src/kv_router/routing_host/builtin.rs,kv.rs: dispatch.lib/llm/src/preprocessor.rs,http/service/openai.rs: preprocessing outcome.Validation
Validated on the rebased branch (main
f7deb693dd):suites (runtime lifecycle and ingress, router scheduling, LLM lifecycle,
routing host, migration, built-in policy) passed. The new migration and
handshake-cancellation tests fail when their fixes are removed.
bindings, runtime examples, and KVBM pass every step. In the root workspace the
remaining unit-test failures also occur on unmodified main in that environment
(disk-storage tests, Hugging Face downloads, and timing-sensitive tests); none
is attributable to this PR.
rejection and queued cancellation; round-robin with a preprocessing rejection;
core mode; lifecycle disabled; and a mid-stream migration in which the first
worker is killed after streaming and the request completes on a second worker.
Zero invariant violations. Every router and dispatch span, including the
migration retry's, descends from
request.lifecycle, and/v1/completionsemits no lifecycle spans.
AgentX concurrency 64, 900 s), on the latest head: 288 request roots whose
success/cancellation counts match AIPerf exactly (225 success, 63 cancelled at
the cutoff, 0 errors); 576 dispatches (288 prefill + 288 decode), each with its
attempt, paired with their selections; all 1,728 router and dispatch spans in
the request's lifecycle subtree; all 576 KV route spans under
request.dispatch, with the worker spans under them; queue deferral recordedonce per queue span; zero invariant violations. Functional validation, not
a performance claim.
Related Issues
Closes DIS-3128
Part of #16381 (DEP: native request-lifecycle observability with OpenTelemetry spans)
Builds on #14101 (per-request lifecycle instrumentation, merged), which added the
lifecycle spans and terminal outcomes this PR extends.
Summary by CodeRabbit
New Features
Bug Fixes