[kernel-1116] browser events add s2 storage support#235
[kernel-1116] browser events add s2 storage support#235archandatta wants to merge 19 commits intoarchand/kernel-1116/external-eventsfrom
Conversation
|
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.