[kernel-1116] browser events add s2 storage support#235
Conversation
…ies and fix delimiter in CategoryFor
… and category validation
…alloc in filewriter, fix session doc
This binary is tracked on main and was incidentally deleted earlier on this branch. Restoring it keeps the 13.4MB binary out of this PR's diff. Removing the tracked binary from main should be done in a separate PR.
|
Firetiger deploy monitoring skipped This PR didn't match the auto-monitor filter configured on your GitHub connection:
Reason: PR title and branch name suggest feature work on browser events/s2 storage, but no file changes are visible to confirm modifications to API endpoints or Temporal workflows. To monitor this PR anyway, reply with |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
…rflow as system event
…e gracefully on s2 init failure
… and RemoveSession
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit df53c74. Configure here.
rgarcia
left a comment
rgarcia
left a comment
There was a problem hiding this comment.
need to resolve #227 first and get a global event stream in place/moving ring/seq off capturesession
and then i think this pr is more reviewable
also in a similar vein instead of one s2 writer per capture session i think there's one global s2 writer that is reading from the global event stream
| @@ -1,5 +1,5 @@ | |||
| [program:kernel-images-api] | |||
| command=/bin/bash -lc 'mkdir -p "${KERNEL_IMAGES_API_OUTPUT_DIR:-/recordings}" && PORT="${KERNEL_IMAGES_API_PORT:-10001}" FRAME_RATE="${KERNEL_IMAGES_API_FRAME_RATE:-10}" DISPLAY_NUM="${KERNEL_IMAGES_API_DISPLAY_NUM:-${DISPLAY_NUM:-1}}" MAX_SIZE_MB="${KERNEL_IMAGES_API_MAX_SIZE_MB:-500}" OUTPUT_DIR="${KERNEL_IMAGES_API_OUTPUT_DIR:-/recordings}" LOG_CDP_MESSAGES="${LOG_CDP_MESSAGES:-false}" exec /usr/local/bin/kernel-images-api' | |||
| command=/bin/bash -lc 'mkdir -p "${KERNEL_IMAGES_API_OUTPUT_DIR:-/recordings}" && PORT="${KERNEL_IMAGES_API_PORT:-10001}" FRAME_RATE="${KERNEL_IMAGES_API_FRAME_RATE:-10}" DISPLAY_NUM="${KERNEL_IMAGES_API_DISPLAY_NUM:-${DISPLAY_NUM:-1}}" MAX_SIZE_MB="${KERNEL_IMAGES_API_MAX_SIZE_MB:-500}" OUTPUT_DIR="${KERNEL_IMAGES_API_OUTPUT_DIR:-/recordings}" LOG_CDP_MESSAGES="${LOG_CDP_MESSAGES:-false}" S2_BASIN="${S2_BASIN:-}" S2_ACCESS_TOKEN="${S2_ACCESS_TOKEN:-}" exec /usr/local/bin/kernel-images-api' | |||
There was a problem hiding this comment.
should there also be an s2 stream config?
There was a problem hiding this comment.
stream names are derived dynamically from the CaptureSessionID at runtime, so one S2 stream is created per capture session within the configured basin
There was a problem hiding this comment.
this will make it hard for, e.g. the API to know what stream to read when it wants to stream telemetry for a browser. It also is at odds with "one global event stream" that the cdp stuff publishes into. I would make this a constant that is injected on startup
|
|
||
| var _ oapi.StrictServerInterface = (*ApiService)(nil) | ||
|
|
||
| // New constructs an ApiService. |
| resp.Status = oapi.CaptureSessionStatusStopped | ||
| // Session cleanup (Remove on the S2 producer) happens automatically in | ||
| // EventsStorageWriter.Run when it processes the SessionEnded event, ensuring | ||
| // all pending writes are flushed before the producer is torn down. |
There was a problem hiding this comment.
this comment feels out of place. remove or move closer to the things it's talking about?
|
|
||
| // EventsStorage is the durable storage interface for the storage writer. | ||
| type EventsStorage interface { | ||
| Append(ctx context.Context, streamName string, data []byte) error |
There was a problem hiding this comment.
i'd couple the storage to the envelope: Append(ctx, env Envelope) error.
| TypeEventsDropped = "events_dropped" | ||
| SessionEnded = "session_ended" | ||
| EventsDropped = "events_dropped" | ||
| EventsStorageError = "storage_error" |
There was a problem hiding this comment.
i don't think we should publish this as an event--create's a feedback loop and exposes details that make more sense as logs
2 participants
Link to example S2 logs -> https://s2.dev/dashboard/basins/dev-capture-session-logs/studio?stream=gpy8681tmxg5j8zk69v5grzl
Note
Medium Risk
Adds a new durable event sink and modifies shutdown ordering to drain/flush events before teardown, which could affect event delivery and shutdown behavior when enabled. When misconfigured/unavailable it should fall back, but the new goroutines and external S2 dependency add operational risk.
Overview
Adds optional durable event storage to S2 for capture-session envelopes, controlled via new env vars
S2_BASIN,S2_ACCESS_TOKEN, and batch tuning (S2_BATCHER_LINGER_MS,S2_BATCHER_MAX_RECORDS).Introduces
EventsStorageWriterplus anS2Storagebackend that appends envelopes to per-session S2 streams, emits a new reserved system eventstorage_erroron append failures, and evicts per-session producers onsession_ended.Updates server startup/shutdown to run the writer in its own context and stop the capture session before cancelling/closing storage, ensuring
session_endedis persisted beforeCaptureSession.Close(); wires the new env vars through docker run scripts and supervisord configs, and updates reserved event-type constants/usages accordingly.Reviewed by Cursor Bugbot for commit bf059ca. Bugbot is set up for automated code reviews on this repo. Configure here.