Improve GPT auction diagnostics observability - #1121
ChristianPavilonis wants to merge 5 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Solid, well-tested addition: stable Ad #N identity, auction classification, and server auction timings all land behind the existing activation gate, with a normalization boundary and tests for the malformed cases. No correctness, security, or WASM-compatibility problem found; all 14 CI checks pass. The findings below are all non-blocking — the substantive ones are about the SPA timing anchor and label accuracy in a tool whose value is precise facts.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because the change touches test files, spans more than one hunk, or is a design choice rather than a mechanical edit.
Non-blocking
🤔 thinking
- SPA
auctionDispatchedMsis always0by construction — see inline atcrates/trusted-server-core/src/publisher.rs:6748 Navigation T0overstates the anchor — see inline atcrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:246- The store infers
ssatwhen auction facts are absent or malformed — see inline atcrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/store.ts:1129 1x1suppression conflates "GPT reported 1x1" with "GPT reported nothing" — see inline atcrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/presentation_helpers.ts:9
♻️ refactor
set_auction_diagnosticsrebuilds the script cell and drops a debug prefix — see inline atcrates/trusted-server-core/src/publisher.rs:3209(suggestion)- Placement wire string has two sources of truth — see inline at
crates/trusted-server-core/src/publisher.rs:6767
⛏ nitpick
- Duplicated 5-arg / 6-arg recorder call — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1113 - Browser spec traded away its only non-
1x1fill-size case — see inline atcrates/trusted-server-integration-tests/browser/tests/nextjs/gpt-diagnostics.spec.ts:223
👍 praise
- Stable
Ad #Non re-entry, and fail-closed dispatch gating — see inline atcrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/store.ts:1201
Cross-cutting / body-level findings
- 📝 Stacked base — this targets
spec/auction-timeline-offsets(stacked on #1076 → #1074). Retarget tomainafter those land, as the description says. Nothing in the diff depends on that ordering beyond the base itself. - 📝 Coverage of the server write path is complete —
write_bids_to_statehas exactly two production call sites (collect_non_html_auction,collect_stream_auction) and both are now paired withset_auction_diagnostics, so there is no document path that commits bids without the timing facts. Verified by grep rather than assumed.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
gh pr checks --required returned no names for this base, so none of the above are annotated as branch-protection-required; all of them are gates CLAUDE.md treats as PR gates, and all pass.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Follow-up to the detailed review above, correcting its verdict: none of its findings are blocking (no 🔧 wrench, no ❓ question), and all 14 CI checks pass, so this should have been submitted as an approval rather than a comment.
Every inline comment there stands as written and remains worth reading — 4 🤔 thinking, 2 ♻️ refactor (one as a one-click suggestion), 2 ⛏ nitpick, 1 👍 praise — but each is a merge-can-proceed observation. The two most substantive, if you want to pick any of them up here rather than in a follow-up:
- SPA
auctionDispatchedMsis a structural0(the clock is read on the line after it starts), which also makesauctionWaitMsa duplicate ofauctionResolvedMs—crates/trusted-server-core/src/publisher.rs:6748. Navigation T0in the panel is the edge request-receipt offset, not the browser'snavigationStart—crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:246.
Approved on ffc0438dd5f872e68c3ccad36d5c35bde47101d2.
|
@ChristianPavilonis please assign issue to this PR |
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Solid, well-tested expansion of the GPT diagnostics console: stable Ad #N identity across badge/panel/export, auction classification, winner and bucketed-price facts, and server auction timings. The privacy boundary is careful — bounded string lengths, a validated price-bucket shape, and timing normalization at the diagnostics boundary.
Two blocking issues: the SPA timing origin can be derived wrongly after a failed or superseded navigation, labelling navigation-T0 offsets as SPA-auction offsets; and the browser spec loses its only normal-fill-size coverage.
None of the inline comments below carry a one-click GitHub
suggestion. Every fix either lands outside the diff hunks or spans a second file, so each is described in prose with the proposed code.
Blocking
🔧 wrench
- Stale navigation-T0 timings relabelled as SPA-auction timings — see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1467 - Browser spec loses all normal-fill-size coverage — see inline at
crates/trusted-server-integration-tests/browser/tests/nextjs/gpt-diagnostics.spec.ts:223
Non-blocking
🤔 thinking
auctionDispatchedMsis structurally always0on the page-bids path — see inline atcrates/trusted-server-core/src/publisher.rs:6748- Page-bids hardcodes
auctionWaitPlacementas a bare string literal — see inline atcrates/trusted-server-core/src/publisher.rs:6766 browser_session_activedrops more eligibility guards than it needs to — see inline atcrates/trusted-server-core/src/integrations/gpt_diagnostics.rs:296
♻️ refactor
- Redundant
if/elsearoundrecordTrustedServerOpportunity— see inline atcrates/trusted-server-js/lib/src/integrations/gpt/index.ts:1115-1132
📝 note
- Dead timing-origin fallback in the overlay — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:239
Cross-cutting / body-level findings
- 📝 Verified during review, no action needed — the following were checked against a scratch worktree at this head and are correct: slot-number reuse across eviction produces no
slotOrder/slotActivityOrderduplicates at theMAX_DIAGNOSTIC_SLOTSboundary;set_auction_diagnosticscannot clobber the auction debug prefix (prepend_auction_debug_commentruns after it incollect_stream_auction, andcollect_non_html_auctionhas no prepend); stripping the console cookie beforehandle_request_cookiesis correctly ordered for the stated privacy goal; the addedprepare_requestcall inhandle_page_bidsis genuinely idempotent via the extension cache, and all four adapters already call it pre-routing; the normalization boundary correctly rejects-1.00,1e3,1.,.5forpriceBucketand negatives /NaN/> u32::MAXfor timings; andelapsed_millissaturation (~49.7 days) is not a practical edge concern.
CI Status
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS
- format-typescript: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
Branch protection reports no required checks on this branch, so none of the above are marked (required).
a758e16 to
563670d
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Second pass, against 563670df9 (the branch was rebased and Address GPT diagnostics review feedback added since the last review at ffc0438dd).
Five of the seven earlier findings are fixed, two were declined with sound reasoning, and I verified each outcome against the new head rather than taking the replies at face value. The 1×1 browser coverage came back stronger than what I proposed, and dropping ?? 'ssat' from the store so malformed direct facts fail closed was a good unprompted improvement.
Two new blocking findings, both consequences of the timing fix rather than pre-existing issues: the guide now describes a clock anchor the code does not use, and two of the four adapters silently fall back to a different anchor than the other two.
Verification of the previous round
| Finding | Outcome |
|---|---|
| 🔧 Stale navigation-T0 timings relabelled as SPA-auction timings | Fixed, re-verified. The original repro now passes: after a failed page-bids fetch ts.auctionDiagnostics is undefined, and the superseded-navigation path clears too. spa_hook.test.ts pins both. |
🤔 auctionDispatchedMs structurally always 0 |
Partly fixed — now a real offset on Fastly and Axum, still structurally zero on Cloudflare and Spin. See finding B. |
🤔 Hardcoded auctionWaitPlacement wire literal |
Fixed. Shared auction_wait_placement_wire mapper, and page-bids routes AuctionWaitPlacement::PreHeader through it. |
♻️ Redundant if/else around the recorder call |
Fixed as proposed; the arity-sensitive assertions now pin { auctionType: 'ssat' } rather than a bare trailing undefined. |
| 🔧 Browser spec lost all normal-fill-size coverage | Fixed, and better than proposed. 300×250 is exercised and exported again, a separate 1×1 cycle proves the raw fact survives to snapshot/export while staying out of the rendered UI, and captureClosedShadowRoots is a tidy way to read the closed root. |
| 📝 Dead timing-origin fallback in the overlay | Declined — reasonable, it is harmless. |
🤔 browser_session_active drops prefetch and bot guards |
Declined, and the rebuttal checks out. publisher.rs:6863 gates the auction on !is_bot && !is_prefetch, so the diagnostics block is unreachable for a bot or prefetch. Withdrawing it. |
Local verification at this head: JS suite 899 passed / 0 failed, no type errors; the previous round's repro re-run against the new code; adapter RequestTimings wiring grepped across all four crates.
Blocking
🔧 wrench
- Guide contradicts the code on the SPA clock anchor — see inline at
docs/guide/integrations/gpt-diagnostics.md:156 - Cloudflare and Spin silently fall back to a handler-entry clock — see inline at
crates/trusted-server-core/src/publisher.rs:6689
CI Status
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS
- format-typescript: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
Branch protection reports no required checks on this branch, so none are marked (required).
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Third pass, against f951955f5. The two blocking findings from the previous round are both genuinely fixed, and I verified each against this head rather than from the replies: the guide and panel labels now describe the adapter server-request clock that the code actually uses, Cloudflare and Spin attach the collector, and handle_page_bids fails closed when an adapter omits it. The pre-aged-collector test closes the coverage gap I raised, and the timing design spec was updated unprompted so it does not go stale.
What remains is the shape of the fix rather than its correctness. RequestTimingMiddleware landed as two byte-identical copies in two adapter crates, which is the fourth instance of a duplication pattern this repo has been carrying, and the new middleware plus the collector it installs are framework-level concerns that belong in EdgeZero rather than in Trusted Server's adapters.
Verification performed at this head
Local runs: cargo test-cloudflare (22 passed), cargo test-spin (44 + 37 passed), cargo test-axum (17 + 1 + 25 passed), core page_bids filter (25 passed, including the new fail-closed test), page_bids_response_includes_auction_id_only_for_winning_bids (passed, confirming auctionDispatchedMs > 0 under the extension path), and the full JS suite (899 passed, no type errors). The first round's stale-timing repro still comes back clear, and both timing origins map to the corrected labels.
git diff 563670df9 f951955f5 is exactly the nine files addressing the previous round, with no incidental drift, so the surfaces cleared in earlier passes are unchanged.
Things that looked like problems and are not: the hardcoded "/health" matches the existing literal in the Fastly and Axum paths rather than introducing it; Spin's second router at app.rs:441 is the degraded-state 503 fallback where no auction runs; and the 2 ms thread::sleep in the async test has ample margin for a > 0 millisecond assertion with nothing else pending on that runtime.
Blocking
🔧 wrench
- Move the request-timing middleware into EdgeZero instead of duplicating it per adapter — see inline at
crates/trusted-server-adapter-cloudflare/src/middleware.rs:73
Non-blocking
🌱 seedling
- The fail-closed timing invariant is enforced on only one of the two handlers that depend on it — see inline at
crates/trusted-server-core/src/publisher.rs:6689
Cross-cutting / body-level findings
-
📌 The other three adapter middlewares carry the same duplication —
SanitizeRequestMiddleware,FinalizeResponseMiddleware, andAuthMiddlewareare each duplicated acrosstrusted-server-adapter-cloudflare/src/middleware.rsandtrusted-server-adapter-spin/src/middleware.rsin the same way, and predate this PR. They are not this PR's to fix, but they are the reason the new middleware should not become a fourth instance: whatever mechanism carriesRequestTimingMiddlewareinto shared code is the one that should eventually carry these too. Worth a follow-up issue so the pattern stops growing. -
📝 Where the generic/domain boundary falls, for whoever picks up the extraction — I mapped
request_timing.rsagainstedgezero-coreto check the move is actually possible. Generic and movable:new,record,span,mark_headers_ready,mark_request_elapsed,set_resp_bytes,server_timing_value,snapshot, andPhaseSpan. Domain-specific and staying in Trusted Server:Phase's eight variants (EcKv,AuctionWait,Origin,TemplateCacheLookupand the rest),AuctionWaitPlacement,record_auction_wait,set_auction_id, and the threemark_auction_*marks.edgezero-corealready carrieshttp,web-time, andasync-trait, souuid(used only byset_auction_id, which stays behind anyway) is the one dependency question, and it resolves itself once the auction marks stay on this side.
CI Status
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS
- format-typescript: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
Branch protection reports no required checks on this branch, so none are marked (required).
| #[async_trait(?Send)] | ||
| impl Middleware for RequestTimingMiddleware { | ||
| async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result<Response, EdgeError> { | ||
| if ctx.request().uri().path() != "/health" |
There was a problem hiding this comment.
🔧 wrench — This middleware should live in EdgeZero, not as a per-adapter copy.
The implementation landed twice, byte for byte. Diffing the RequestTimingMiddleware block in this file against trusted-server-adapter-spin/src/middleware.rs returns no differences at all, tests included — same struct, same handle, same /health skip, same request_timing_middleware_attaches_a_collector_except_for_health loop over [("/test", true), ("/health", false)].
Nothing in it is Trusted Server's concern. "Attach a per-request clock at router entry unless the path is the liveness probe" is a property of the edge framework, and EdgeZero already accepts middleware of exactly this kind: edgezero-core/src/middleware.rs:74 ships a concrete RequestLogger alongside the Middleware trait, so this is not a category EdgeZero refuses. EdgeZero currently has no timing concept at all, which is precisely why each adapter is now carrying its own copy.
I checked that the move is actually possible rather than just desirable, because the collector has to move with the middleware — RequestTimings lives in trusted-server-core, and dependencies run Trusted Server → EdgeZero, never back. The split is cleaner than it looks:
- Generic, moves to EdgeZero:
new,record,span,mark_headers_ready,mark_request_elapsed,set_resp_bytes,server_timing_value,snapshot,PhaseSpan, plus this middleware. - Domain-specific, stays here:
Phase's eight variants (EcKv,AuctionWait,Origin,TemplateCacheLookup, …),AuctionWaitPlacement,record_auction_wait,set_auction_id, and the threemark_auction_*marks. A generic edge framework should not know about ad auctions or EC key-value stores, so the phase taxonomy needs to become a parameter (generic over a phase enum, or an index/string-keyed slot API) rather than moving as-is.
The dependency side is already in place: edgezero-core carries http, web-time, and async-trait today. uuid is the only addition the collector would suggest, and it is used solely by set_auction_id, which stays on this side — so it does not need to follow.
The one genuine coupling to unpick is request_timing.rs:19, use crate::cache_policy::cache_control_headers_are_private_or_no_store, called once at line 455 to decide whether Server-Timing may be appended. Either take a predicate at that boundary, or leave append_server_timing_if_private in Trusted Server and move only the collector.
Two practical notes on sequencing, since EdgeZero is a separate repo pinned at tag = "v0.0.7" in the root Cargo.toml. First, this needs an EdgeZero change, a tag, and a bump here, coordinated across four adapters — it is a migration, not a cleanup, and doing it as a follow-up PR stacked after this one is entirely reasonable. Second, if you want to unblock merging sooner without shipping the duplication, there is a smaller intermediate step available today: trusted-server-core already depends on edgezero-core for the Middleware trait and already has async-trait, and both adapters already depend on trusted-server-core — so hosting one copy in trusted-server-core right now is a pure move, no redesign, and it reduces the eventual EdgeZero extraction to relocating a single item instead of reconciling two copies.
Apply manually — this spans two adapter crates plus request_timing.rs, and the EdgeZero half lives in another repository, so it cannot be expressed as a suggestion.
| let has_request_timings = request_timings.is_some(); | ||
| let timings = request_timings.unwrap_or_default(); | ||
|
|
||
| // Adapter fallbacks prepare this before routing. Keep this idempotent call as |
There was a problem hiding this comment.
🌱 seedling — The fail-closed rule now guards this handler but not the other one that depends on it.
has_request_timings correctly stops handle_page_bids from publishing handler-local offsets when an adapter omits the collector. The initial-document path has no equivalent guard: handle_publisher_request at publisher.rs:4266-4270 still does
let timings = req
.extensions()
.get::<RequestTimings>()
.cloned()
.unwrap_or_default();and set_auction_diagnostics gates only on diagnostics_active and on whether a dispatch mark exists, never on where the clock came from. So the same defect this commit fixes for SPA offsets could reappear for initial-document offsets, reported under the Initial document request T0 label.
Not reachable today, and I confirmed that rather than assuming it: all four adapters now attach the collector in production code (Fastly 3 insert sites, Axum 1, Cloudflare 1, Spin 1). So this is about where the invariant is written down, not a live bug — worth a follow-up issue rather than a change in this PR.
The reason it is worth recording: the invariant is now enforced in one of the two places that rely on it, and the unguarded side is the one with no test covering a missing collector. A future adapter, or a router path that bypasses the new middleware, would reintroduce it on the navigation side only. Applying the same has_request_timings gate in handle_publisher_request — or pushing the check down into set_auction_diagnostics so both callers inherit it — would make the rule hold structurally instead of per-call-site.
# Conflicts: # crates/trusted-server-core/src/publisher.rs # crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts # crates/trusted-server-js/lib/test/integrations/gpt_diagnostics/overlay.test.ts # docs/guide/integrations/gpt-diagnostics.md
This branch has not been deployed
3 participants
Summary
Ad #Nin the TS Console panel and export.1×1and size terminology.This is stacked on #1076, which is stacked on #1074. Retarget this PR to
mainafter those dependencies merge.EdgeZero dependency and release gate
The latest review feedback is addressed by stackpop/edgezero#389 and TS commit
2791ceb46908e6af04ccaae0c10b29d35336a9f4.All six EdgeZero dependencies temporarily pin exact candidate commit
9c03cc59300363ae5339fc970dded08ede563e98for integration testing. The lockfile resolves one EdgeZero core identity, with no local path overrides.Do not merge this PR into main until a proper EdgeZero release is published. Replace all six temporary revision pins with that release tag, regenerate/review Cargo.lock, and rerun tag-backed verification first. Neither repository has been merged or released by this update.
Shared timing migration
Verification of the migration
Independent joint reviews found no remaining issues. All 14 GitHub checks passed at
2791ceb46908e6af04ccaae0c10b29d35336a9f4, including full integration and browser jobs. Final local validation was tied to the exact committed source tree:Browser setup failures from regenerated Prebid artifacts and a temporary cache override were corrected without source changes or weakened assertions; the complete unchanged suites then passed. Runtime evidence uses local Viceroy/workerd/headless-browser harnesses, not deployed providers. TS Spin has native and WASM compile/lint evidence, not a deployed runtime test.
Changes
crates/trusted-server-core/src/publisher.rscrates/trusted-server-js/lib/src/integrations/gpt/crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/crates/trusted-server-js/lib/test/and browser integration testsdocs/guide/integrations/gpt-diagnostics.mdCloses
Closes #1081
Test plan
cargo test-fastly && cargo test-axumcargo test-cloudflarecargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflarecargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(899 passed, no type errors)npm run lint && npm run buildcd crates/trusted-server-js/lib && npm run formatcd docs && npm run format && npm run lint && npm run buildcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1Checklist
unwrap()added in production codeprintln!/eprintln!added