[kernel-1116] rename capture session -> browser telemetry#242
Conversation
|
Firetiger deploy monitoring skipped This PR didn't match the auto-monitor filter configured on your GitHub connection:
Reason: PR title suggests a rename/refactoring of telemetry terminology, but no file changes are provided to confirm modifications to API endpoints or Temporal workflows. To monitor this PR anyway, reply with |
rgarcia
left a comment
There was a problem hiding this comment.
shape looks right to me overall — this lines up with the declarative telemetry API direction: hard break from events/capture_session, no DELETE /telemetry, /telemetry/stream for SSE, and /telemetry/events for arbitrary event injection.
a couple things I’d tweak before merge:
-
server/cmd/api/api/telemetry.go:40—PUT /telemetrywith all four categories disabled currently returns400when telemetry is inactive. since disabling is now expressed through config rather thanDELETE, I think this should be idempotent: return200withstatus: stopped. -
server/cmd/api/api/telemetry.go:44andserver/cmd/api/api/telemetry.go:88— the stopped response is built from the previous active config, so an all-disabledPUT/PATCHreturnsstatus: stoppedbut still shows the old enabled categories. I’d return the effective disabled config in that response. -
server/openapi.yaml:1248/server/cmd/api/api/telemetry.go:177— forPATCH, I prefer the behavior where only categories with explicit non-nilenabledare changed. But the docs/schema should say that clearly, because the category schema currently says omittedenableddefaults to true. For PATCH specifically, maybe document that{ "console": {} }is a no-op and callers must send{ "console": { "enabled": true } }or{ "console": { "enabled": false } }to enable / disable. -
server/lib/cdpmonitor/README.md:92— stale docs still refer tocapture_session_id; should betelemetry_session_id.
rgarcia
left a comment
There was a problem hiding this comment.
thanks, this is close. remaining fixes from my side:
- for the idempotent all-disabled
PUT /telemetrypath when telemetry is already unset, can we return the same disabled config shape as the other all-disabled paths?
right now the inactive path returns only:
TelemetryState{Status: TelemetryStateStatusStopped}but the active PUT/PATCH unset paths return Config: disabledConfig(). i’d make the inactive path return Config: disabledConfig() too so the response consistently reflects the effective config.
- server/lib/cdpmonitor/README.md still has stale capture-session wording in the field table:
| `capture_session_id` | string | Pipeline-assigned ID for the capture session (not a CDP concept). |
| `seq` | uint64 | Monotonically increasing per-capture-session sequence number. |
- one broader api-shape note: i did another pass over server/openapi.yaml and don’t see capture_session_id / “capture session” left there. however, the telemetry openapi still has a lot of stateful lifecycle language: “state”, “active”, “starts telemetry”, “stops telemetry”, status: running|stopped, created_at, “session was started”, etc.
i think we should scrub that language before merging so this reads as declarative config setting/unsetting rather than a start/stop session API. concretely, the remaining places are around /telemetry descriptions and the TelemetryState schema.
- schema alignment thing: i think
server/openapi.yamlin kernel-images should own the browser telemetry event schemas too.
kernel/kernel#1965 is adding a full BrowserTelemetryEvent discriminated union with schemas for console/network/page/interaction/monitor events. since kernel-images is the actual producer of these events, the image OpenAPI should expose the same event taxonomy/payload schemas rather than only the current generic Event { type, category, source, data } shape. otherwise the public API can drift from the image API and we end up documenting the concrete event contract downstream instead of at the source. also code that produces events should use the generated oapi types here when constructing events. keep the internal events.Event envelope generic, but use generated oapi payload structs at the construction points before marshaling into Data.
one nuance: because POST /telemetry/events intentionally allows arbitrary caller-published events, the stream schema probably should not be only BrowserTelemetryEvent. maybe something like this would work?
TelemetryEnvelope:
type: object
required: [seq, event]
properties:
seq:
type: integer
format: int64
event:
$ref: "#/components/schemas/TelemetryEvent"
TelemetryEvent:
type: object
required: [type]
properties:
type:
type: string
category:
type: string
enum: [console, network, page, interaction, system]
source:
$ref: "#/components/schemas/EventSource"
data:
description: >
Arbitrary JSON payload. For Kernel-generated browser events, see the
known browser telemetry event schemas for the expected shape.
truncated:
type: boolean
KnownBrowserTelemetryEvent:
oneOf:
- $ref: "#/components/schemas/BrowserConsoleLogEvent"
- $ref: "#/components/schemas/BrowserNetworkRequestEvent"
# ...
… typed oapi structs
| LoaderId: ptrOf(lid), | ||
| Url: ptrOf(url), | ||
| NavSeq: nseq, | ||
| CdpTimestamp: float32(p.Timestamp), |
There was a problem hiding this comment.
CDP timestamps lose precision via float32 cast
Medium Severity
CdpTimestamp: float32(p.Timestamp) truncates the CDP float64 monotonic timestamp to float32, which only has ~7 significant digits. For a browser running several hours (e.g., timestamp 86400.123456), sub-millisecond precision is lost. The OpenAPI spec declares cdp_timestamp as type: number without format: double, causing oapi-codegen to generate float32 — adding format: double to the spec would fix the generated type.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5967aa5. Configure here.
Drop id, status, created_at from TelemetryState so the /telemetry API reads as declarative config setting/unsetting rather than a start/stop session API. Also clean up remaining 'not active' / 'status: stopped' phrasing in path descriptions and handler docs.
…artial-context events
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d6d80bb. Configure here.
rgarcia
left a comment
There was a problem hiding this comment.
two small wording nits before merge:
server/lib/cdpmonitor/README.mdnow says:
| `seq` | uint64 | Monotonically increasing per-telemetry-session sequence number. |i think this should say process-monotonic across config changes.
- in
server/openapi.yaml, the /telemetry/stream description says the event’s data conforms to variants of KnownBrowserTelemetryEvent.
KnownBrowserTelemetryEvent is full-event shaped, not data-shaped, so i’d tweak the wording to something like:
"data conforms to the matching Browser*EventData schema, selected by type"


High Level Changes
DELETEendpoint is gone, stop telemetry is controlled via setting all values to false -> https://onkernel.slack.com/archives/C0B10F24TG9/p1778620560896099?thread_ts=1778586830.702399&cid=C0B10F24TG9Note
Medium Risk
Moderate risk because it changes public API endpoints and refactors the event pipeline to rely on new OpenAPI-generated types and a new telemetry session lifecycle, which could impact event delivery/compatibility.
Overview
Replaces the
capture_sessionAPI and internalcapturesessionpackage with a new browser telemetry model (GET/PUT/PATCH /telemetry) where telemetry is stopped by setting all categories toenabled:false(no DELETE endpoint).Renames/relocates event ingress and streaming to
POST /telemetry/eventsandGET /telemetry/stream, updates tests and e2e S2 storage coverage accordingly, and wirescdpmonitorto publish throughTelemetrySession(stampingtelemetry_session_id).Refactors the event schema to use OpenAPI-generated types (
TelemetryEventCategory,BrowserEventSource, typeddatapayloads), addspage_lcpas a distinct event, and updates screenshot/lifecycle/computed events to emit typed payloads and process-monotonic sequencing.Reviewed by Cursor Bugbot for commit 381ffa4. Bugbot is set up for automated code reviews on this repo. Configure here.