feat: add filtering and full-archive paging to browsers telemetry events#190
feat: add filtering and full-archive paging to browsers telemetry events#190archandatta wants to merge 3 commits into
Conversation
rgarcia
left a comment
There was a problem hiding this comment.
reviewed from correctness/qa and CLI surface consistency. make build and make test passed, but I think these need changes before merge.
QA / correctness
-
cmd/browsers_telemetry.go:273— multi-category filtering does not work in real QA. With a telemetry archive containingconsole,network, andpageevents,--categories consoleand--categories networkeach returned rows, but both--categories console,networkand repeated--categories console --categories networkreturned zero rows. This needs a live/SDK-level fix and a test that covers more than one category. -
cmd/browsers_telemetry.go:263—--limitaccepts invalid values even though the help says1-100. In QA,--limit -1exited 0 and returned the default page, and--limit 101also exited 0. Please validate the range client-side or loosen the help text. -
cmd/browsers_telemetry.go:267—--since 1sreturnedInternal_error: failed to read telemetry eventswith exit 1 on an otherwise valid session, while an empty--until 1hwindow returned zero rows cleanly. This may be API-side duration handling, but the CLI exposes it directly, so the command should either validate supported duration forms or the endpoint needs to stop returning a 500 for an empty/recent window.
CLI surface consistency
-
cmd/browsers.go:2757—telemetry eventsomits the--typesfilter that exists on siblingtelemetry stream. Since both commands expose telemetry event envelopes, users will expect the same category/type filtering shape unless there is a clear reason documented in help/docs. -
cmd/browsers.go:2754— the command uses list-like paging language (--limit, “per page”, “first page”) but exposes no--offset/cursor, and the NDJSON output only prints event items. That makes the default mode non-pageable, unlike the rest of the CLI list surfaces that expose--limit+--offset. Consider exposing a continuation mechanism or framing this as “latest N events” with--allfor archive dumps. -
README.md:316— the README telemetry section documentstelemetry streambut not the new recordedtelemetry eventscommand. Since this adds a user-facing CLI surface, please document the command, flags, output mode, and how it differs from live streaming.
Sayan-
left a comment
There was a problem hiding this comment.
Confirming all feedback from \raf has been addressed except for
cmd/browsers_telemetry.go:267 — --since 1s returned Internal_error: failed to read telemetry events with exit 1 on an otherwise valid session, while an empty --until 1h window returned zero rows cleanly. This may be API-side duration handling, but the CLI exposes it directly, so the command should either validate supported duration forms or the endpoint needs to stop returning a 500 for an empty/recent window.
Which we should resolve server side
Once the flagged bugbot issue is addressed happy to approve
Sayan-
left a comment
There was a problem hiding this comment.
Thanks for iterating - confirming all marked issues have been addressed
Layer additive options onto the existing telemetry events command: - --categories: server-side filter, sent as repeated category query params (the SDK comma-joins the typed field, which the endpoint will not match) - --types: client-side filter; walks every page in the window so matches on later pages are not dropped - --all: walk every page in the window instead of just the first - fall back to the raw identifier when Get 404s, so the archive of an ended session stays readable - validate --limit is within 1-100 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1fa9194 to
be15bdc
Compare
--all and --types walk the whole window via the auto-pager, but --offset was still forwarded, so the scan started mid-window and dropped earlier pages — contradicting the documented behavior that --all ignores --offset. Gate the offset on the single-page path only; a full scan now walks from --since. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7f2f6be. Configure here.
The session-resolution fallback treated every browsers.Get failure as an ended session, swallowing auth/5xx/transient errors and querying the archive with an unresolved id. Restrict the fallback to 404 (via util.IsNotFound) and surface any other error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Rebased onto
mainnow that thebrowsers telemetry eventscommand and thekernel-go-sdkv0.72.0 bump landed separately. This PR no longer adds the command or bumps the SDK — it layers additive options onto the existing command:--categories— server-side filter, sent as repeatedcategoryquery params (the SDK comma-joins the typed field, which the endpoint will not match)--types— client-side filter; walks every page in the window so matches on later pages are not dropped--all— walk every page in the window instead of just the first (ignores--offset; nonext_offsetis returned)Get404s, fall back to the raw identifier so an ended session's archive stays readable--limitrange validation (1-100)The base command's
--offset/X-Next-Offsetcursor paging,{ "events": [...], "next_offset": "..." }JSON shape, and table output are unchanged.Review feedback
Covers the original review:
--typesparity withtelemetry stream--offset) plus--all--limitrange validationOut of scope (server-side):
--since 1sreturning a 500 on a recent/empty window — to be fixed in the API.Test plan
go build ./.../go vet ./.../gofmtcleango test ./...passing locallymake lintin CI--categories,--types,--all,--limitbounds,-o json, ended-session idNote
Low Risk
CLI-only additive flags and paging logic on an existing command; behavior change is opt-in via new flags, with solid test coverage.
Overview
Extends
kernel browsers telemetry eventswith options aligned totelemetry stream, without changing default single-page behavior or JSON/table output shape.--categoriesis applied server-side via repeatedcategoryquery params (workaround for SDK comma-joining).--typesis filtered client-side and forces a full-window walk so later pages are not missed.--allwalks every page from--since, ignores--offset, and omitsnext_offset.--limitis validated to 1–100.Session resolution now tolerates
Get404 (ended session) by using the raw identifier so archives remain readable; otherGeterrors still fail. README and cobra flag help document the new behavior. Tests cover validation, query encoding, 404 fallback, auto-paging paths, and full-scan pagination rules.Reviewed by Cursor Bugbot for commit 003dfa6. Bugbot is set up for automated code reviews on this repo. Configure here.