Skip to content

Add bounded web performance capture API - #167

Draft
bjdodson-openai wants to merge 1 commit into
bjd/debugger-web-tracing-compatfrom
bjd/debugger-web-performance-api
Draft

Add bounded web performance capture API#167
bjdodson-openai wants to merge 1 commit into
bjd/debugger-web-tracing-compatfrom
bjd/debugger-web-performance-api

Conversation

@bjdodson-openai

Copy link
Copy Markdown
Collaborator

Description

Adds a dormant Chromium recording controller and bounded loopback API for web performance capture.

  • Enforces one exact global CDP tracing owner and fail-closed uncertain termination.
  • Normalizes trace output under response, event, string, and lifecycle bounds.
  • Separates the capture API from the later presentation layer.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation improvement
  • Performance optimization
  • Test improvement
  • Other (new debugger capability)

Testing

  • Tests pass locally (bazel test //...)
  • Added/updated tests for changes (if applicable)
  • Tested on multiple platforms (iOS/Android/Web/macOS as applicable)
  • Manual testing performed (describe below)

Testing Details

  • Incremental branch: Recorder/server 89/89, full CLI 331/331, production build, and repository query passed; the isolated exact-next-port rerun and subsequent suites passed.
  • Assembled debugger stack: npm test passed 436/436; the CLI production build passed.
  • Focused //src/valdi_modules/src/valdi/web_renderer:test passed.
  • bazel query //... passed.
  • The broad Valdi suite reproduced the established 12 unrelated failures; all new debugger specs passed.

Checklist

  • Code follows project style guidelines
  • Documentation updated (if needed)
  • No breaking changes (or documented in description)
  • Commit messages follow conventional format
  • No secrets, API keys, or internal URLs included

Related Issues

Relates to #154

Additional Context

Stack 14/22. Stacked on #166 (bjd/debugger-web-tracing-compat). Review this PR as the single incremental commit f7ad9a8a against that base; do not merge it before its parent.

@github-actions

Copy link
Copy Markdown

📊 PR Size: size/XL

Total changes: 2317 lines (6 files)

Top files changed:

  • npm_modules/cli/src/debugger/webPreviewPerformance.ts: +1021 -0
  • npm_modules/cli/src/debugger/server.spec.ts: +609 -42
  • npm_modules/cli/src/debugger/webPreviewPerformance.spec.ts: +392 -0
  • npm_modules/cli/src/debugger/server.ts: +226 -3
  • npm_modules/cli/src/utils/chromiumDevToolsClient.ts: +12 -1
  • npm_modules/cli/debugger/README.md: +11 -0

Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+)

@clholgat clholgat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review of the bounded web-performance capture API. This is well-hardened: genuinely opt-in/dormant until an explicit POST; event/pair/name-byte/duration/response caps all enforced (with the 4 MiB response rebuild correctly including the added browserMetrics/browserSummary fields); runExclusive enforces a single global CDP owner and a second start throws; the stop path's finally always calls connection.close() so tracing can't be left running; and recoverMissingOwner refuses to touch a possibly-live foreign trace. Trace-name allowlisting (Valdi.* / fixed BROWSER_TRACE_NAMES, dropping arbitrary user-timing names) is what keeps app-chosen strings out of the output. Two low notes below.

Note: please carry this feedback into the squashed PR when the stack is collapsed.

  • 🟢 Low — the trace-name allowlist is effectively the redaction mechanism for captured traces (unlike console output, trace payloads are not run through redactConsoleText). This is a deliberate and reasonable design, but it's load-bearing for the no-secret-leak guarantee. Please add a code comment at the allowlist / summarizeTraces stating that name-allowlisting is the intended redaction boundary, so a future change that widens accepted trace names doesn't silently reintroduce a leak.
  • 🟢 Low — the 15 s watchdog and 60 s completion-TTL timers are created with setTimeout without .unref(). Benign during operation and cleared on close(), but they can hold the Node event loop open if shutdown ever bypasses controller.close(). Consider .unref() on both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants