Repository navigation
Consolidate request timing and GPT diagnostics - #1261
ChristianPavilonis wants to merge 59 commits into
Conversation
* Add request phase timing design spec (Server-Timing subtimings + access telemetry) * Address review round 1: freeze point, template-cache naming, snapshot semantics, KV scope, geo carry, route template, sink confirmation, sampling and query model, config rollback * Address review round 2: auction-wait placement modes, conservative private-only header emission, non-null sorting key with service identity, coarse publisher route template, telemetry snapshot and outage behavior, tinybird flag decoupling, adapter phase semantics * Add request phase timing implementation plan * Address engineer review: KV timing decorator, try_lock sampling, route metadata extension, adapter-derived env, typed template-cache state, adapter-owned emission context, per-mode delivery semantics, Axum outer wrapper
…ite-back in middleware Three final-review fixes for access telemetry correctness: - Normalize the HTTP method to an allowlist (GET/HEAD/POST/PUT/DELETE/ PATCH/OPTIONS, else "other") inside access_event_row, so a client- controlled extension-method token can never inflate the LowCardinality method column, regardless of which adapter builds the row. - Guard emit_access_telemetry_after_send against snapshots carrying a degraded sample_rate of 0.0 (captured on the app-state-build-failure fallback path), which could otherwise be sampled in by freshly reloaded settings and corrupt the sum(1.0/sample_rate) volume estimator. - Mirror the geo lookup write-back from apply_entry_point_finalize_headers into FinalizeResponseMiddleware::handle, so a middleware-finalized response that resolved geo via fallback carries the resolved GeoLookupState for the access-telemetry snapshot instead of showing country "unknown".
std::time::Instant::now() panics on wasm32-unknown-unknown, so every publisher request on the Cloudflare adapter trapped when the timing collector was constructed, and the two auction-wait sites would trap once an auction dispatched. web_time re-exports std's Instant on every other target, so Fastly, Axum, and Spin behavior is unchanged. The publisher.rs sites are qualified locally because that module's std Instant import still serves the pre-existing template-cache sites, which are out of scope here.
The character allowlist alone does not bound identity: [a-z0-9_-] is exactly the alphabet UUIDs, hex ids, reset tokens, and article slugs are built from, and truncating to 32 characters still leaves a globally unique prefix. A first segment now rejects whole to /other/* when it exceeds 32 characters or carries more than 7 ASCII digits, alongside the existing charset rejection. Year archives and hyphenated section names still pass. Extends the adversarial tests to the publisher-fallback path with UUID, hex-id, token, and slug shapes, and fixes the stale event_date reference in the row-builder doc.
- Gate building the access snapshot on tinybird.enabled and access_enabled, threaded through SendContext: a disabled deployment (the default) no longer pays env reads and String allocations on the pre-send path. DeliveryOutcome.snapshot becomes Option and the emitter treats None as nothing to send. - Classify asset-fallback responses as route_class asset with the operator-configured route prefix as the template, instead of landing in the other/unknown bucket alongside 404s. - Pin Phase::index() to PHASE_COUNT with a uniqueness-and-bounds test so a future variant fails the suite instead of panicking at runtime. - Drop the tautological sampled-out emission test; the 0.0-rate behavior is covered by sampled_in_boundary_rates_are_unconditional. - Clarify that the local dev config env var name genuinely triples trusted_server_config (prefix, store, key) rather than reading as a find/replace mistake.
Adds section 18 to the request phase timing spec: three first-call-wins T0 offsets (auction dispatched, resolved, committed) on RequestTimings, emitted as additive nullable columns on access_logs_raw with auction_id as the join key to the per-bidder auction dataset. Answers the overlap-proof questions the two existing clocks cannot: when the auction started relative to request entry, when the final bid landed, and when targeting was committed toward GAM.
Implements spec section 18: three first-call-wins marks on RequestTimings (dispatched at the DispatchAuctionOutcome::Dispatched arm, resolved after collect at both sites, committed after write_bids_to_state at both sites), carried through TimingSnapshot into four additive access_logs_raw columns: auction_dispatched_ms, auction_resolved_ms, auction_committed_ms, and auction_id as the join key to the per-bidder auction dataset. Null offsets mean no auction ran; a failed dispatch records nothing. FORWARD_QUERY fills the new columns with typed defaults for pre-existing rows. No header emission, no config surface, no adapter changes: the values ride the existing snapshot and the tinybird.access_enabled gate.
The Cloudflare integration harness writes wrangler.integration.generated.toml at test time; it was swept into the previous commit by accident. Ignore it so local CI=1 runs cannot commit it again.
The first path segment is only a section name when the path has depth: under a /%postname%/ permalink structure every article is a single-segment path, so keeping those segments verbatim put full article slugs into the 30-day dataset, against spec section 9. Depth is now required for a named template; single-segment paths, root landing pages included, bucket to /other/*. Route slicing keeps route_class and multi-segment templates like /news/*.
The bucket-quantized sampler truncated rates below one in a million to a zero threshold (silently emitting nothing) and quantized other low rates downward while rows still carried the configured rate, biasing the sum(1.0 / sample_rate) volume estimator. Its no-rand premise was also wrong: rand::thread_rng() is WASI-backed on this target and the EC generation path already relies on it. The sampler is now a direct uniform-roll comparison, and the roll gates on the rate stored in the snapshot itself, so emission probability and the row's sample_rate column cannot diverge; the divergence guard and its tests are removed. Also per review: the settings-reload fallback in the post-send path could never emit (no snapshot exists when settings were absent) and is removed; the dead_code allow on DeliveryOutcome narrows to the one collected-but-unemitted field; and the post-send ordering test is narrowed to the leg it actually proves, that request_elapsed is stamped when send returns.
On origin failure with a dispatched auction, the origin span guard stayed alive through the emit_abandoned_auction await, so ts-origin and origin_ms absorbed Tinybird emission time. The span now closes when the send resolves, before either branch, with an error-path regression test.
- Pin HEADER_PHASES against Phase::header_name() in the phase-index test, closing the second hand-synced list. - Add RouteClass::Asset to the snake_case rendering test; rename the lowercasing test to say what it does. - Give the Axum adapter a named, fully configured construction path (TrustedServerApp::dev_server_service) so server_timing_enabled is never silently discarded; the tuple API is private now. - Document that Server-Timing is client-visible when enabled, in the configuration guide's observability section. - Replace stale event_date references in the spec, plan, and dashboard guidance with the toDate(event_ts) sorting-key expression, and state the single-segment rejection rule in spec section 9.
…ming # Conflicts: # crates/trusted-server-adapter-fastly/src/app.rs # crates/trusted-server-adapter-fastly/src/main.rs # crates/trusted-server-adapter-fastly/src/middleware.rs # crates/trusted-server-core/src/publisher.rs # crates/trusted-server-core/src/settings.rs # docs/guide/configuration.md # trusted-server.example.toml
The access emitter carried the configured body limit without enforcing it, allowing oversized rows to bypass the intended transport safeguard.
Co-authored-by: prk-Jr <49094961+prk-Jr@users.noreply.github.com>
Keep the diagnostics evidence wording and request controls while adopting feature/ts-console-improvements' shared server-request timing origins. Align overlay assertions and the label dictionary with those origins.
Resolve five conflicting files plus the semantic conflicts the textual merge could not mark. Adopt main's removal of the legacy consent KV path (#903): the Fastly adapter no longer opens a consent store per request, so the branch's timed wrapper around it, its route call sites, and the test asserting consent-store reads land in ts-kv are dropped. TimedKvStore keeps both trait impls; its module doc and the design spec no longer describe a consent-store read. Forward the EcKvStore::list_keys_with_prefix method main added for EID write-conflict reduction (#1157) through TimedKvStore with the same ts-kv span. Give main's new admin cache-purge named route (#1150) a RouteClass::Other classification so access telemetry carries a template for it. Union the remaining hunks: the Axum adapter keeps dev_server_service and routes_with_server_timing_flag beside main's routes_with_settings_and_services; the Fastly route tests keep the RouteMetadata assertions beside main's EID sync-source dispatch tests, with the short-circuit test renamed to main's name; the configuration guide's section table takes main's layout with the observability row and the access-telemetry wording restored. Update tests main added that build branch-extended structs: AuctionCollectDeps initializers gain timings and placement, the Settings debug-redaction fixture gains auction_enabled, and the EC finalize freeze-point test marks its context as a navigation now that returning- user EID persistence is gated on the request source.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This consolidates the request-timing, access-telemetry and GPT-diagnostics stack, and the code is in good shape against the EdgeZero combination it was developed on: with 683202c + ab444946 patched in locally, clippy-fastly, test-fastly (262 adapter + 2,950 core), test-fastly-reuse, all five non-Fastly clippy gates, the Axum/Cloudflare/Spin suites, 1,199 vitest tests and the docs build all pass. What blocks it: the committed EdgeZero pin cannot build Fastly, the Tinybird forward query targets a schema main never shipped, one dispatch-accounting bug fabricates auction offsets, a join-key contract that differs by route, mutation-proven test gaps on the new timeline wiring, and operator docs that still say access telemetry is rejected.
12 of the inline comments carry a one-click GitHub
suggestion, each verified in isolation (rustfmt, patched clippy with-D warnings, the affected test suites, prettier/eslint/vitest/build-all.mjsfor JS, prettier for docs). The rest describe the fix in prose because it spans files or lines outside the diff.
Blocking
🔧 wrench
- EdgeZero pin cannot build the Fastly adapter: see inline at
Cargo.toml:59 FORWARD_QUERYselects columns main's liveaccess_logs_rawlacks: see inline attinybird/datasources/access_logs_raw.datasource:41- A provider that sends nothing is counted as launched: see inline at
crates/trusted-server-core/src/auction/orchestrator.rs:1794 - Disabled-auction join key differs by route: see inline at
crates/trusted-server-core/src/publisher.rs:5106 - Navigation timeline and stream placements untested (mutation-proven): see inline at
crates/trusted-server-core/src/publisher.rs:5149 - JS auction-classification outputs survive mutation: see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:84 - Fastly freeze-point ordering and geo write-back untested: see inline at
crates/trusted-server-adapter-fastly/src/main.rs:1003 - CI is red and operator docs still say access telemetry is rejected: see below
Non-blocking
🤔 thinking / ♻️ refactor / 📝 note / ⛏ nitpick
- Route identity lost on fallback short-circuits: see inline at
crates/trusted-server-adapter-fastly/src/app.rs:837 private="field"counts as conclusively private: see inline atcrates/trusted-server-core/src/request_timing.rs:345- Post-send access emission blocks the callback: see inline at
crates/trusted-server-adapter-fastly/src/main.rs:826 ts-appbuildon reused sandboxes: see inline atcrates/trusted-server-adapter-fastly/src/main.rs:382ts-originon the early-send path: see inline atcrates/trusted-server-core/src/publisher.rs:5360R - Dis not auction duration on streamed pages: see inline atdocs/superpowers/specs/2026-08-24-request-phase-timing-design.md:729- Diagnostics code in the always-served Prebid shim: see inline at
crates/trusted-server-js/lib/test/prebid-artifact-integration.test.mjs:328 - First-impression fallback drops auction facts: see inline at
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1408 ts_versionis the Fastly service version: see inline atcrates/trusted-server-core/src/access_telemetry.rs:203- Unused delivery-result groundwork: see inline at
crates/trusted-server-adapter-fastly/src/main.rs:860 - Spec and plans describe a different design: see inline at
docs/superpowers/specs/2026-08-24-request-phase-timing-design.md:3 [tinybird]env overrides are no-ops against the example: see inline atdocs/guide/configuration.md:3076- Cloudflare/Spin ignore both flags: see inline at
docs/guide/configuration.md:2988 - Stale
run_auctioncomment: see inline atcrates/trusted-server-core/src/publisher.rs:7342 TimingServicecalls an un-readied clone: see inline atcrates/trusted-server-adapter-axum/src/timing.rs:79- Disabled path still parses cache headers: see inline at
crates/trusted-server-core/src/request_timing.rs:342 - Test
unwrap()and missing assertions: see inline atcrates/trusted-server-core/src/auction/endpoints.rs:1411 expectmessage form: see inline atcrates/trusted-server-core/src/publisher.rs:6196--punctuation thread still open in code: see inline atcrates/trusted-server-adapter-axum/src/timing.rs:13.gitignorenames the wrong template: see inline at.gitignore:67- Badge selection always opens history: see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:523 - Dictionary link without
noreferrer: see inline atcrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:724
Cross-cutting / body-level findings
- 🔧 CI is red, all from the EdgeZero pin.
cargo fmt(required; it fails in itsclippy-fastlystep, so every later clippy step for axum, cloudflare native/wasm, spin native/wasm, cli and codegen was skipped),cargo test(required),cargo test (axum native)(its Axum build and tests pass; only "Verify Fastly WASM release build" fails) andprepare integration artifactsall fail withcannot find 'lifecycle' in 'edgezero_adapter_fastly', sointegration tests,integration tests (Fastly EC lifecycle)andbrowser integration testswere skipped. The Playwrightgpt-diagnostics.spec.tshas therefore never run at this head. - 🔧 Operator docs still describe access telemetry as rejected/reserved, and nothing describes the new 30-day dataset.
docs/guide/telemetry.md:44-47saysaccess_enabled = true"is rejected because no access-log emitter is wired" and thataccess_token_secretis deprecated and normalized away;tinybird/README.md:13-14saysaccess_logs_rawis reserved and not emitted, and:20-23only covers the auction APPEND token;docs/guide/configuration.md:1307-1308("access-log emission is not wired") sits right above the rewritten table, and:77-79/:111still call the token deprecated. This PR validates and requires the token and emits rows from Fastly. telemetry.md has a privacy boundary section for auction telemetry only; the access row needs the same: its columns (route class and bounded template, country, pop, service id, publisher domain, env, normalized method, status, phase timings, response bytes), thatauction_idjoins it toauction_events_raw(device class, browser family), uniform per-response sampling withsample_ratestored per row, Fastly-only emission, and the post-send ingest wait. Also add thets_access_ingesttoken step to the README. - 📝 PR description and history. The four source PRs were closed on 2026-10-09 ("Superseded by #1261"), so "All four originals remain open" is stale, and their 31 unresolved threads (and CHANGES_REQUESTED reviews) now sit on closed PRs with the table as the only tracker; a reply on each linking its disposition would close the loop. "Keep disabled auctions unattempted with null access UUID" holds only for
/auction(see the inline onpublisher.rs:5106). The review fixes live inside the merge commite6d51e36c: about 1,190 inserted and 626 deleted lines beyond a clean auto-merge ofee66fbfb9andda31a215e(the EdgeZero repin, the/auctiondispatch/collect split, +332 lines inpublisher.rs,config_payload.rs,settings.rs), visible only viagit show --cc. A clean merge commit plus a separate fix commit would make them reviewable. The October RC (#1228) still carries the pre-fix heads of the four source PRs. - 📝 No CHANGELOG entry, though this changes operator-facing behavior with rollback hazards:
[observability]is rejected by older binaries (Settingsisdeny_unknown_fields); older binaries ignoreauction_enabled = false;access_logs_rawis replaced (deploy ordering); the Axum dev server runs its own serve loop; and the diagnostics export now includes winning bidder names and bucketedhb_pb. - 📝
auction_events_rawcontract changes on failure paths./auctionand page-bids moved fromrun_auctionto dispatch plus collect: onDispatchFailed,ExecutionFailednow carries the launch-failure provider responses (one row becomes N+1; theendpoints.rs:1311-1319test change confirms it),/auction's summaryelapsed_msnow comes from the orchestrator instead ofobservation.elapsed_ms(), and a fatal admission error loses the "Planned auction admission failed" context. Probably improvements, but Tinybird consumers should hear about it. - 📌 Follow-ups the dispositions imply but nobody filed: the EdgeZero Axum service/layer hook (listed as "Upstream follow-up", but no issue exists on stackpop/edgezero); CI validation that
tinybird/SCHEMA, FORWARD_QUERY, fixtures and producer agree (requested on #1076; the 30-field check is manual); the duplicated Sanitize/Finalize/Auth middlewares in the Cloudflare and Spin adapters (requested on #1121); and transport-completion capture for the auction (see theR - Dinline). - ♻️/⛏ Smaller items (no inline comment, to stay under the comment cap):
SendContextis built three times with identical fields (main.rs:562,:603,:642); build it once after the finalize block.require_identity_graph_with_timing(main.rs:1307-1321) isidentity_graph_with_timing(...).ok_or_else(...).endpoints.rs:376-454andpublisher.rs:7401-7450re-implementrun_planned_auction's outcome mapping, keyed onsettings.auction.providersinstead of the planned set and hand-buildingOrchestrationResultinstead ofno_bid(). Equivalent today (plan compilation is 1:1), but three copies will drift; consider returning(result, launched)from the orchestrator.- The diagnostics seam branch
s(b,a,d)(publisher.rs:6259-6276) is unreachable: the seam is built only for template-authorized responses, which require!requires_private_no_store, while active diagnostics force private. collect_non_html_auctionstamps committed with no injection point (publisher.rs:4374-4380); page-bids synthesizesauctionWaitMs = R - Dwithpre_headerfor the browser while the access row for the same request hasauction_wait_msnull.RouteMetadata.route_template: Stringallocates on every routed response even with access telemetry off;Cow<'static, str>(borrowed for named routes,/,/other/*) avoids it.- The
AdBidsStatedoc block (publisher.rs:3281-3294) now documentsBrowserAuctionDiagnostics; move the new items above it. /verify-signatureand/_ts/admin/keys/*are classifiedRouteClass::Ec; they are request-signing routes.handle_page_bidsnow runsgpt_diagnostics::prepare_request(which can fail on config) before the CSRF gate whose comment says it runs "before any other work".route_sectionsaccepts entries that can never match a percent-encoded path (spaces, non-ASCII,?,#,*;*would emit/*/*); restrict to unreserved ASCII. Docs say "case-insensitively" (configuration.md:2963); it is ASCII-only.max_body_bytes >= 1024does not guarantee one access row fits (bounded inputs reach 1,074 bytes); raise the floor whenaccess_enabledor soften the doc claim atsettings.rs:1879-1882.every_phase_index_is_unique_and_in_bounds's comment claims a compile-time guarantee the hand-written array doesn't give;#[repr(usize)]plusPHASE_COUNT = Phase::Stream as usize + 1would.rendered_names_never_include_vendor_termscannot fail (the names are static literals).- Fixture row 2's
template_cache_state: "bypass"is not a producible value (asset rows emitunknown), and row 1 showstime_elapsed_ms>auction_resolved_mson a streamed row, against the documented ordering. - Axum:
main.rs:44has an unresolved intra-doc link toRouterService(cargo docwarns); "Listening on" is logged before the bind succeeds; tower 0.4 is now a normal dependency next to tower 0.5 from axum 0.8 (bumping the workspace totower = "0.5"passestest-axumand drops 0.4 from the lock). - Cloudflare excludes
/healthfrom timing but has no/healthroute, so a real publisher path loses its collector; Fastly short-circuits onlyGET /healthwhile the others exclude every method. - Each adapter starts T0 at a different point (Fastly before config-store open, Axum at
TimingService::call, Cloudflare/Spin after a per-request app build), so GPT-diagnostics offsets are not comparable across adapters; worth a table in spec §8a and a sentence ingpt-diagnostics.md. Spec §8a also says the freeze point is insideAxumDevServer(it isn't) and that Axumstream_msmeasures write-out (Axum never recordsStream);axum/src/timing.rs:27-30still says Fastly builds state per request. - Cloudflare/Spin timing tests: deleting
RequestTimingMiddlewarefrom either adapter's router leavestest-cloudflare/test-spingreen, and forcingserver_timing_enabled = falseindev_server_serviceleaves the Axum suite green. The new Cloudflare and Spin middleware tests are byte-identical, exercise EdgeZero's middleware on a private router rather than TS'sbuild_router, and rustfmt gave up on their long closures. - Cargo.lock rewrites unrelated edges (
windows-sys 0.61.2 -> 0.48.0,hashbrown 0.17.1 -> 0.16.1); restore them when repinning. - JS:
currencyhas no producer anywhere (delete until one exists); "Compatibility field" describesauctionWinner, which is new here; publisher-initiated Prebid auctions are never classifiedclient_side(only the synthetic refresh records one), so the docs read broader than the code;gpt-diagnostics.md:381-383says the badge addsCompeting pathsforcompetingwhilebadges.ts:118suppresses it;store.test.ts's "does not infer an SPA auction from navigation generation alone" overstates what is tested; the browser spec captures the closed shadow root twice andnot.toContain("1×1")passes vacuously on empty text; a lazy-loaded slot rendering more than 30 s after its auction dropsbidWonsilently (plausible, unprobed). - Docs: the two
[tinybird]tables (configuration.md:1312-1323and:3040-3048) disagree on defaults/types andaccess_datasetis marked required though it defaults; "(host, store, credentials)" at:3042refers to the ignoredsecret_store; the key-sections row for[observability]omitsroute_sections; the dictionary lists aGPT-reported creativelabel the UI never renders; the rollback warning says an older binary reads an access-only config as auction telemetry on, but without an auction token it fails startup instead. - Style:
randsits out of alphabetical order in the FastlyCargo.toml, anduse rand::Rng as _;sits between std imports.
CI Status
- cargo fmt: FAIL (required)
- cargo test: FAIL (required)
- cargo test (axum native): FAIL
- prepare integration artifacts: FAIL
- format-typescript: PASS (required)
- format-docs: PASS (required)
- vitest: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: SKIPPED
- integration tests: SKIPPED
- integration tests (Fastly EC lifecycle): SKIPPED
| # Temporary integration pin for EdgeZero PR #389. Before merging TS into main, | ||
| # replace all six pins with the approved EdgeZero release tag and revalidate. | ||
| edgezero-adapter-axum = { git = "https://github.com/stackpop/edgezero", rev = "ab444946fcacb1d44c020c6079abb6ba29231402", default-features = false } | ||
| edgezero-adapter-cloudflare = { git = "https://github.com/stackpop/edgezero", rev = "ab444946fcacb1d44c020c6079abb6ba29231402", default-features = false } | ||
| edgezero-adapter-fastly = { git = "https://github.com/stackpop/edgezero", rev = "ab444946fcacb1d44c020c6079abb6ba29231402", default-features = false } | ||
| edgezero-adapter-spin = { git = "https://github.com/stackpop/edgezero", rev = "ab444946fcacb1d44c020c6079abb6ba29231402", default-features = false } | ||
| edgezero-cli = { git = "https://github.com/stackpop/edgezero", rev = "ab444946fcacb1d44c020c6079abb6ba29231402" } | ||
| edgezero-core = { git = "https://github.com/stackpop/edgezero", rev = "ab444946fcacb1d44c020c6079abb6ba29231402", default-features = false } |
There was a problem hiding this comment.
🔧 wrench: The pinned EdgeZero rev cannot build the Fastly adapter.
ab444946 (feat/shared-request-timing, EdgeZero #389, still open with changes requested and not mergeable) has the request-timing collector but not edgezero_adapter_fastly::lifecycle, which main needs since #1179: it is 2 commits ahead of and 1 behind main's pin 683202c. Every Fastly build fails with cannot find 'lifecycle' in 'edgezero_adapter_fastly' at sandbox.rs:61, main.rs:118 and main.rs:139, which is the root of all four red checks (see the CI section in the review body).
For review I rebuilt the combination locally (683202c merged with ab444946 merges cleanly) and patched it in. clippy-fastly, test-fastly (262 adapter + 2,950 core), test-fastly-reuse, and the Axum/Cloudflare/Spin clippy and test aliases all pass against it, so the code is sound on that base. Nothing reachable from this PR builds it, though.
Merge gate, as the comment above already says: repin all six crates to an EdgeZero release, or at least a pushed commit that contains both #379 and #389, and get CI green including the integration and browser jobs. When regenerating the lock, use a scoped update and restore the unrelated edges this one rewrote (windows-sys 0.61.2 -> 0.48.0 in two places, hashbrown 0.17.1 -> 0.16.1). Until then, consider converting the PR to draft so it can't be merged by accident.
There was a problem hiding this comment.
Still a merge blocker. We will wait for a stable EdgeZero release containing lifecycle and request timing, repin the workspace entries with a scoped lockfile update, then run committed-dependency CI including browser integration before requesting another review. Local-candidate Fastly passes are provisional, not a green committed dependency graph.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
This consolidates request timing (shared clock, private-response Server-Timing, sampled post-delivery access telemetry), auction timeline offsets and the GPT diagnostics overhaul. The core design holds up well: clock math saturates correctly, Server-Timing contains only static or numeric content, the reusable sandbox has no per-request state leak, the 30 Tinybird columns match the producer exactly, and no PII reaches access rows. It can't merge yet, though: the EdgeZero pin breaks the Fastly build (CI red), the Tinybird migration doesn't line up with main's schema, and one of the stated telemetry contracts isn't what the code does.
6 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch) to apply them. Each was applied alone to the PR head and checked: prettier and vitest for the JS ones;cargo fmt --check, the targeted native core tests andcargo docfor the Rust ones. The full Fastly gate can't run at this pin, so the Rust suggestions are compile- and targeted-test-verified only. The remaining comments describe the fix in prose because they span hunks or files.
Blocking
🔧 wrench
- EdgeZero pin breaks the Fastly build; CI red: see Cross-cutting.
❓ question
FORWARD_QUERYvsmain'saccess_logs_rawschema: see inline attinybird/datasources/access_logs_raw.datasource:41- Disabled auction still gets an access UUID (spec and PR body say null): see inline at
crates/trusted-server-core/src/publisher.rs:5106
Non-blocking
🤔 thinking
- Field-qualified
private="…"passes the private-response gate:request_timing.rs:345 - Rollback hazard for non-default
[observability]/access_enabled; doc names the wrong cause:settings.rs:2814 - Inactive
access_token_secretnow push-validated; shared key trips Spin collision check:config.rs:185 - Blocking post-delivery Tinybird send, up to ~2s per sampled request:
fastly/main.rs:827 - Auction wait recorded when no provider launched:
publisher.rs:4362 - Cloudflare/Spin collector wiring untested:
cloudflare/app.rs:606 - Docs don't say the flags are no-ops on Cloudflare/Spin:
docs/guide/configuration.md:2990 - Re-entered evicted slot reuses its number:
gpt_diagnostics/store.ts:1258 - Diagnostics failure can suppress the GAM refresh (suggestion):
prebid/index.ts:2978
♻️ refactor
DeliveryOutcome.result/.bytesunused; truncated streams look successful:fastly/main.rs:860- Dispatch → collect → NotStarted mapping triplicated:
publisher.rs:7437 - Page-bids diagnostics duplicate
from_request_timings:publisher.rs:7457 - Duplicated Cloudflare/Spin middleware test:
cloudflare/middleware.rs:298 - Diagnostics-only code in the production Prebid shim; cap raised:
prebid-artifact-integration.test.mjs:328 - Duplicate call branches only for test argument counts:
gpt/index.ts:1414 SendContextrepeated 3×;main.rs~2,310 lines:fastly/main.rs:562
⛏ nitpick
gpt_diagnostics::prepare_requestruns before the page-bids CSRF gate:publisher.rs:7173- Struct inserted between
AdBidsStateand its rustdoc:publisher.rs:3295 - Stale
run_auctioncomments:publisher.rs:7343 expectafter send infrom_access_config:fastly/tinybird.rs:90route_sectionsacceptsotherand non-ASCII:settings.rs:2866noreferreron the dictionary link (suggestion):overlay.ts:724unwrap()in test (suggestion):endpoints.rs:1411- Unresolved intra-doc link (suggestion):
axum/src/main.rs:44 - Auction-token error message condition (suggestion):
settings.rs:1978 .gitignorecomment source file (suggestion):.gitignore:67--punctuation still present (topic r4186272563):axum/src/timing.rs:13
🌱 seedling
- Fixture row the code can't produce:
tinybird/fixtures/access_logs_raw.ndjson:2
Cross-cutting / body-level findings
-
🔧 The committed EdgeZero pin breaks the Fastly build, and CI is red.
ab444946is not a descendant of683202c: the compare API reportsdiverged, ahead_by=2, behind_by=1. It drops "Add opt-in reusable application lifecycles (#379)", whichmaindepends on. All four failing checks (cargo fmtandcargo test, both required, pluscargo test (axum native)andprepare integration artifacts) fail witherror[E0433]: cannot find 'lifecycle' in 'edgezero_adapter_fastly'atcrates/trusted-server-adapter-fastly/src/sandbox.rs:61andsrc/main.rs:118/:139. Corerequest_timing.rsnow aliasesedgezero_core::request_timing::*, so the whole feature rests on an unreleased upstream API (EdgeZero #389 is still open). Fix: rebase #389 onto EdgeZeromain(on top of #379), release, and repin all six crates. Please mark this PR as a draft until then. -
❓ PR state vs description. The body opens with "Draft and dependency blocker" / "Not mergeable", but the PR isn't marked as a draft. It is also
CONFLICTINGagainstmain. The body says "All four originals remain open", but #1074, #1076, #1121 and #1154 are all closed, so the review-topic table is now the only record of those threads. Please convert to draft, resolve the conflicts and refresh the body. -
🤔 Stale "access logs are not emitted" text contradicts this PR's new
[tinybird]table:docs/guide/configuration.md:1307-1308("access-log emission is not wired")docs/guide/telemetry.md:44-47(access_enabled = true"is rejected",access_token_secret"is deprecated input")tinybird/README.md:13-14("reserved; Trusted Server does not currently emit access logs"). The README also needs anaccess_logs_rawAPPEND-token step and the schema cutover note.
Two topics marked implemented are only partly done:
- r4186272474:
docs/guide/configuration.md:3042still says "host, store, credentials". - r4186272448: spec
…-design.md:481still lists asecret_storeand "a positivemax_body_bytes".
The plan is also stale: line 293 lists
secret_store, and line 648 still describesTimedKvStoreas "a decorator implementingPlatformKvStore", which this PR removed (r4186272463). -
⛏ Unrelated
Cargo.lockdowngrades, probably from merge resolution:coloredandwinapi-util:windows-sys 0.61.2 → 0.48.0gix-imara-diff:hashbrown 0.17.1 → 0.16.1
Please restore those entries from
mainso the lock diff shows only the EdgeZero repin andrand. Also,randis out of alphabetical order incrates/trusted-server-adapter-fastly/Cargo.toml. -
⛏ Axum
TimingService::poll_readyreadiesself.innerbutcalluses an un-polled clone (axum/src/timing.rs:79,:101-103). It's harmless while the inner service is always ready. Swap in the readied service (std::mem::replace), or document thatSmust be always-ready. -
⛏
normalize_methoddoc (access_telemetry.rs:24) says every standard method keeps its name, but CONNECT and TRACE map to"other", so EdgeZero TRACE canary rows show up asother. Either add both or reword the doc. Theto_ascii_uppercase()there also allocates per row; aneq_ignore_ascii_casescan over a const list avoids that. -
⛏ The browser spec patches
attachShadowtwice (gpt-diagnostics.spec.ts:217-230).captureClosedShadowRootsalready records every root; useroots.get(document.getElementById(HOST_ID))throughout. -
⛏ Lost error context in Axum
main.rs. The runtime build (:61) andserver.await(:86-90) now surface rawio::Errors. edgezero wrapped them with "failed to build tokio runtime" / "axum server error". -
⛏ The label dictionary (
gpt-diagnostics-dictionary.md:13) listsGPT-reported creative, which the UI never renders. The overlay showsAd Manager reported … · creative <id>. Separately, new Fastly test configs addtest-publisher.comhosts. That follows an existing pattern, but CLAUDE.md asks for example.com only. -
🤔 Reviewability. 55 commits, 68 files, +10,974. Consider splitting into:
- (a) timing collector,
Server-Timing, Fastly access telemetry and the Tinybird schema, gated on the EdgeZero release - (b) auction timeline offsets
- (c) GPT diagnostics JS and docs. The JS parts don't need the pin and could land independently.
- (a) timing collector,
-
🌱 Follow-ups:
- Add
ENGINE_PARTITION_KEY "toYYYYMM(event_ts)"alongside the 30-day TTL, so expiry drops whole parts. - Strip upstream
ts-*Server-Timingentries before appending, so an origin can't spoof TS metric names. enabled = truewith both sinks off is accepted and emits nothing; reject it or warn.- The 30s
bidWonwindow vs GPT lazy-load: show "win not observed within window" rather than leaving it blank. - The hand-rolled Axum
serve()copies edgezero's privateserve_with_stores. File an upstream issue for anAxumDevServerlayer seam so it doesn't drift on the next repin. /healthexclusion is method-agnostic on Axum/CF/Spin butGET-only on Fastly.- On multi-threaded Axum, contended spans are silently dropped. That's acceptable for a dev server, but don't treat Axum numbers as authoritative.
- Missing tests:
access_sample_rateset to NaN, negative or >1; non-default[observability]serialization; andpublisher_route_templatewith a trailing slash,//or percent-encoding.
- Add
-
👍 Checked and fine:
Server-Timingvalues are static names plus numeric formatting only, gated on private/no-store with no edge-cache headers, and appended (not overwritten).- Clock arithmetic is monotonic with saturation (
duration_mscaps atu32::MAX). TimedKvStoreforwards all seven methods and times each operation exactly once.- Secret gating is symmetric across deploy, runtime and blob load.
- No new statics or thread-locals, so the reusable sandbox doesn't leak per-request state.
- Telemetry runs after
send_to_client/finish(), and the pull-sync KV time doesn't leak intokv_ms. - The 30 producer keys match the 30 datasource columns, and access rows contain no IP, UA, query string or raw EC IDs.
- The overlay uses no
innerHTML; all strings go throughtextContent. - Diagnostics-off paths are a no-op, and single-shot SSAT attribution holds.
- The auction-ID renaming (
diagnostics_request_id) removes the earlier name collision.
CI Status
- cargo fmt: FAIL (required)
- cargo test: FAIL (required)
- cargo test (axum native): FAIL
- prepare integration artifacts: FAIL
- format-typescript: PASS (required)
- format-docs: PASS (required)
- vitest: 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 test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS
- CodeQL: PASS
- CLAUDE.md symlink guard: PASS
- integration tests: SKIPPED
- integration tests (Fastly EC lifecycle): SKIPPED
- browser integration tests: SKIPPED
Brings #1175 to 3add18d, which carries main through 5319594 and repins EdgeZero to v0.0.9. Conflict resolutions: - Fastly and Axum entry points keep the RC's request-timing stack and adopt main's reusable sandbox (#1179) and RUST_LOG handling (#1227). Sandbox counters are threaded through send_edgezero_response alongside SendContext, following the shape used in #1261. - tester_cookie.rs takes main's final #1137. - #1175's own docs, CLI test and Viceroy fixture take its new revision. - browser.rs drops the RC-only launch timeout const that main's #1256 now defines.
Preserve the legacy Tinybird access schema and introduce access_logs_v2 with a scoped append token and documented cutover/rollback. Keep disabled-auction access IDs null and count only actual planned-provider launches. Add navigation, streaming, KV, Fastly freeze/geo/body-limit and JS classification/correlation regression coverage. Document privacy, retention, sampling and post-delivery ingest costs.
Keep completed-auction diagnostics exceptions from suppressing GAM refresh, and preserve the captured winner and timing facts in publisher first-impression fallback. Require bare private or no-store directives before appending Server-Timing while preserving quote-aware directive matching. Call the Axum service instance whose readiness was polled rather than an unreadied clone. Cover throwing diagnostics setup and fallback recorders, snapshot preservation, qualified cache directives and readiness-sensitive services with regression tests.
Gate duration and placement on the actual launch flag in both collectors, without skipping collection, bid-state updates or terminal summaries. Preserve measured zero waits. Cover empty tokens in both placements, buffered and streamed routing-only navigation, and zero-duration recording. Document null and zero semantics.
Preserve the timing-aware Axum terminal wrapper alongside main's logging defaults, retain both Fastly health contracts, and keep portable publisher clocks. Align access version rustdoc with the new header split and handle fallible normalization in route-section tests. Validated merged JS, adapter/parity and target-matched lint gates. Fastly passes only against the disclosed local lifecycle-plus-timing candidate; the public pin remains blocked. Native core passes without Cargo's build-version environment injection, whose legacy test-loader failure is reproduced on unmodified main.
|
Merged current main with a normal merge commit, 686826e. Conflicts are resolved and GitHub reports MERGEABLE. The dependency manifest and lockfile are unchanged. Posted dispositions on 90 review threads and resolved 89: 58 on this PR and 31 on the related source PRs. The current EdgeZero dependency thread, r4240834032, remains open. Deferral replies explicitly describe unchanged behavior; they supersede earlier disposition entries that overstated optional cleanup as implemented. The approved correctness, test and versioned-schema fixes, the four follow-up fixes and the launch-gated wait correction remain in the branch. Local checks on the merged tree passed JS build/lint/format and all 1,236 Vitest tests; Axum 56, Cloudflare 55, Spin 89, cross-adapter parity 17, CLI 890, native filesystem tests, native and WASM lint including the portable-clock gate, and docs/Markdown checks. Fastly adapter 268 and core 3,002 tests, reuse, build and lint pass only against the disclosed local lifecycle-plus-timing candidate, not the committed pin. Native core has an inherited test-environment failure from main: Cargo injects the build-only TRUSTED_SERVER__GIT_VERSION into the legacy test-only environment loader, which rejects git_version. The same failure was reproduced on an unmodified archive of main. The rebuilt merged core binary passes all 3,008 tests with only that build-only variable removed. Normal cargo test is not claimed green. Rustdoc builds but retains warnings, including the deferred Axum link correction. Deferred work remains visible in the thread replies: timing contract/transport-completion semantics, fallback route identity and adapter wiring/health coverage, synchronous ingestion/target caching, raw-TOML secret preflight, delivery-result cleanup, diagnostics bundle/UX/privacy hardening, and documentation/refactor cleanup. No follow-up issues or deployments were created. Current CI is still blocked by the committed EdgeZero pin's missing lifecycle API. The misleadingly named cargo fmt job fails in Fastly clippy, and the Axum job fails in its Fastly release-build verification, not the Axum tests. Integration and browser jobs remain skipped. We will wait for a stable EdgeZero release, repin with a scoped lockfile update, then get committed-dependency CI and browser integration green before re-requesting review. No review request has been sent. |
This branch has not been deployed
4 participants
Draft and dependency blocker
This consolidates #1074 → #1076 → #1121 → #1154 against
main, including the actual source tips and their newer main merges. All four originals remain open; no review threads have been resolved.Not mergeable: the committed public EdgeZero pin does not build Fastly. All six direct pins use timing candidate
ab444946, which has the collector but lacks the reusable lifecycle API required by Trusted Server main. Neither the current release nor that timing candidate satisfies both requirements.The combined code passed local checks using a separate, unpushed EdgeZero integration candidate combining lifecycle base
683202cwith timing headab444946. This draft deliberately contains no absolute-path dependency patches. Publishing that dependency branch, repinning the six dependencies and validating the reachable git-backed graph are still pending. Do not treat the local results below as validation of this draft's public dependency pin.The required timing API remains unreleased. Release-backed repinning and revalidation are merge gates. Newer EdgeZero main also removes the store API this TS base uses; adopting that store migration is outside this consolidation. No upstream branch was changed, merged or released.
Changes
Fixes and main integration are in
e6d51e36; the four-source combination isee66fbfb.Checks
Rust results below used the local combined EdgeZero dependency candidate, not the committed public pin:
Not verified: reachable combined git pin, release-backed dependency, GPT diagnostics Playwright runtime, deployed cross-adapter clock behavior, live Tinybird schema migration/ingestion, production providers, or fronting delivery-cache pass-through. Coordinate the schema cutover and cache checks before rollout.
Review topic dispositions
31 original open threads represent 30 topics, including one duplicate. These statuses describe implementation, not GitHub thread resolution.