Repository navigation
feat(ext/node): emit perf_hooks PerformanceEntry for http2 sessions and streams - #33618
Conversation
…nd streams Enables tests/node_compat/runner/suite/test/parallel/test-http2-perf_hooks.js Co-authored-by: Divy Srivastava <[email protected]>
Signed-off-by: Divy Srivastava <[email protected]>
fibibot
left a comment
There was a problem hiding this comment.
The implementation is well-structured. Three coordinated pieces:
- Rust frame counters:
frames_received/frames_sentincremented in the nghttp2on_frame_recv_callback/on_frame_send_callback, exposed via#[fast] #[smi] fn frames_received/sent()ops. Counter type isu32withsaturating_addso it won't panic on overflow at the cost of saturating after 4B frames — fine for any reasonable session. - JS perf-entry plumbing:
kPerfStats/kPerfEmittedSymbols on each session/stream,emitSessionPerfEntry/emitStreamPerfEntrycalled fromfinishSessionCloseand the stream-close path. UsesenqueuePerformanceEntry({ name, entryType: "http2", startTime, duration, detail })matching Node's PerformanceEntry shape for http2. - Test enrollment alphabetically positioned correctly (
padding-aligned<perf_hooks<ping-settings-heapdump).
Holding off on autonomous APPROVE for two reasons per fibibot's policy:
-
CI not yet complete — 80/n checks passing, none failed, but the relevant
test node_compat debugshards on most platforms are still queued. -
Cross-cutting infrastructure change: this PR adds two new public exports to
ext/web/15_performance.js(enqueuePerformanceEntry,registerExtraEntryType) plus a module-levelextraEntryTypesSet that thePerformanceObserverconstructor now consults. That's not justnode:http2plumbing — it's new infra that any future polyfill (node:dns,node:http,node:net, etc.) can hook into to surface their own performance entries. Worth a maintainer's deliberate sign-off on the API shape (registerExtraEntryTypeglobal registry vs. per-observer subscription,enqueuePerformanceEntryexposingqueuePerformanceEntrydirectly with no validation, etc.).
cc @bartlomieju on the ext/web/15_performance.js API additions — would appreciate your read on whether the extraEntryTypes registry is the right shape for general perf_hooks integration, or if the http2 case should be more contained.
Two non-blocking inline notes below.
| // about "mark" and "measure"; this lets non-web entry types be observed | ||
| // without having to fork the whole observer machinery. | ||
| const extraEntryTypes = new SafeSet(); | ||
| function registerExtraEntryType(type) { |
There was a problem hiding this comment.
API-shape note worth bartek's input on:
registerExtraEntryType(type)is write-once, never-deregister — module-level Set that grows monotonically. Multipleregistercalls for the same type are idempotent (Set handles dedup), so that's fine.- But the registry is global to the isolate, which means a module that registers
"http2"then gets unloaded would still leave the type recognized. Probably not an issue (modules don't unload in practice), butregisterExtraEntryTypebeing global rather than scoped to the registering module makes it harder to reason about ownership. - No validation on
type: passing aSymbolor empty string would silently corrupt the Set. Worth atypeof type !== "string"guard at minimum, since this is a public-facing helper that Node polyfills will call.
Filing these as observations on the API shape, not as blockers.
| // plain object (or PerformanceEntry) with `name`, `entryType`, `startTime`, | ||
| // `duration` properties, and any extra fields (`detail`, etc.) the consumer | ||
| // expects. | ||
| function enqueuePerformanceEntry(entry) { |
There was a problem hiding this comment.
enqueuePerformanceEntry is currently a thin re-export of queuePerformanceEntry, which doesn't validate the entry has the expected name/entryType/startTime/duration properties. The http2 caller in this PR builds entries by hand (plain object literal, not new PerformanceEntry), and queuePerformanceEntry just passes those through to observers' entryLists.
If a future polyfill calls this with a malformed entry, observers will get garbage with no fast-fail path. Worth either:
- Adding a
webidl.converters["PerformanceEntry"]-style coercion here, so the helper produces a realPerformanceEntry(or rejects), or - Documenting the contract ("caller must pass a frozen plain object with these fields") and leaving validation to the call sites.
The http2 caller's entries match the contract, so this is forward-looking guidance more than a blocker.
…rformanceEntry Reject non-string `type` and malformed entries with TypeError so polyfills fail fast instead of silently corrupting observer state. Co-authored-by: Divy Srivastava <[email protected]>
fibibot
left a comment
There was a problem hiding this comment.
Delta addresses my prior "no validation" concern on enqueuePerformanceEntry. Verified:
registerExtraEntryType(type)now throwsTypeErroron non-string or empty-string. ✓enqueuePerformanceEntry(entry)validatesentryis non-null object with required fields (name,entryType,startTime,duration) of the right type. Extra fields likedetailpass through untouched, which matches the docstring. ✓TypeErroris the primordial (imported at15_performance.js:19), so the throw can't be hijacked. ✓- Validation happens before
queuePerformanceEntry, so an observer never sees a malformed entry. ✓
CI fully green: 133 SUCCESS, 0 FAILURE, 2 SKIPPED. All test node_compat × all platforms × debug+release pass.
Holding to COMMENT rather than APPROVE — same architectural escalation as my prior review. The validation patches the obvious foot-gun, but the bigger call still lives with the maintainer:
extraEntryTypesis a global set that mutatesPerformanceObserver's allowed entry types for the whole isolate. Oncenode:http2callsregisterExtraEntryType("http2"), everyPerformanceObserver({ entryTypes: ["http2"] })becomes legal everywhere, including in code that never touched http2.- An alternative (per-polyfill or per-observer subscription) would scope this. Not necessarily wrong as is, but it's a one-way door for the API shape.
cc @bartlomieju — registry-shape decision is the call worth deliberately signing off.
… addressed the actionable Enables tests/node_compat/runner/suite/test/parallel/test-http2-perf_hooks.js Co-authored-by: Divy Srivastava <[email protected]>
Co-authored-by: Divy Srivastava <[email protected]>
Co-authored-by: Divy Srivastava <[email protected]>
Co-authored-by: Divy Srivastava <[email protected]>
Adapt enqueuePerformanceEntry/registerExtraEntryType to lazy-loaded script pattern from denoland#33760: 15_performance.js now returns its exports from an IIFE; consumers (perf_hooks.js, http2.ts) load via core.loadExtScript instead of static ESM import. Co-authored-by: Divy Srivastava <[email protected]>
…r review, normal) Enables tests/node_compat/runner/suite/test/parallel/test-http2-perf_hooks.js Co-authored-by: Divy Srivastava <[email protected]>
Re-added in 7d76a6f; broke top-level-entry lint check. Co-authored-by: Divy Srivastava <[email protected]>
|
@littledivy heads-up: the bot's analysis of this PR's CI failures says they look unrelated/flaky and not addressable from this PR's diff. Verdict:
Please verify and either rerun the failing checks (admin needed), waive them, or merge if the green checks are sufficient. Pinged once; the bot won't re-engage on the same signals. |
Resolves overlap with denoland#33796 (Http2Session PerformanceObserver entries): - Drop changes to ext/web/15_performance.js (registerExtraEntryType / enqueuePerformanceEntry); main's denoland#33796 implements perf entries via a contained nodeObservers registry in node:perf_hooks instead, addressing fibibot's architectural concern. - Use enqueueNodePerformanceEntry from node:perf_hooks; keep this PR's fuller emit helpers (Http2Stream entries + bytesRead/bytesWritten/ pingRTT/streamAverageDuration/streamCount + framesSent). - Deduplicate Session.frames_received field/initializer/op (added by both PRs). Co-authored-by: Divy Srivastava <[email protected]>
Head branch was pushed to by a user without write access
Resolves conflict in ext/node/ops/http2/session.rs on_frame_send_callback: combined frames_sent counter (this PR) with GOAWAY error_code capture (main denoland#33795) in a single callback body. Co-authored-by: Divy Srivastava <[email protected]>
Head branch was pushed to by a user without write access
…d streams (denoland#33618) Enables `test-http2-perf_hooks` in node_compat suite. Co-authored-by: Divy Srivastava <[email protected]>
Summary
Enables
test-http2-perf_hooksin node_compat suite.Test plan
cargo test --test node_compat -- test-http2-perf_hooks