Skip to content

[kernel-1116] rename capture session -> browser telemetry#242

Merged
archandatta merged 28 commits into
mainfrom
archand/kernel-1116/rename-browser-telemetry
May 15, 2026
Merged

[kernel-1116] rename capture session -> browser telemetry#242
archandatta merged 28 commits into
mainfrom
archand/kernel-1116/rename-browser-telemetry

Conversation

@archandatta
Copy link
Copy Markdown
Contributor

@archandatta archandatta commented May 14, 2026

High Level Changes

Endpoints:
- GET /telemetry — get current state (TelemetryState), 404 if not active
- PUT /telemetry — create/replace config (BrowserTelemetryConfig), returns 201 on start or 200 on replace
- PATCH /telemetry — update active config, 404 if not active
- POST /telemetry/events — publish an event into the bus
- GET /telemetry/stream — SSE stream of events, supports Last-Event-ID for resumption

Key schemas:
- BrowserTelemetryConfig — top-level config with a browser key → BrowserTelemetryCategoriesConfig
- BrowserTelemetryCategoriesConfig — four categories: console, network, page, interaction, each with { enabled: bool }
- Event — { ts, type, category, source, data, truncated }, category enum: console | network | page | interaction | system
- TelemetryState — { id, status (running|stopped), config, seq, created_at }
- PublishedEnvelope — { seq, event } — what the SSE stream and publish response return

Note

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_session API and internal capturesession package with a new browser telemetry model (GET/PUT/PATCH /telemetry) where telemetry is stopped by setting all categories to enabled:false (no DELETE endpoint).

Renames/relocates event ingress and streaming to POST /telemetry/events and GET /telemetry/stream, updates tests and e2e S2 storage coverage accordingly, and wires cdpmonitor to publish through TelemetrySession (stamping telemetry_session_id).

Refactors the event schema to use OpenAPI-generated types (TelemetryEventCategory, BrowserEventSource, typed data payloads), adds page_lcp as 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.

@firetiger-agent
Copy link
Copy Markdown

Firetiger deploy monitoring skipped

This PR didn't match the auto-monitor filter configured on your GitHub connection:

Any PR that changes the kernel API. Monitor changes to API endpoints (packages/api/cmd/api/) and Temporal workflows (packages/api/lib/temporal) in the kernel repo

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 @firetiger monitor this.

Comment thread server/cmd/api/api/telemetry.go
Comment thread server/cmd/api/api/telemetry.go Outdated
Comment thread server/cmd/api/api/telemetry.go
Comment thread server/cmd/api/api/telemetry.go
Comment thread server/cmd/api/api/telemetry.go
@archandatta archandatta requested review from Sayan- and rgarcia May 14, 2026 15:00
Comment thread server/cmd/api/api/telemetry.go Outdated
Copy link
Copy Markdown
Contributor

@rgarcia rgarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:40PUT /telemetry with all four categories disabled currently returns 400 when telemetry is inactive. since disabling is now expressed through config rather than DELETE, I think this should be idempotent: return 200 with status: stopped.

  • server/cmd/api/api/telemetry.go:44 and server/cmd/api/api/telemetry.go:88 — the stopped response is built from the previous active config, so an all-disabled PUT/PATCH returns status: stopped but 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 — for PATCH, I prefer the behavior where only categories with explicit non-nil enabled are changed. But the docs/schema should say that clearly, because the category schema currently says omitted enabled defaults 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 to capture_session_id; should be telemetry_session_id.

@archandatta archandatta requested a review from rgarcia May 14, 2026 17:02
Copy link
Copy Markdown
Contributor

@rgarcia rgarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, this is close. remaining fixes from my side:

  1. for the idempotent all-disabled PUT /telemetry path 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.

  1. 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. |
  1. 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.

  1. schema alignment thing: i think server/openapi.yaml in 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"
    # ...

Comment thread server/lib/cdpmonitor/screenshot.go Outdated
Comment thread server/lib/cdpmonitor/screenshot.go
LoaderId: ptrOf(lid),
Url: ptrOf(url),
NavSeq: nseq,
CdpTimestamp: float32(p.Timestamp),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

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.
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ 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.

Comment thread server/lib/cdpmonitor/types.go
@archandatta archandatta requested a review from rgarcia May 15, 2026 15:57
Copy link
Copy Markdown
Contributor

@rgarcia rgarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two small wording nits before merge:

  1. server/lib/cdpmonitor/README.md now says:
| `seq` | uint64 | Monotonically increasing per-telemetry-session sequence number. |

i think this should say process-monotonic across config changes.

  1. 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"

@archandatta archandatta merged commit 08fbd6d into main May 15, 2026
10 checks passed
@archandatta archandatta deleted the archand/kernel-1116/rename-browser-telemetry branch May 15, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants