fix(api): measure timings from enqueue, decode rate from execution - #93
Merged
Conversation
…ution
The four reported timings were internally inconsistent: total_ms was
end - first_prefill (queue excluded), but execution_ms was derived as
total_ms - queue_wait_ms. That subtracted queue wait from a number that
never contained it, so execution_ms was under-reported by exactly the
queue wait and clamped to 0.0 whenever queue_wait > total_ms -- the
common case under load. No field reported true end-to-end latency, and
ttft_ms returned a -1.0 sentinel that violated the ge=0.0 constraint on
GenerateResponse.ttft_ms.
Each metric is now measured between two raw timestamps, so
total_ms == queue_wait_ms + execution_ms holds by construction:
enqueue -- queue_wait --> first prefill -- execution --> completion
|<---------------------- total -------------------------->|
The arithmetic moves into a pure compute_timings() so the timeline can
be written out directly in tests instead of faking a clock.
tokens_per_s is now derived from execution_ms rather than total_ms.
Now that total_ms spans enqueue to completion, dividing by it would fold
queue wait into the rate: a request that queued 4s and decoded 1s would
report a fifth of its real decode speed, precisely in the overload cells
this metric is used to compare. Aggregate system throughput is a
separate figure computed by the benchmark client over its measurement
window.
DoneEvent.ttft becomes ttft_ms: float | None, and the API schema's
ttft_ms fields are nullable, dropping the -1.0 sentinel.
BREAKING: total_ms, ttft_ms, execution_ms and tokens_per_s all change
meaning. Artifacts under bench-results/ produced before this commit are
not comparable with artifacts produced after it.
nosyndicate
force-pushed
the
bench/server-timing-correctness
branch
from
August 16, 2026 23:55
af255af to
1c818a3
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes inconsistencies in server-reported per-request timing metrics by measuring each metric directly from raw timestamps (starting at enqueue), removing the -1.0 TTFT sentinel in favor of null, and redefining tokens_per_s to reflect decode rate over execution time (excluding queue wait).
Changes:
- Introduces a pure
compute_timings()helper to derivetotal_ms,queue_wait_ms,execution_ms, and nullablettft_msfrom raw timestamps, ensuringtotal_ms == queue_wait_ms + execution_ms. - Updates
DoneEvent/API schema to usettft_ms: float | None(nullable) and updates routes to propagate the renamed field. - Computes
tokens_per_susingexecution_ms(decode-only time) instead oftotal_msand updates tests/docs accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/executor/test_events.py | Adds unit tests for compute_timings() invariants, null TTFT behavior, and regression coverage under heavy queue wait. |
| tests/api/test_routes.py | Updates DoneEvent construction and adjusts tokens_per_s expectation to use execution_ms. |
| tests/api/test_collector.py | Updates DoneEvent construction to use ttft_ms. |
| server/executor/types.py | Renames DoneEvent.ttft to ttft_ms: float | None and documents updated timing semantics. |
| server/executor/events.py | Adds compute_timings() and uses it to populate DoneEvent timings; stamps first_token_ns only for real output tokens. |
| server/api/v1.py | Clarifies v1 timing/token rate semantics (no queue; execution == total). |
| server/api/schema.py | Makes ttft_ms nullable and updates field descriptions to match the new semantics. |
| server/api/routes.py | Computes tokens_per_s from execution_ms and switches ttft references to ttft_ms. |
| benchmarks/README.md | Documents the updated server timing semantics and compatibility implications for benchmark artifacts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+176
to
+178
| - When preemption available, `execution_ms` includes any time a sequence spent | ||
| preempted. The start of the *first* prefill is not reset on resume, so preemption | ||
| cost is accounted as execution rather than as queue wait. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The four timings reported per request were internally inconsistent.
total_mswas measured from the start of the first prefill (queue wait excluded), butexecution_mswas derived astotal_ms - queue_wait_ms— subtracting queue wait from a number that never contained it.execution_mswas therefore under-reported by exactly the queue wait, and clamped to0.0whenever queue wait exceeded it, which is the common case under load. No field reported true end-to-end latency, andttft_msreturned a-1.0sentinel that violated thege=0.0constraint onGenerateResponse.ttft_ms.Every metric is now measured directly between two raw timestamps, so
total_ms == queue_wait_ms + execution_msholds by construction:compute_timings()inserver/executor/events.pyreturning a frozenRequestTimings, so tests can write the timeline out directly instead of faking a clock.ttft_msis now measured from enqueue and includes queue wait.tokens_per_sis now derived fromexecution_msrather thantotal_ms. Sincetotal_msspans enqueue to completion, dividing by it would fold queue wait into the rate: a request that queued 4s and decoded 1s would report a fifth of its real decode speed, precisely in the overload cells this metric exists to compare. Aggregate system throughput remains a separate figure, computed by the benchmark client over its measurement window.DoneEvent.ttftbecomesttft_ms: float | None, and the API schema'sttft_msfields are nullable — the-1.0sentinel is gone.execution_msincludes any time a sequence spent preempted:start_nsis deliberately not reset on resume, so preemption is charged to execution rather than to waiting for admission.server/api/schema.pyand a new "Server-reported timing semantics" section inbenchmarks/README.mddocument the definitions and the compatibility break.total == queue + executioninvariant, a regression case for the zeroed-outexecution_msunder heavy queue wait, null TTFT, and clamping of out-of-order timestamps.BREAKING:
total_ms,ttft_ms,execution_msandtokens_per_sall change meaning. Artifacts underbench-results/produced before this change are not comparable with artifacts produced after it.