Repository navigation
Conversation
The post-TTFT decode estimate divides all output tokens, reasoning included, by the time after the first VISIBLE delta, so reasoning models read up to ~2.5x high. Record when generation starts (first output item of any kind) and when the last output delta arrives, persist both to usage.jsonl as a validated pair, and prefer that window in decodeTokPerSecondResult. Rows without the pair keep the old estimate. Refs lidge-jun#4038, lidge-jun#6309 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
✅ Deterministic PR hygiene checks passed. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (16)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change records generation start and last-output times from Anthropic and Responses events. Request logs expose these times relative to request start, and usage entries persist valid pairs. Decode-rate calculations use the generation window when available. ChangesGeneration Window Tracking
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant SSE as SSE inspector
participant Recorder as recordGenerationEvent
participant RequestLog
participant UsageLog
participant DecodeRate as decodeTokPerSecondResult
SSE->>Recorder: parsed event type and timestamp
Recorder->>RequestLog: update generation timestamps
RequestLog->>UsageLog: persist request-relative timestamp pair
UsageLog-->>DecodeRate: normalized usage entry
DecodeRate->>DecodeRate: calculate rate from generation window when valid
Merge Risk: ⚪ Minimal · up to The change adds optional generation-window decode-rate estimates while retaining the older-record fallback. The PR reports passing focused checks; no actionable risk is established, with the full suite left to CI. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 6 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 16 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. Hygiene✅ Deterministic PR hygiene checks passed. |
Ingwannu
left a comment
There was a problem hiding this comment.
Review at 895d2e9. I read the complete eight-file patch and ran bun test tests/usage/request-log-generation-window.test.ts tests/server/management-api-logs-metrics.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts: 51 passed, 0 failed, 1015 assertions, exit 0, Bun 1.4.0. The actual Responses/Anthropic SSE taps, paired usage persistence, legacy fallback, short-window refusal and existing attempt/history behavior passed in isolated temporary homes under CPUQuota=75%, MemoryHigh=1280M, MemoryMax=1536M, zero swap and TasksMax=64. No live provider request or security scan. The approved current-head hosted CI run 36917793105 completed successfully; intentionally unselected platform jobs do not establish universal OS coverage.
One completion blocker remains: this changes user-visible decode-rate semantics and the persisted usage-row contract, but no owning structure or user documentation changes accompany it. Please update the mapped structure/dashboard-and-usage.md contract and the Logs documentation to explain:
- Optional
genStartMs/lastOutputMsare an observed request-relative pair, measured from the first output item/block (reasoning included) to the last delta, not provider-internal token timing. - The generation-window estimate is preferred only with a valid pair; legacy rows retain the post-TTFT fallback and minimum-window/unavailable behavior.
- End-to-end tok/s remains unchanged; attempt-specific generation windows and request-history presentation are not added by this patch.
Also check the existing logs.detail.reason.decode_window_too_short wording (currently “window after the first token”) against the newly preferred event window, and keep any corrected visible wording consistent across registered locales. A short measured window is still an estimate, not a provider decode benchmark. The new source comment is useful rationale but does not replace the repository's owning-document/user-contract requirement.
Please revalidate and re-attest the resulting head. This request does not ask for a broad metrics redesign or a live benchmark; the focused production-path regressions are good evidence for the bounded implementation.
Review on lidge-jun#6416 asked for the owning structure doc and the Logs user docs to state the new decode-rate semantics, and for the short-window reason wording to match the window that is now preferred. Constraint: structure/dashboard-and-usage.md sits at the 600-line budget, so two redundant blank lines were dropped to fit the new paragraph Rejected: keep "window after the first token" wording | wrong for rows that carry the generation window Confidence: high Scope-risk: narrow Directive: keep decode_window_too_short wording identical in meaning across all registered locales Tested: bun run typecheck; structure:check; privacy:scan; focused usage/logs/layout/ratchet tests 60 pass; gui i18n tests 12 pass Not-tested: docs-site build; full bun run test (left to CI) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Addressed in ab2c357 (after merging current
Revalidated: |
…jun#6416) Persist the first output-item and last output-delta timing pair, including reasoning. Prefer that window for estimated decode throughput and retain the legacy TTFT fallback. Keep end-to-end speed unchanged and synchronize unavailable-window locale copy. Carries lidge-jun#6416 by @xyjk0511. Co-authored-by: xyjk0511 <127614382+xyjk0511@users.noreply.github.com>
|
Superseded by the integration in #6487, with reviewed follow-up fixes in #6490 and Windows validation repairs in #6494/#6495, all merged into Generation-window decode-rate telemetry, legacy fallback, documentation and locale wording were carried and reconciled. End-to-end throughput remains a distinct metric. Original carry commit: Closing this PR as superseded, not claiming that its original head was merged. Thank you for the contribution. |
Summary
The Logs decode rate (#4038, #4166) is
outputTokens / (durationMs - firstOutputMs). On a reasoning modelfirstOutputMsis the first visible delta, i.e. the end of the reasoning phase, whileoutputTokensstill includes the reasoning tokens. The numerator counts work the denominator leaves out, so the estimate is inflated by however long the model thought. It also runs to stream close rather than the last token.This PR records the generation window in the proxy and prefers it when present:
genStartMs: the first output item of any kind (Anthropiccontent_block_start, Responsesresponse.output_item.added), reasoning included.lastOutputMs: the last output delta (content_block_delta,response.*.delta).Both values are request-relative. They are recorded from the two existing SSE taps (
inspectResponseLogSsePayloadParsedfor Responses/WebSocket,tapAnthropicSseForLogfor Anthropic passthrough and native) by a small new module,src/server/request-log-generation-window.ts, and persisted tousage.jsonlas a validated pair.decodeTokPerSecondResultuseslastOutputMs - genStartMswhen the pair exists and keepsMIN_DECODE_WINDOW_MSandestimated: true. Rows without the pair fall back to the existing post-TTFT window, so the change is additive and older rows keep the behavior they have today.Persisting the window is also the server-side groundwork that #6309 needs: average tokens/sec in Usage from usage history rather than the Logs buffer. The Usage aggregation and UI are deliberately left out of this PR.
Measured effect. These are medians from ~3,000 of my own
usage.jsonlrows (output ≥ 200 tokens, window ≥ 2 s), recorded with the same instrumentation applied to 2.63:The visible-text-only rate (non-reasoning tokens ÷
lastOutputMs - firstOutputMs) is an independent cross-check. The generation-window number agrees with it; the current estimate is up to ~2.5× high.Not covered: translated Claude-inbound paths that do not go through
tapAnthropicSseForLog, and per-attempt windows on combo attempts. Both keep the existing post-TTFT estimate.Refs #4038, #6309
Verification
bun run typecheck: passed.bun test tests/usage/request-log-generation-window.test.ts tests/server/management-api-logs-metrics.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts: 60 pass, 0 fail.src/server/management/shared.tsreverted todev, the new decode-rate test fails (expected 100 tok/s, got the post-TTFT 200).bun run privacy:scan: passed.bun run structure:check: passed.bun run test: did not complete locally. The parallel runner hit its 900 s limit on this Windows machine, with failures in 38 files (quota probes, Fake-IP/proxy, gateway profiles, Kiro, account pools). I ran each of those 38 files serially on this branch and on an untoucheddevworktree (f86ad0a). Results were identical in 35. Two failed less on this branch.codex-catalog-sync-hardeningalternated between 3 and 4 failures on both sides across reruns, and every one was a 5 s timeout. None of these files exercise the changed code paths. The full suite is left to CI.devat b4616be): docs and wording only.bun run typecheck,structure:checkandprivacy:scanpassed; the focused tests above 60 pass / 0 fail;gui/tests/i18n-locales.test.tsandgui/tests/compatibility-lab-i18n.test.ts12 pass / 0 fail.logs.detail.reason.decode_window_too_shortwording in all ten locales ("window after the first token" became "the measured output window", true for both windows). Rendered from this branch's GUI against a live proxy:Checklist
structure/dashboard-and-usage.md, the Logs section ofdocs-site/.../guides/web-dashboard.md, and thedecodeTokPerSecondResultdoc comment.)🤖 Generated with Claude Code
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Required local validation passed; commands, results, and any full-suite exception are documented.
I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit