Skip to content

[kernel 1116] browser logging: cdp pipeline#194

Open
archandatta wants to merge 32 commits intomainfrom
archand/kernel-1116/cdp-pipeline
Open

[kernel 1116] browser logging: cdp pipeline#194
archandatta wants to merge 32 commits intomainfrom
archand/kernel-1116/cdp-pipeline

Conversation

@archandatta
Copy link
Copy Markdown
Contributor

@archandatta archandatta commented Apr 1, 2026

Scaffolding of the CDP Pipeline implementation on the functions are in -> #202


Note

Medium Risk
Introduces new capture-session lifecycle endpoints and wiring that affects API startup/shutdown and event persistence; while mostly scaffolding, it adds concurrency/lifecycle management and new disk writes that could misbehave under load or during shutdown.

Overview
Adds a new capture session resource backed by an events pipeline and CDP monitor scaffold, including POST/GET/PATCH/DELETE /events/capture_session to start, inspect, reconfigure (category filtering), and stop capture.

Wires the API service to own a long-lived events.CaptureSession and cdpmonitor.Monitor, constructs them in main, and ensures they are stopped/closed during ApiService.Shutdown.

Refactors the events package to support reusable sessions (Start/Stop), category validation/filtering, ring buffer reset, and safer file writing (create log dir, filename-based writes), and updates generated oapi types/clients plus new unit tests for the capture session handlers.

Reviewed by Cursor Bugbot for commit a7b2e54. Bugbot is set up for automated code reviews on this repo. Configure here.

@archandatta archandatta marked this pull request as ready for review April 2, 2026 15:19
Base automatically changed from archand/kernel-1116/browser-logging to main April 2, 2026 15:34
@archandatta archandatta force-pushed the archand/kernel-1116/cdp-pipeline branch from 1e544a7 to 6d929c8 Compare April 2, 2026 17:39
@archandatta archandatta force-pushed the archand/kernel-1116/cdp-pipeline branch from 6d929c8 to 76d837f Compare April 2, 2026 17:44
}

func (s *ApiService) Shutdown(ctx context.Context) error {
s.monitorMu.Lock()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this going to add a latency in delete browsers request? If so, how much?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't add any latency, shutdown holds the lock briefly to stop the CDP monitor and close the capture session i think both are fast synchronous teardown calls


s.captureSession.Start(uuid.New().String())

if err := s.cdpMonitor.Start(context.Background()); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the comment says this restarts an existing monitor, but the handler never checks/stops an already-running one. might be worth making that behavior explicit here instead of relying on Start() side effects.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah this was just a scaffolding I did for the actual implementation. I should've mentioned that there is a chained PR #202 that is capturing this https://github.com/kernel/kernel-images/pull/202/changes#diff-0ddb00e75e5f21a1deb16fb852c7f352f234bbf0b9ddcc3d11639b1d3293c572R90

// Generates a new capture session ID, seeds the pipeline, then starts the
// CDP monitor. If already running, the monitor is stopped and
// restarted with a fresh session ID
func (s *ApiService) StartCapture(w http.ResponseWriter, r *http.Request) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

question: is a no-arg /events/start intentional? i'd expect callers to want some capture config here over time (categories, detail level, screenshot/network verbosity, etc.), or at least an explicit note that this is a fixed server-defined profile for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah good point, added a body field to be able to send something like

  {
    "categories": ["console", "network"],
    "detailLevel": "verbose"
  }

}

// New creates a Monitor. displayNum is the X display for ffmpeg screenshots.
func New(_ UpstreamProvider, _ PublishFunc, _ int) *Monitor {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i don't think we'll need displayNum for a pure CDP monitor, so i'd remove this until actual implementation lands.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is handled in #202

@archandatta archandatta requested a review from rgarcia April 8, 2026 19:12
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.

api binary needs to be removed

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.
@archandatta archandatta requested a review from rgarcia April 9, 2026 17:23
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.

one small thing but otherwise i think this is good to go!

return oapi.StartCaptureSession400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: err.Error()}}, nil
}

id := uuid.New().String()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's use github.com/nrednav/cuid2 to be consistent with the rest of the api (and remove format: uuid for this id field in the openapi.yaml spec)

Copy link
Copy Markdown
Contributor

@Sayan- Sayan- left a comment

Choose a reason for hiding this comment

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

I think the last bugbot comment is worth resolving. Thanks for iterating - interfaces, components, and integration are looking great!

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.

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 a4fd0d6. Configure here.

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.

4 participants