feat(engine): measure live DOM size on every render, not just probed ones - #2891
Conversation
…ones
The short-comp routing gate can only read a live element count when a
probe session exists, and the first v0.7.83 data shows that is far rarer
than estimated: 17% of renders (86/503), not the ">=28%" the video-presence
proxy suggested. The other 83% fall back to a static source scan, which
is exactly blind to the shape that motivated the live count — small
markup, thousands of script-created nodes.
That leaves the fleet element-count distribution unknowable for most
renders, and the observed distribution is already surprising: p99 ~900,
max 1,420 against a 2,500 ceiling calibrated on 7k/20k/40k synthetic
nodes. Either the ceiling is close to irrelevant, or the large-DOM tail
is hiding in the 83% we cannot see. Both readings change what PR B
should do, and neither is decidable from probed renders alone (they are
a biased sample — they got a probe *because* they carry media or
unresolved compositions).
So measure it where every render already goes: capture-session init.
`collectSessionInitTelemetry` gains a querySelectorAll("*") count beside
the tween count it already collects, riding the same channel to
`observability_init_element_count`. This is observational only — capture
has begun, far too late to route on — and it deliberately does not feed
the gate. It answers the distribution question the gate cannot.
Coverage for this channel is proven rather than assumed: the tween-count
fix that shipped in v0.7.83 took the clamped-parallel bucket from 0/272
renders to 217/217, and 23.1% -> 100% overall.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Baseline-first sequencing done right — this is the #2875 R2 pattern applied to the short-comp band gate: the routing mechanism ships behind HF_DE_SHORT_BAND_ROUTE=false while attribution (applied / skipped_elements / unmeasured) is computed and emitted on every render, so the fleet distribution and the DiD control cohort are established BEFORE the flip. The R4-annotated regression test at renderOrchestrator.test.ts that walks the full probe → resolve → attribute chain against a 4000-span caption-generator composition is exactly what the earlier review rounds asked for, and resolveDeShortBand's fail-closed-on-unmeasured semantic prevents a static undercount from masquerading as a small comp — the R4 finding.
One framing note before findings: the Slack framing ("observational … without touching that routing gate itself") is true for RUNTIME behavior (flag defaults false, deInversionEligible = invertAtBaseFloor, no change to what actually routes) but the PR also adds all the machinery the follow-up flip will need — resolveCompositionElementCount, resolveDeShortBand, envInt, mergeWorkerInitObservability, countElementTags, and the two-floor wiring at renderOrchestrator.ts:2607-2705. That's the correct sequencing shape; just flagging so downstream reviewers don't skim expecting an "observability only" surface.
Concerns
-
countElementTagsfalse-positives on</letterinside inline<script>—renderOrchestrator.ts:1289regex's first alternation</[a-zA-Z]matches ANY</+ letter, including inside JS strings and template literals. The docstring addresses<letter(a < b) but not</letter. A compiled composition withconst html = "</div>"orarr.map(w => \`)inflates the static count for each occurrence. Sincecomposition_element_count(source:"static") is the ONLY signal for the ~83% of renders without a probe, systematic script-string inflation biases the fleet distribution upward for that cohort. Band routing itself is safe (the resolver correctly fails closed on"static"provenance), but the analytical goal — "readable fleet distribution" — is directionally right with a systematic bias whose magnitude depends on how script-heavy your compiled comps run. Worth documenting as a known fallback limitation, or skipping<script>...</script>` blocks before matching. -
elementCount = 0onpage.evaluatefailure is indistinguishable from an empty DOM —frameCapture.ts:423-427. Consistent with the existingtweenCountfallback pattern, but forelementCountthe ambiguity matters more: a page.evaluate crash inside init on a huge-DOM composition (the exact tail this PR wants to observe) emits the same 0 as a legitimately-empty comp, and the p50/p99 of the fleet distribution absorbs both without a way to filter. AnelementCount?: number(undefined on eval-fail) would let downstream analytics distinguish "measured 0" from "measurement failed" — same shaperesolveCompositionElementCountalready uses at the resolver seam (livevsstaticprovenance). Fixing bothelementCountandtweenCountat once is a larger refactor, but the new field is a natural place to break the pattern. -
Sampling bias: renders that fail before
collectSessionInitTelemetryruns are still absent —frameCapture.ts:408-451. The claim "every render reaches this path" holds only for renders that survive to end-of-init. Renders that crash during navigation, exhaust memory before init completes, or time out in browser launch never reach the emit — and on the "large-runtime-DOM tail" this PR is meant to reveal, those are exactly the cohort most likely to fail early. Not a design flaw (you can't measure a session that didn't initialize) but worth acknowledging in the docstring so the "closes the blind spot" claim doesn't over-index for readers doing rollout analysis. -
Two element-count fields with subtly different semantics —
observability_init_element_count(measured at end of init from the capture session's own DOM,frameCapture.ts:425) vscomposition_element_count(measured from the PROBE session's DOM when available,renderOrchestrator.ts:1324, else static scan). For most compositions these agree; for a comp that mutates DOM between probe launch and capture-session init they diverge. A dashboard reader needs to know which is authoritative for what question. Worth a one-liner in the type docs atengine/src/types.ts:246-259andproducer/observability.ts:71-93explicitly calling out thatcomposition_element_countis the routing-relevant signal andinit_element_countis the observational counterpart — otherwise the follow-up "PR B" analytics could easily query the wrong one.
Nits
-
envIntatrenderOrchestrator.ts:1242returnsparsedas-is, soHF_DE_SHORT_MAX_ELEMENTS=1.5returns1.5. The name implies integer-only. Consumers (compositionElementCount <= 1.5) still work sensibly sincecompositionElementCountis always integer, but aNumber.isIntegercheck (falling back to default on non-integer) orMath.truncwould match the name. -
document.querySelectorAll("*").lengthatframeCapture.ts:425andrenderOrchestrator.ts:1324—document.getElementsByTagName("*").lengthreturns a live collection whose.lengthis slightly cheaper than QSA's array snapshot. On 40k-node compositions this shaves a few ms; minor, but this is the exact tail where every ms in observability code lands directly on p95. -
resolveDeShortBandis called even whenHF_DE_SHORT_BAND_ROUTE=falseand the attribution is emitted regardless — correct for baseline-first sequencing (attribute the counterfactual now, establish the DiD cohort before the flip). The wiring atrenderOrchestrator.ts:2691-2697could use a one-liner reminder that "attribution runs even when routing is off — this is the baseline-release counterfactual" so a future reader doesn't try to short-circuit the whole block behinddeShortBandRoute.
Questions
-
The 2500 ceiling was calibrated against synthetic sweeps at 7k/20k/40k, and the observed
live-cohort p99 in the 6-hour data is 818. The 83%staticcohort is where the large-DOM tail (if any) would surface — but the fallback's own upward bias (see first concern) makes reading that distribution hard. Any plan to launch a conditional probe on band-candidate renders specifically to widen thelivecohort before "PR B" flips the flag? -
The
HF_DE_SHORT_BAND_ROUTEflip — will it flip at once for all workers, or is a staged rollout mechanism the DiD read depends on? A percentage rollout during the flip window would complicate cohort attribution unless the flag state itself is emitted per render.
What I didn't verify
- The measured
~0.50ms/nodeDE cost and~0.22ms/nodeparallel-screenshot cost cited in the docstring atrenderOrchestrator.ts:2607-2625. Vance's sweep is the source of truth. - The
probeRequiresBrowserconditions cited by the R4 regression test — trusting the test's assertion that a known-duration, media-free, resolved-composition comp gets no probe. - Live behavior in a real Chrome —
page.evaluatetiming overhead may differ from theoretical (document.querySelectorAll("*")on 40k+ nodes measured cost varies wildly between engines). - The dashboard PR / PostHog view that will actually consume
de_short_band,composition_element_count*,observability_init_element_count— not in this diff by design.
— Review by Rames D Jusso
miga-heygen
left a comment
There was a problem hiding this comment.
SSOT
Clean separation. elementCount rides the observability pipeline only (collectSessionInitTelemetry → perf summary → mergeWorkerInitObservability → summarizeInitObservability → telemetry event). The routing gate uses resolveCompositionElementCount/countElementTags — a static HTML counter in a completely separate code path. No field in the diff touches compositionElementCount or deShortBandOpen. The comment explicitly documents this: "measured here it is observational only — capture has already started, so it is far too late to route on." No leakage.
Correctness
Every modification mirrors the existing tweenCount pattern exactly:
collectSessionInitTelemetry:let elementCount = 0; try { ... } catch { elementCount = 0; }— same try/catch-with-fallback shape. ✓readUnsignedIntAfter(line, "elementCount=")insummarizeInitObservability— same parser, samemaxReadingcombinator. ✓mergeWorkerInitObservability:Math.max(elementCount, perf.initElementCount)guarded by!== undefined— same defensive max-across-workers pattern. ✓- Undefined check updated from
initDurationMs === undefined && tweenCount === undefinedto include&& elementCount === undefinedin bothsummarizeInitObservabilityandmergeWorkerInitObservability. Correct — prevents returning a zeroed-out object when no source reported any signal. ✓
Telemetry Chain
Traced end-to-end, no gaps:
| Hop | Field | Source |
|---|---|---|
| 1 | elementCount (local) |
document.querySelectorAll("*").length |
| 2 | session.initTelemetry.elementCount |
recordSessionInitTelemetry |
| 3 | Console INIT line elementCount=<N> |
appendBrowserDiagnostic |
| 4 | CapturePerfSummary.initElementCount |
getCapturePerfSummary |
| 5 | mergeWorkerInitObservability → .elementCount |
max-merged from per-worker perfs |
| 6 | RenderInitObservability.elementCount |
fallback seed + console parse |
| 7 | observabilityInitElementCount |
renderObservabilityTelemetryPayload |
| 8 | observability_init_element_count |
telemetry event property |
Naming transitions follow the same convention as tweenCount → initTweenCount → tweenCount → observabilityInitTweenCount → observability_init_tween_count. Consistent.
Side Effects
page.evaluate(() => document.querySelectorAll("*").length) is a read-only DOM query. Runs after init completes. try/catch ensures failures are swallowed. No mutation, no timing interference. ✓
Tests
- mergeWorkerInitObservability: Existing max-merge updated with
initElementCount. New test: "surfaces an element count even when a worker reported nothing else" — covers the edge case where onlyinitElementCountis present. ✓ - Observability fallback: Structured fallback, max-merge console over fallback, and the stays-undefined case all updated with
elementCount. ✓ - Single-session path (new): "parses elementCount from the console INIT line with no fallback at all" — this IS the 83% path the PR exists for. Good that it has dedicated coverage. ✓
No missing critical edge cases.
Standards
No bare as T. No ! assertions. All new fields optional (?:) at interface boundaries. JSDoc on every new field. ✓
CI
11 failures across regression shards + Windows are all pending re-run (same flakes visible across the DE stack, not caused by this PR). Confirm they clear before merge.
Verdict
Approve. Textbook plumbing extension — elementCount follows the exact same path as tweenCount at every hop, with the same error handling, same merge semantics, same test patterns. The SSOT invariant is maintained by design (measured after routing) and by documentation (explicit comments). The telemetry chain is complete with no gaps. Test coverage hits the critical single-session path and the merge edge cases.
Makes the fleet element-count distribution observable on every render, not just the 17% that get a probe session. Clean follow-up to #2875.
-- Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head review at d74afc7b7dc620036439b398c6bca823fb3911b4, additive to Miga and Rames.
The new field stays strictly observational at packages/engine/src/services/frameCapture.ts:413: collection occurs after routing, then follows the existing structured/console paths through packages/producer/src/services/renderOrchestrator.ts:1347, packages/producer/src/services/render/observability.ts:301, and the CLI event at packages/cli/src/telemetry/events.ts:99. The max-across-workers and single-session/no-fallback cases are both pinned by focused tests.
Non-blocking telemetry-quality caveats from Rames are valid: packages/engine/src/services/frameCapture.ts:423 currently collapses an evaluation failure into measured zero, and the “every render” wording at packages/engine/src/services/frameCapture.ts:420 excludes failures before init telemetry runs. Neither affects routing or render correctness, but dashboards should treat zero cautiously until measurement failure is separately representable.
Current exact-head gates: 38 success, 8 skipped, 7 regression shards still running, 0 failures. Code approved; merge only after the remaining shards finish green.
— Magi
Verdict: APPROVE
Reasoning: The element-count signal is correctly isolated from routing and is propagated end-to-end with matching merge and parser coverage; remaining concerns affect interpretation of observational data, not correctness of this change.
Review findings on #2891. Two of them bite directly on this PR's own purpose — making the fleet element-count distribution readable — so they are fixed rather than noted. countElementTags counted `</` + letter anywhere, including inside inline JS. A compiled comp containing `const h = "</div>"` or a template literal building `</span>` inflated the count once per occurrence. Compiled comps embed large inline scripts, so the bias is systematic, not noise, and it lands entirely on the ~83% of renders with no probe session — precisely the cohort this PR exists to characterize. Script and style bodies are now stripped before matching; losing their own closing tags costs 1-2 counts against a threshold in the thousands. The new elementCount fell back to 0 when its page.evaluate threw, following the tweenCount pattern beside it. For this field that pattern is wrong: evaluate failures concentrate on the huge-DOM compositions the field is meant to observe, and a 0 there is indistinguishable from a legitimately empty comp, so the fleet p50/p99 would absorb both silently. It is now undefined on failure, the INIT console line omits the token entirely rather than emitting a zero, and the parser reports absent — mirroring the live/static provenance split the routing resolver already uses. Also documented: the "every render reaches this path" claim holds only for renders that survive to end of init, so the tail is survivor-biased and should be read as a lower bound; and the two element-count fields now say plainly which is which — composition_element_count gates routing, observability_init_element_count is the observational counterpart — so the follow-up analysis can't query the wrong one. Nits: envInt is integer-only per its name, both live-DOM reads use getElementsByTagName (live collection length, no NodeList materialized on the 40k-node tail), and the attribution block notes that it runs with routing off by design. Fault injection confirms the new tests bite: disabling script stripping fails 4, and the zero-vs-undefined case is pinned separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks both — all seven items addressed in Fixed
Attribution-runs-with-routing-off reminder — added at the Documented rather than fixed (both are inherent)Sampling bias from renders that die before init — correct, and "every render reaches this path" was too strong. It's every render that survives to end of init, and huge-DOM comps are disproportionately the ones that don't. Docstring now says the distribution is survivor-biased and the tail should be read as a lower bound. Two element-count fields with different semantics — real trap for PR B's analysis. Both type docs now state plainly which is which: One clarification@james-russo-rames-d-jusso on the framing note — the symbols you listed ( VerificationFault injection on both substantive fixes: disabling script stripping fails exactly 4 tests, and zero-vs-undefined is pinned separately. 200 producer + 26 engine tests green, engine/producer/CLI all typecheck. |
CodeQL (incomplete multi-character sanitization, code-scanning/803) on the script-stripping regex added in c61a24b — a failing check, and correct: a single replace can reform the pattern it just removed, since `<scr<script>ipt>` leaves a whole `<script>` behind. The security framing does not apply — the stripped string is counted and discarded, never rendered, inserted, or served — but the incompleteness is real for this use: a reformed tag survives into the match pass and perturbs the element count the routing gate reads. Suppressing a gate over a technicality when the fix is four lines is the wrong trade. Now loops to a fixed point. Terminates by construction: each iteration either strictly shortens the string or changes nothing and exits. Regression covers the reform case and an unterminated `<script>` that must not spin; fault injection confirms the reform test fails under the old single pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to #2875, driven by its first six hours of data.
What the baseline showed
Probe coverage is far lower than estimated. The routing gate can only read a live element count when a probe session exists:
livestaticI had projected ">=28%" from a video-presence proxy. It's 17%. So the fail-closed design in #2875 (correct, and staying) leaves 83% of renders measured by a static source scan — which is blind to precisely the shape that motivated the live count: small markup, thousands of script-created nodes.
And the distribution is surprising:
Nothing approaches the 2,500 ceiling — which was calibrated against synthetic comps at 7k / 20k / 40k elements. Either that ceiling is near-irrelevant in practice, or the large-DOM tail is hiding in the 83% we can't see. Both readings change what PR B should do, and neither is decidable from probed renders alone: they're a biased sample, having earned a probe because they carry media or unresolved compositions.
Band attribution says the same thing from the other side — of 53 band-decisive renders, 49 were
unmeasured, 4applied, andskipped_elementshas never fired.What this ships
Measure the live DOM where every render already goes: capture-session init.
collectSessionInitTelemetrygains adocument.querySelectorAll("*")count beside the tween count it already collects, riding the same channel out toobservability_init_element_count.Explicitly observational — capture has already begun, far too late to route on, and it deliberately does not feed the gate. #2875's fail-closed behaviour is untouched. This answers only the distribution question the gate structurally cannot.
Why this channel will actually cover everything
Not assumed — measured. The parallel-worker init fix that shipped in v0.7.83 uses this exact path:
Tests
Merge helper (max across workers, element-only worker, absent stays absent), console-line parsing with and without a structured fallback, and the single-session no-fallback path — the one that covers renders the gate can't measure. Fault injection confirms they bite: deleting the parser line fails exactly 2. 196 producer tests green, engine + CLI typecheck clean.
What it unblocks
With a full distribution across both buckets we can decide PR B on evidence: whether the ceiling needs to exist at all, whether a hidden large-DOM tail is real, and whether conditionally launching a probe for band candidates is worth its cost.
🤖 Generated with Claude Code