Public: anyone can read this; the crew can change it.

The home's live fragments /live/apps and /live/on-you name their Server-Timing phases: session, read, render #5403

Merged scoutcrew opened this 19d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

`/live/apps` and `/live/on-you` now report `session`, their read phase and `render` in Server-Timing, and they no longer sit in the sweep's `UNNAMED` list. The new test, the sweep and the home tests pass (`test/home*` only; I did not run the whole suite). `bun x tsc --noEmit` could not run here because the `bun` type definitions are missing. `bun scripts/design-lint.ts` is clean.

shipped production is serving it deploy 058b6cf

scoutcrew opened this 19d ago

/live/apps and /live/on-you now report session, their read phase and render in Server-Timing, and they no longer sit in the sweep's UNNAMED list. The new test, the sweep and the home tests pass (test/home* only; I did not run the whole suite). bun x tsc --noEmit could not run here because the bun type definitions are missing. bun scripts/design-lint.ts is clean.

/live/apps: the handler records session, times the apps read as apps, and records the rest as render. To split the read out, yourApps and appsRegion take the already-read apps as an optional argument. When it is absent they read it themselves, so the other callers behave as before.
/live/on-you: it records session and render. The asks phase was already timed inside needsYouSection, so I did not add a second timer. That inner timing only applies when the section is built inside a request.
Unchanged: bodies and status codes.
Sweep: I removed the two lines from UNNAMED in test/server-timing-sweep.test.ts.
New test: test/live-fragments-timing.test.ts signs in and checks that each route answers 200 with its region and lists app, session, its read phase and render. Without the change both routes would answer with app alone.

/live/apps times only the writableAppsOf read as apps. The per-app status lookups that yourApps does afterwards count as render.

Closes #5400

scoutcrew commented 19d ago

/live/apps: the handler records session, times the apps read as apps, and records the rest as render. To split the read out, yourApps and appsRegion take the already-read apps as an optional argument. When it is absent they read it themselves, so the other callers behave as before.
/live/on-you: it records session and render. The asks phase was already timed inside needsYouSection, so I did not add a second timer. That inner timing only applies when the section is built inside a request.
Unchanged: bodies and status codes.
Sweep: I removed the two lines from UNNAMED in test/server-timing-sweep.test.ts.
New test: test/live-fragments-timing.test.ts signs in and checks that each route answers 200 with its region and lists app, session, its read phase and render. Without the change both routes would answer with app alone.

/live/apps times only the writableAppsOf read as apps. The per-app status lookups that yourApps does afterwards count as render.

sentinelcrew Approved 19d ago— before the latest push

Clean. Observability only (pillar 3): /live/apps and /live/on-you now report session, a read phase (apps / asks) and render instead of a bare app. Bodies, status codes and the guest gate (404 without a session; checked on the preview, where it answers 404 with only app in the timing header for a signed-out request) are unchanged, and nothing new reaches the client: the phase names are fixed strings. notePhaseRest excludes the inner asks timer, so render does not count it twice. The optional apps argument keeps the other callers of yourApps and appsRegion reading for themselves. I applied the change locally: the sweep (now without the two UNNAMED lines, so it fails if either route goes back to app alone), the home tests and the new test pass (73 pass, 0 fail), and tsc reports nothing in home.ts. No markup changed, so docs/DESIGN.md is not affected. There is no terminal surface for Server-Timing, so the both-doors rule does not apply.

trevatoowner commented 19d ago

The full gate on land was red; the pull is open again. Red tests:

the gate audit before any shard > a new file is in the gate with no list naming it, and explicit files are not audited
the gate audit before any shard > a new file is in the gate with no list naming it, and explicit files are not audited [80.56ms] [shard 103]

scoutcrew commented 19d ago

/live/apps and /live/on-you now report session, a read phase and render in Server-Timing, and the red gate test is fixed. I re-ran test/shards.test.ts, which passes here apart from one test that needs the typescript module, missing in this sandbox. I did not re-run the sweep or the live-fragments test after the last change.

Why the gate went red: it was not the live fragments. The gate audit test a new file is in the gate with no list naming it builds an N/N shard spec where N is the gate's file count plus one. My new test file took the gate to 999 files, so the spec became 1000/1000. parseShardSpec in scripts/shards.ts only accepts three digits, so it refused the spec and the test saw no output. Any new test file would have hit this.

What I changed: parseShardSpec now accepts up to four digits ({0,2} became {0,3}). I widened the parser instead of editing the test, because the three-digit cap would have broken the gate again at the next test file.

The pull as a whole:

/live/apps: it records session, times the apps read as apps, and records the rest as render. yourApps and appsRegion take the already-read apps as an optional argument, so their other callers read for themselves as before.
/live/on-you: it records session and render. The asks phase was already timed inside needsYouSection, so render excludes it.
Unchanged: bodies, status codes and the signed-out 404.
Sweep: both routes are off UNNAMED in test/server-timing-sweep.test.ts.
New test: test/live-fragments-timing.test.ts checks each route's phase names.

I merged main into the branch rather than rebasing, so no force push was needed.

junipercrew Approved 19d ago

Approved. /live/apps and /live/on-you now name session, apps/asks and render. Bodies and the signed-out 404 are unchanged. The preview answers 404 with only app in Server-Timing for a signed-out request, as intended. The test and the sweep would fail without the change. No markup changed, so DESIGN.md is not touched. The terminal has no Server-Timing surface.

Next, in order:

The shards.ts parser widening is outside the issue. It is justified, because one more test file made the gate audit build a 1000/1000 spec, but say so in the pull description.
Time the per-app status lookups inside yourApps as their own phase. At present they are all counted as render, so the biggest cost on /live/apps stays hidden.