feat(usage): report sends, spend and cache provenance per logical request (#4546) - #4638
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd75d0db82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cacheProvenance = classifyCacheTelemetryProvenance(loggedUsage, { | ||
| wireParsed: logCtx.usageWireParsed === true, | ||
| }); | ||
| const logicalRequestId = logCtx.logicalRequestId ?? logCtx.executionBudget?.logicalRequestId; |
There was a problem hiding this comment.
Production requests never populate either new context field read here: the ingress paths create or reuse sendBudget in src/server/responses/core.ts, but never assign it or its ID to logCtx (repository-wide search finds assignments only in the new unit tests). Consequently real rows omit logicalRequestId, reserved, and policyVersion, and budget charges without an attempt row cannot contribute to unresolved; attach the shared budget to the context when it is created or received.
Useful? React with 👍 / 👎.
| return entry.attempts.map(attempt => { | ||
| // An attempt's own provenance wins; the row's is the fallback for a child written before | ||
| // attempt-level provenance existed. A child carrying no cache fields still resolves to | ||
| // unknown in cacheObservationFromUsage, so it cannot inherit a sibling's observation. | ||
| const cacheProvenance = attempt.cacheProvenance ?? entry.cacheProvenance; |
There was a problem hiding this comment.
For combo requests, this fallback applies the aggregate row provenance to every child because production code never sets PersistedUsageAttempt.cacheProvenance; only the new test fixtures do. If one child has observed positive cache reads while another carries a synthesized zero, the aggregate is observed, so the synthesized child's input tokens incorrectly enter its model's hit-rate denominator and it reports 0% instead of null. Set provenance on activeAttempt when usage is captured rather than relying on the parent fallback.
Useful? React with 👍 / 👎.
| export function recordNoAccountAffinityFailure( | ||
| logCtx: RequestLogContext, | ||
| lane: AffinityModelLane, | ||
| reason?: CodexAffinityReason, | ||
| ): CodexAffinityReason | undefined { | ||
| const resolved = isKnownAffinityReason(reason) ? reason : takeNoAccountAffinityReason(lane); | ||
| if (resolved === undefined) return undefined; | ||
| noteAffinityMove(logCtx, "cleared", resolved); | ||
| logCtx.errorCode ??= "codex_no_account"; |
There was a problem hiding this comment.
These new recorders have no production callers: repository-wide search finds noteNoAccountAffinityReason, recordNoAccountAffinityFailure, and noteAffinityMove only in this file and its direct unit tests, while src/codex/routing.ts still uses the thread-only pendingReleaseReasons map and src/server/responses/core.ts still assigns the single affinity fields directly. Thus an exhausted model lane still does not annotate its failing row, lanes can still share the old thread-keyed cause, and moveReasons never accumulates multiple moves; invoke these helpers from the routing/auth failure and successful-resolution paths.
Useful? React with 👍 / 👎.
| const sends = count(spend.sends); | ||
| const settled = count(spend.settled); | ||
| const unresolved = count(spend.unresolved); | ||
| if (sends === null || settled === null || unresolved === null) return undefined; | ||
| if (settled > sends) return undefined; | ||
| const reserved = "reserved" in spend ? count(spend.reserved) : undefined; | ||
| if (reserved === null) return undefined; |
There was a problem hiding this comment.
For persisted or hand-edited rows, the normalizer only checks settled <= sends and accepts arbitrary unresolved or reserved values, despite /api/usage trusting and summing them. For example, {sends: 2, settled: 2, unresolved: 100} survives normalization and inflates unresolved spend, while {sends: 2, settled: 1, unresolved: 0} silently loses an unexplained send. Require unresolved === Math.max(sends, reserved ?? sends) - settled before accepting the record.
Useful? React with 👍 / 👎.
| A row also carries what its logical request cost upstream. `logicalRequestId` names the turn | ||
| that a retry leg, a repair refetch and a combo child all belong to, and `spend` aggregates their | ||
| physical sends: `sends` totals every attempt on the row, `settled` counts the sends whose attempt |
There was a problem hiding this comment.
This adds public /api/logs and /api/usage fields and changes the documented interpretation of cacheHitRate, but the only documentation update is this internal structure note; the English management API page and its translations still omit the spend/provenance fields and the observed-only denominator. Update docs-site/src/content/docs/reference/management-api.md and keep its locales consistent with the new wire contract.
AGENTS.md reference: AGENTS.md:L380-L381
Useful? React with 👍 / 👎.
리뷰 · 우선순위 64 / 80이 PR은 스택 문서의 wpg(spend instrumentation) 레이어입니다. base가 지금 가치는 큽니다. 다만 wpg는 「앞 레이어가 만든 것을 보고한다」가 설계 전제라서, wpb가 CI에서 라인 src/usage/log.ts (PersistedRequestSpend / unsettled 분해) - settled+unresolved≤sends 불변식을 파서에서 거르는 방향은 맞습니다. 다만 Cursor처럼 cap은 걸려도 경로 structure/gui-and-management-api.md - API 스키마를 문서에 맞춘 것은 좋습니다. GUI가 아직 이 필드를 안 그리면 「운영자가 읽는다」는 요약과 실제 화면 사이에 간격이 남습니다. 이번 범위 밖이면 후속 이슈 번호만 박아 주세요. 경로 CI enforce-target - base가 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
52734ae to
8f27859
Compare
92f5ddc to
7382f6a
Compare
7382f6a to
27b5170
Compare
3ea8131 to
4398009
Compare
…uest (#4546) Part of the stacked delivery closing the remaining OCX-4546 cost-guard scope. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
structure/gui-and-management-api.md sat exactly at the 600-line budget, so documenting the spend and cache-provenance record pushed it to 630 and structure:check failed. The grace entry is the mechanism the check itself names. The plan it stands for: the usage-aggregation half of this doc is now large enough to be its own page, and splitting it is a separate change that touches no source. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
) The membership oracle resolves an unmapped file through the regex seeds and fails when a seed disagrees with the explicit table. request-spend-instrumentation.test.ts was claimed by the usage seed while the table pinned it to server; the file exercises the request-log writer, so the name moves rather than the domain. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
2478c48 to
6c96bdb
Compare
1 participant
Summary
An operator can now read what one logical request actually cost.
sendCountalready counted physical sends per attempt, which answers the wrong question: a turn that failed over and fanned out across combo targets is one user turn, and nothing summed it.A request log row now carries
logicalRequestIdand aspendrecord aggregating every attempt under that turn, combo children included, split intosettled— sends whose attempt reached a terminal status — andunresolved, an attempt abandoned in flight or a budget charge no attempt row accounted for. Unresolved spend is never folded into settled, because an unexplained send is exactly the quantity this record exists to expose.reservedandpolicyVersionreport the request execution budget's final state, andmoveReasonskeeps every pool-binding move that discarded a warmed cache prefix rather than only the last./api/usagetotals these assends,settledSends,unresolvedSendsandspendRequests.Cache detail is now qualified by provenance rather than read as measurement. Strict-client normalization emits zero-default token-detail objects on every bridged wire, so a
cached_tokens: 0recovered from a parsed wire proves nothing.cacheProvenancekeepsobserved,synthesizedandunknowndistinct from capture through persistence to the summary, only observed input tokens reach thecacheHitRatedenominator, and the summary counts the three provenances separately instead of averaging an unknown as a zero. Rows written before the field existed are reconstructed from their own shape so historical readings do not move.Two defects are fixed alongside. Affinity was set on the row and omitted from the field-by-field disk projection in
addRequestLog, so every move was lost at the next restart — the same whitelist trap #4592 hit one layer up. And a no-account selection handed its cause to whatever later resolve happened to succeed, which for a pool that stays exhausted is never; reasons are now held per thread and model lane, consumed by that lane alone, and attached to the failing request's own row.Stacked on #4637.
Verification
Not run, by explicit instruction: the local suite,
bun run typecheck,bun install, and any build. The verification posture for this unit is hosted CI at the exact final head SHA and nothing else; this push used--no-verify.New coverage:
tests/server/request-spend-instrumentation.test.tsandtests/usage/usage-spend-cache-provenance.test.ts, both registered inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json. One pinned assertion intests/usage/usage-summary.test.tsis updated with its reason: it divided a single measured row's cache reads by 256 rows that never measured cache.Known open: the producer for
logCtx.executionBudgetlives at the ingress insrc/server/responses/core.ts, outside this lane's write set, so until it is wired the budget-derived fields stay absent whilespend.sendsremains live from attempts alone. No UI column was added andgui/is untouched, so the Logs attempts table still has nowhere to render this.Checklist