Add bounded web performance capture API - #167
Conversation
📊 PR Size: size/XLTotal changes: 2317 lines (6 files) Top files changed:
Size calculated as additions + deletions. Labels: XS (<10), S (<50), M (<250), L (<1000), XL (1000+) |
clholgat
left a comment
There was a problem hiding this comment.
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 /summarizeTracesstating 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
setTimeoutwithout.unref(). Benign during operation and cleared onclose(), but they can hold the Node event loop open if shutdown ever bypassescontroller.close(). Consider.unref()on both.
Description
Adds a dormant Chromium recording controller and bounded loopback API for web performance capture.
Type of Change
Testing
bazel test //...)Testing Details
npm testpassed 436/436; the CLI production build passed.//src/valdi_modules/src/valdi/web_renderer:testpassed.bazel query //...passed.Checklist
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 commitf7ad9a8aagainst that base; do not merge it before its parent.