Repository navigation
fix(producer): render telemetry reports beyond-viewport capture as the workers used it - #6702
Conversation
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 276e3cf2
- blocker The segmented path still reports the requested value, not the one the workers used. In
renderOrchestrator.ts:4899-4904thesegmentedRes.successbranch never updatescaptureBeyondViewport. So observability keepsresolvedCaptureBeyondViewport, which:3661set to the video request. Segmented workers already push their summaries intodedupPerfs(captureSegmentedStage.ts:435,:611), and those summaries now carry the field. Repro: a long-form render with a native video, on a page that fits, planned assdr_segmented, still reportscaptureBeyondViewport: trueeven though every worker loggeddowngraded. That is the same bug on a third multi-worker path. Fix: afterupdateCaptureObservability({ workerCount }), set the field fromcapturedBeyondViewport(dedupPerfs, undefined)when it isn't undefined, and add a test that covers it.
Scope
Read the full diff, the three orchestrator capture branches, the stages that push into dedupPerfs, and the init downgrade (frameCapture.ts:2951). Ran it locally: the 4 named tests pass at head, and the engine test plus the 3 capturedBeyondViewport tests fail with src reverted to main, so the 'fails without the fix' line holds. CI was still in progress when I posted this. Retry accumulation in dedupPerfs on the streaming path predates this PR, and the downgrade depends only on the page, so attempts agree. Not flagged.
|
Fixed in 95994ee, at the root rather than per branch: the observability summary (built once on success and once on failure) now applies the sessions' value from |
Edit accuracy: accurate 271 (base branch 271), smooth 114 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
There was a problem hiding this comment.
Verdict: APPROVE · 1 resolved, 0 open, 0 new · 276e3cf → 95994ee
The segmented-path blocker is fixed: withCapturedBeyondViewport is applied in both observability.summary calls (success and failure), and those are the only places captureObservability is read, so it covers every capture path. Tests pass 12 + 10 locally. Required CI is green after a rerun: the Windows engine-cli lane hit its 16-minute deadline with every engine test passing. Comments fails, but it isn't required.
…e workers used it
…ry, on every capture path
b9d040c to
aa7ca08
Compare
There was a problem hiding this comment.
Approve. No blockers at aa7ca084.
- Fails without the fix: checked locally. At this head,
frameCapture-discardWarmup.test.tspasses 10/10. I putengine/src/services/frameCapture.tsback to the merge-base version andgetCapturePerfSummary > reports the beyond-viewport value the session captured withfails withexpected undefined to be false. - Where the summaries come from:
- Every summary in
dedupPerfscomes from a session that captured frames: the parallel workers throughparallelCoordinatorandpushWorkerDedupPerfs, the segmented workers, and the sequential or streaming session. - The probe session's summary is never added, so a probe that kept it on can't make the result read on.
- "On if any session kept it" is the correct way to combine them.
- Every summary in
- When no session reported: if no summary carries the field,
withCapturedBeyondViewportreturns the capture facts unchanged. Renders that report nothing look the same as before. - Both summaries: the success and failure observability summaries both apply it. The
capture_strategycheckpoint still records the plan, as the body says. - CI at review time: preview-regression, Player perf, PR captures and Comments pass. CI, regression, Windows render verification and CodeQL were still running.
Description
Render telemetry can say a render captured beyond the viewport when none of its frames did. A render with a native video asks for beyond-viewport capture, and each capture session turns that request off at init when the page fits the frame. On a parallel render, though, the value reported after capture came from the probe session, not from the workers that captured. A 4-worker render of a page that fits reported
captureBeyondViewport: true, while all four workers loggedcaptureBeyondViewport downgradedand every screenshot was taken with it off. That makes the field useless for telling whether renders pay for beyond-viewport capture.Each session's perf summary now carries the value it captured with. Every multi-worker capture path (streaming, disk and segmented) already collects those summaries, and the render's observability summary already merges per-worker facts from them. That summary, on success and on failure alike, now applies the sessions' value: on if any session kept it. When no session reported one, the value recorded during the render stands, as before. Doing it at the summary covers every capture path, including any added later.
The
capture_strategycheckpoint is unchanged. It records the plan before any session starts, so it still shows the request.Testing
fails without the fix:
getCapturePerfSummary > reports the beyond-viewport value the session captured with(engine) and thecapturedBeyondViewportandwithCapturedBeyondViewporttests inperfSummary.test.ts(producer) fail on main.frameCapture-discardWarmup.test.tsandperfSummary.test.ts: 10 and 12 passed, 3 runs in a row.tsc --noEmitfor engine and producer: clean. Pre-commit (lint, format, fallow, typecheck): clean.What I did NOT exercise
No real render on this branch. The before state (trace true, four downgrades, every screenshot with it off) comes from a 4-worker render on a Linux box on main.
Why under 100 lines
The workers already report a perf summary to the orchestrator on both parallel paths; this adds one field to it and applies it at the two places the summary is built.
Size
A lone fix: no open PR of ours touches this code, so it ships on its own.