Repository navigation
Feat: Populate the BYTES column for ordinary traffic - #1334
Merged
Merged
Conversation
An operator hit a ContextWindowExceededError from LiteLLM and wanted to know how big the request Cortex had just forwarded actually was. They found agentop's BYTES column, switched it on, and it was blank on every row: the column only ever had a figure on an opaque tunnel's close row, which is what its own help text said. To get the number at all they yanked the event to a file and measured the file — which is the event's JSON, not the body that went on the wire. Every request row now carries the size of the body it forwarded and every response row the size of the body that came back, on all three listeners. BYTES goes default-on in the same change: #1200 made it the one opt-in column explicitly because only tunnel rows had a figure, and that reason is gone. Response bytes are summed in a new pipeline.Context.ResponseBytes rather than measured at the record site, because len(pctx.ResponseBody) is a buffer and not a tally — extproc replaces it per chunk on the SSE path and truncates it at its cap, and neither proxy's streaming relay fills it at all. Reading it would have reported a streamed inference response as the size of its final chunk, on exactly the traffic the column was asked for. The proxies count at one choke point each, through a new counting reader (core/listener/internal/bodycount), rather than per arm. Counting per arm put them 32 bytes below extproc on a four-event SSE response: the re-framing arm sees sseframe payloads with the `data: ` prefixes and blank-line separators already stripped. A parity fixture now pins all three listeners to the same figure for the same body. Zero means the listener counted nothing — not that the body was empty. Bodies are buffered only when a plugin asks, and both proxies' fully-unbuffered relays copy after the row is appended, so those rows report zero and agentop renders them blank. Content-Length is not a substitute: it is -1 on anything chunked and deleted outright on every streaming arm, so it would be a confident guess exactly where it was wrong. Fixes #1309 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughListeners now record request and response body byte counts on session events. The agentop BYTES column displays counts for regular traffic and is enabled by default. Tunnel rows show byte counts for both directions. ChangesBody byte counts
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant Proxy as Forward or reverse proxy
participant Body as bodycount reader
participant Context as Pipeline Context
participant Event as Session event
participant Pane as agentop Events pane
Proxy->>Body: Wrap upstream response body
Proxy->>Body: Read response body
Body->>Context: Accumulate bytes read in ResponseBytes
Proxy->>Event: Record ResponseBytes as BytesDown
Event->>Pane: Provide event byte counts
Pane->>Pane: Render counted directions in BYTES
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The documented BYTES behavior remains clear. No actionable merge-blocking risk is established. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
One conflict, in CLAUDE.md's event-schema list, and it is the ordinary kind: both sides edited the `tunnel`/`tunnelReason` bullet. Main added an `inference.requestedModel` bullet beside it (#1314's pctx.SetRequestModel); this branch had split `bytesUp`/`bytesDown` out of that bullet's heading, because they stopped being tunnel-only. Resolved by keeping all three bullets: `inference.requestedModel` from main, placed next to `requestedHost` since both read "present only when a plugin changed X", then this branch's `bytesUp`/`bytesDown` bullet and its rewritten `tunnel`/`tunnelReason` one, which no longer claims the two fields for itself. Main also dropped `pctx.RewrittenBodyLen` under this branch (081ede5), which turns out to confirm the request-side count rather than break it: that commit moved settlement onto `len(pctx.Body)` once `BodyMutated` is true, "with a comment saying why that is the body sent" — the same primitive, at the same point in the pipeline, that this branch reads for `bytesUp`. Nothing to reconcile. Verified after the merge: from core/, `gofmt -l .`, `GOWORK=off go vet ./...`, `go mod tidy -diff`, and `go test ./listener/... ./pipeline/... ./sessionapi/... -race -count=1`; from cmd/agentop/, the same three plus `go test ./... -race -count=1`. All clean. The four bytes parity fixtures still pass on every listener they cover — buffered request and response on both sides, SSE, and the unbuffered none case. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
`TestCommittedAssetIsCurrent` caught this, and the whole diff is one token: the footer hint goes from `→ 3 more columns` to `→ 4 more columns`. That is BYTES becoming default-on. It keeps `keep: keepLow`, so at the demo's terminal width it is the first column dropped — it does not appear in the animation, it just moves the off-screen count up by one. The staleness check compares content, not size, which is why the two byte counts in its failure message were identical at 71121. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
pdettori
approved these changes
Oct 9, 2026
pdettori
left a comment
pdettori
left a comment
Member
There was a problem hiding this comment.
Reviewed the full diff against the event contract and each listener's body paths. What I verified:
- Record sites complete: all nine ordinary request/response sites (extproc ×4, forwardproxy ×2, reverseproxy ×3) read the correct side —
len(pctx.Body)post-pipeline on requests,pctx.ResponseByteson responses. No site missed; tunnel sites undisturbed. - Choke points: both proxies wrap
resp.Bodybefore any arm consumes it; the buffered arms replace the body with a reader over already-counted bytes, so nothing double-counts; extproc's accumulator sits after the nil-pctxguard and before both arms. - Request-side exactness rests on extproc's BUFFERED request-body mode (one message ≤ maxBodySize) — exact on the shipped Envoy config, the same reachability caveat the response-side comments already record for a statically-configured STREAMED mode.
- Zero semantics: pinned by a producer test on each proxy's unbuffered relay and on extproc's header-only response;
bytesCellkeeps the measured-zero pair on tunnel closes and renders ordinary rows one-sided. - Settings round-trip: a file that opted into BYTES before #1309 now matches the new default and drops its entry on the next save; the synthetic-column tests keep the default-off path exercised.
- Docs: README's ~185 is exact at declared widths (159 + 13×2 padding); the regenerated demo asset is enforced by
TestCommittedAssetIsCurrent.
No findings. (Trivia, no action: the PR body says "eight record sites"; the diff touches nine.)
Author: cwiklik (MEMBER — maintainer)
Areas reviewed: Go (extproc/forwardproxy/reverseproxy, pipeline, agentop TUI), docs, tests
Agent/IDE config (.claude/.vscode): none
Commits: 3, all signed-off ✓
CI status: passing
Assisted-By: Claude Code
Two conflicts, both in what the agentop TUI's demo output looks like, and both from #1342 replacing the events pane's filter with a search: - `cmd/agentop/README.md` — the ASCII mock's two footer lines. Took main's (`[/anthropic 2 matches]`, `[/] search`, `[n/N] next/prev`) and dropped the `→ 4 more columns ([c] to choose)` fragment it also added there. That hint is emitted only when `eventColsDropped > 0` (`tui/keys.go`), and the mock is captioned as the terminal wide enough for every column — so in that frame it cannot appear. The hint itself stays documented in the paragraph above, which main and this branch both edited and which merged cleanly. - `docs/assets/cortex-demo.svg` — generated, so regenerated rather than merged: `go run .` in `scripts/readme-demo`, whose staleness check passes. The asset's one-line delta from main is the demo frame's footer reading `→ 4 more columns` where it read `→ 3`, which is this branch's extra default column showing up in the real agentop the generator drives. Everything else merged clean, including the four `tui/` files both sides touched. The three comments in `tui/settings.go` that this branch reworded — BYTES was the only `defaultOn:false` column, and is not any more — still read correctly against main's text. Verified on the merge result: `gofmt` clean across `core`, `cmd` and `scripts`; `GOWORK=off go vet ./...` clean in `core`, `cmd/agentop` and `cmd/cortex`; `go mod tidy -diff` clean in all four touched modules; and full `GOWORK=off go test ./... -race -count=1` green in both `core` and `cmd/agentop`. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
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
3 participants
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.
Summary
Fixes #1309. agentop's BYTES column was blank on every ordinary row — it only
ever had a figure on an opaque CONNECT tunnel's close row, which is what its own
picker help said. An operator who hit a
ContextWindowExceededErrorand wantedthe size of the request Cortex had just forwarded found the column, switched it
on, got nothing, and fell back to
y-yanking the event and measuring the file —which is the event's JSON, not the body on the wire.
Every request row now reports the body it forwarded and every response row the
body that came back, on all three listeners, and BYTES is default-on: #1200 made
it the one opt-in column because only tunnel rows had a figure, and that reason
is gone.
What changed
core/pipeline/context.go— newResponseBytes int64, summed by whoevermoves the bytes and read at every response record site.
core/listener/internal/bodycount(new) — a countingio.ReadCloserwrapper, installed once per proxy at the single point every response arm reads
the upstream body through.
core/listener/{extproc,reverseproxy,forwardproxy}/server.go— accumulate,and set
BytesUp/BytesDownat the eight record sites.core/pipeline/session.go,CLAUDE.md,cmd/agentop/README.md—the wire contract, split out of the tunnel-only prose.
cmd/agentop/tui/—bytesCellrenders one side on an ordinary row andkeeps the pair on a tunnel close; BYTES goes
defaultOn, with the threecomments and two settings tests that named it as the opt-in column reworked
against a synthetic column rather than deleted.
No wire field, decoder, sort key or
summarizeEventfield-count guard changes —reusing the two existing fields buys all of that, and a request row and a
response row are different phases, so they never collide.
Why an accumulator, and why one choke point
len(pctx.ResponseBody)at the record site is the obvious implementation and iswrong on three paths: extproc's SSE arm replaces the buffer each chunk, its
non-SSE arm truncates at its cap, and neither proxy's streaming relay fills it.
It would have reported a streamed inference response as the size of its final
chunk — on exactly the traffic the column was asked for.
Counting inside each proxy arm instead of at the body put the proxies 32 bytes
below extproc on a four-event SSE response: the re-framing arm sees
sseframepayloads with the
data:prefixes and\n\nseparators already stripped, andsseframe.Readerexposes no raw-bytes-consumed counter. A new parity fixturepins all three listeners to one figure for one body.
Zero means not counted
There is no third value for "unknown", by design. Bodies are buffered only when a
plugin asks, and both proxies' fully-unbuffered relays copy after the row is
appended, so those rows report zero and agentop renders them blank rather than
0B.Content-Lengthis not a substitute:-1on anything chunked and deletedoutright on every streaming arm, so it would be a confident guess precisely where
it was wrong.
Verification
core/listener/parity) — the same body must count thesame on all three listeners, both directions, both phases.
extproc's multi-message response (the only fixture that distinguishes the sum
from the trailing chunk), forwardproxy's
streamPassthrougharm (reached by noparity fixture), and the honest zero on each proxy's unbuffered relay.
recording: swapping extproc's accumulator back for
len(pctx.ResponseBody)leaves the parity suite green, because its driver delivers each body in one
message. That finding is in the extproc test's doc comment so the next person
does not assume parity covers it.
go test -raceoncore/{listener,pipeline,sessionapi}and all ofcmd/agentop;go vetandgofmt -lclean in both modules;go mod tidy -diffunaffected.Still to do by hand, and the only thing that truly answers the issue: run a
Claude Code session through the laptop proxy and confirm BYTES is populated
without being switched on, that an inference request's figure is the body size
and not the event JSON's, and that a GET renders blank.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit