Repository navigation
Consolidate request timing and GPT diagnostics - #1261
ChristianPavilonis wants to merge 55 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.
Conflicts: publisher.rs (origin span now wraps the early-dispatch pending-origin wait as well as the direct send, still dropping before the abandonment-telemetry branch), main.rs (EdgeZero env parameter threaded through the AppBuild span block and the finalize signature gaining both mut ec_state and timings), app.rs (both sides' test-module additions kept). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rface
Review round 3, the blocking route finding plus the config items:
- Publisher route templates now come from an operator allowlist
(observability.route_sections, default empty): a first segment that
matches an entry and has further depth emits the lowercased allowlist
entry as /{section}/*; everything else emits /other/*. The shape
heuristics (charset, length, digit bounds) are gone because they
could not bound identity: depth-2 first segments are usernames under
/{username}/posts shapes and single-segment paths are documents. The
emitted value set is now fixed by configuration, so no
request-derived byte reaches the row.
- Integration-proxy responses carry the registered route pattern
verbatim (bounded, integration-defined) instead of a classifier
output; the registry stores the pattern at registration.
- auction_enabled serializes only when false, so a pushed config
cannot silently re-enable auction telemetry on rollback; with a
serialization test alongside the observability one.
- The secret-store validator is renamed to validate_secret_store_key_name
with a key_name parameter: it validates an identifier, never a
credential, and the old name tainted the key name as a secret value
in CodeQL, lighting up eleven pre-existing log sites.
- Docs: the tinybird table gains its three missing rows,
max_body_bytes states the 1024 floor the code enforces, the rollback
guidance now describes the real compatibility boundary (push a
compat config first: drop [observability], access_enabled = false,
and enabled = false for access-only deployments), and the fixture
uses the example-domain convention.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The access sink streams the Tinybird response (the body is never consumed; buffered conversion materializes chunked bodies before the limit check) and newline-terminates rows to match the auction sink's NDJSON framing, with the recording client now asserting both. - Poisoned RequestTimings locks recover via into_inner instead of silently dropping every subsequent sample: the guarded data are plain counters, so one panic cannot blank the header and row for the rest of the request. Module and spec docs updated to stop conflating poisoning with contention. - The geo write-back skips 401 responses through a shared helper: resolve_geo_for_response short-circuits on 401 before consulting the carried state, so the old unconditional write downgraded a carried Resolved to Attempted and cost the row its country. - TimedKvStore forwards exists, so decorating a store with a cheap metadata probe (Spin) no longer downgrades it to the get-and-discard default body; with a contradiction-stub delegation test. - Post-send ordering is owned by run_post_send_steps, which both production sites route through, and the instrumented sequence test drives the real seam: elapsed stamped by send, then pull-sync, then telemetry. - Axum: dev_server_service remains the standard path; new tests pin flag-off suppression and the extension round trip (a phase recorded in the handler must surface as ts-filter in the header); the outer-wrapper rationale is reworded to the terminal-freeze-point argument; the configuration guide notes the flag is read once at startup. - The no-Cache-Control fail-closed case is pinned by a test, and t0's boundary (constructed after the adapter prologue) is documented. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…indings The base branch moved the auction dispatch block behind a request head snapshot, so the conflict in publisher.rs resolves to the base's relocated block with the timeline mark re-applied there. request_timing.rs auto-merged without a conflict but left the three new marks as the only lock-taking methods that did not recover from a poisoned mutex; they now match the rest of the collector. Instrument all three auction sources. Only the publisher navigation path marked before, so POST /auction and /_ts/page-bids emitted access rows that claimed no auction ran while auction_events_raw held a full record for the same request, and the documented join returned nothing for the route class named after auctions. Both handlers now take the collector off the request extensions and bracket run_auction. Split the join key from the dispatch mark. set_auction_id is called where the AuctionObservationContext is built, which happens on every auction-eligible request, so skipped and dispatch-failed auctions stay joinable to the rows they emit, and a dropped dispatch sample no longer takes the key with it. A null offset now means "this milestone was not reached" rather than "no auction ran", with auction_id separating the two; the abandoned-auction paths make that distinction load-bearing. Type auction_id as Nullable(UUID) to match auction_events_raw.auction_id. As a String with a 'none' sentinel the join was a ClickHouse type error, and casting the sentinel through toUUID throws. Untrack the generated Cloudflare wrangler config. The .gitignore entry alone was inert because the file is tracked on the base branch, so the merge re-added it. Also: extend the Tinybird fixture with the four columns (reusing the auction fixture's UUID so the pair demonstrates the join) and a no-auction row; make the first-call-wins test actually detect a restamp by sleeping between calls, verified by injecting a last-call-wins regression; assert the marks at both collect sites; correct the spec's interpretation ladder, which put time_elapsed_ms last even though headers commit before the stream seam. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…wn entry The status said the implementation was a follow-up PR; it is in this one. The generated Cloudflare wrangler config was ignored under a comment about defunct pre-rename crate directories, which it has nothing to do with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolve eleven conflict hunks across six files, plus two semantic conflicts the merge did not mark. Import and structural hunks are unions of both sides: the Fastly adapter keeps TimedKvStore, RouteClass/RouteMetadata and response_builder alongside main's StoreName, RuntimeStoreConfig and auction-plan symbols; the integration registry keeps main's enabled_integration_ids loop with the branch's route-path element added to the router value; the config template keeps both the new [observability] leaf and main's auction provider examples. main.rs keeps the AppBuild timing span wrapping main's runtime-store-based config open and app build. Port access telemetry onto main's resolved-secret model (#1036). tinybird.access_token_secret becomes Option<Redacted<String>>, resolved through config.rs as an optional secret reference like the auction token, so the Fastly access emitter reads it from settings instead of opening a Secret Store per emission; load_access_token and validate_secret_store_key_name are gone. Secret-reference validation now gates the auction token on auction_enabled so an access-only configuration is expressible. Drop main's guard rejecting access_enabled, which this branch wires the emitter for. Forward EcKvStore::key_exists through TimedKvStore, added to the trait by main's Edge Cookie withdrawal hardening, and restore validate_tinybird_secret for the resolved-value checks.
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.
| FORWARD_QUERY > | ||
| SELECT event_ts, method, status, time_elapsed_ms, sample_rate, service_id, publisher_domain, env, route_class, route_template, body_mode, auction_wait_placement, appbuild_ms, filter_ms, geo_ms, kv_ms, origin_ms, template_cache_ms, auction_wait_ms, stream_ms, request_elapsed_ms, resp_bytes, template_cache_state, country, ts_version, pop, CAST(NULL AS Nullable(UInt32)) AS auction_dispatched_ms, CAST(NULL AS Nullable(UInt32)) AS auction_resolved_ms, CAST(NULL AS Nullable(UInt32)) AS auction_committed_ms, CAST(NULL AS Nullable(UUID)) AS auction_id |
There was a problem hiding this comment.
🔧 wrench: This FORWARD_QUERY selects columns that main's live access_logs_raw does not have.
Main's schema (git show da31a215e:tinybird/datasources/access_logs_raw.datasource) is event_ts, method, path, status, time_elapsed_ms, cache_state, country, sample_rate, event_date. The query selects service_id, publisher_domain, env, route_class, route_template, body_mode, auction_wait_placement, the phase columns, resp_bytes, template_cache_state, ts_version and pop, none of which exist there. They come from the intermediate schema on the stacked branch (72d5755, 7cf7d86), which is presumably the workspace the spec's tb --cloud deploy --check ran against.
Tinybird executes the forward query against the live datasource during deploy, and main's tinybird/README.md tells operators to tb deploy the whole project, so any workspace deployed from main has the 9-column version live. There the deploy fails and the datasource never migrates, and the README's schema-before-code ordering blocks the rollout. (I couldn't run tb here; tb deploy --check against a workspace deployed from main would confirm.)
Options: project from main's columns, e.g. CAST(time_elapsed_ms AS Nullable(UInt32)) AS time_elapsed_ms, ifNull(cache_state, 'unknown') AS template_cache_state, 'unknown' / 'other' / 'none' literals for the new dimensions and CAST(NULL AS Nullable(...)) for the new metrics; or ship a versioned datasource, as spec §9 and rollout step 4 already describe for the deployed-reserved case. Either way, document removing the forward query after promotion: left in place, the CAST(NULL ...) AS auction_* projections would null those columns on a later backfill.
❓ Which workspace was --check run against, and has main's reserved datasource been deployed anywhere?
| Ok(ProviderRequestOutcome::Immediate(response)) => { | ||
| immediate_response_count += 1; | ||
| provider_launch_count += 1; | ||
| completed_responses.push(response); | ||
| } |
There was a problem hiding this comment.
🔧 wrench: A provider that sends nothing is counted as launched.
In the planned path, the only Immediate a GenericOpenRtbProvider returns is the OpenRtbBuildOutcome::NoImpressions skip (provider.rs:300-304, tagged routing.skipped_no_usable_demand), so no request leaves the edge. Counting it sets has_provider_launch(), and all three call sites then stamp dispatched, resolved and committed (navigation publisher.rs:5149, page-bids publisher.rs:7407, /auction endpoints.rs:383); page-bids and active diagnostics documents also hand auctionDispatchedMs to the browser. That contradicts mark_auction_dispatched's own contract (request_timing.rs:206-208: routing-only skipped outcomes do not stamp it). It is reachable with a PBS or APS provider whose augment_request drops every impression.
Probe (orchestrator test: one PBS provider, one slot with {"trustedServer":{"storedRequest":false,"bidderParams":{"alpha":{}}}}, then dispatch and collect): requests_sent=0 has_provider_launch=true.
| Ok(ProviderRequestOutcome::Immediate(response)) => { | |
| immediate_response_count += 1; | |
| provider_launch_count += 1; | |
| completed_responses.push(response); | |
| } | |
| Ok(ProviderRequestOutcome::Immediate(response)) => { | |
| // Planned providers return `Immediate` only for the routing-only | |
| // `skipped_no_usable_demand` skip, so no request left the edge. | |
| immediate_response_count += 1; | |
| completed_responses.push(response); | |
| } |
The "produced an immediate outcome or started a request" wording at orchestrator.rs:87 and :92, request_timing.rs:206 and the spec's "or returned an immediate result" should follow, and the probe is worth keeping as a regression test (both outside this hunk). Verified alone: rustfmt, clippy-fastly, the 2,956 core tests, the 262 Fastly adapter tests and the Axum suite pass.
| // T0-anchored timeline (spec section 18): stamp the join key here | ||
| // rather than on dispatch, because every branch below emits an | ||
| // `auction_events_raw` row under this id — completed, dispatch | ||
| // failed, and skipped alike. Stamping it on dispatch would leave the | ||
| // failed and skipped rows unjoinable. | ||
| timings.set_auction_id(observation.auction_id); |
There was a problem hiding this comment.
🔧 wrench: A disabled auction produces a different access row depending on the route.
Navigation (here) and page-bids (:7346) stamp auction_id before branching, so auction.enabled = false with matching slots yields a non-null auction_id joined to an auction_disabled skip row. /auction's disabled branch (endpoints.rs:191-222, unchanged) builds an observation and emits the same Skipped { reason: "auction_disabled" } row under observation.auction_id, but never calls timings.set_auction_id, so its access row has auction_id = null and the skip row can't be joined. The spec table (§18, "null: no auction was attempted (assets, EC endpoints, disabled)") and the PR description ("Keep disabled auctions unattempted with null access UUID") match only /auction.
Pick one contract and apply it on all three routes. The smaller change, and the one that matches the comment above ("every branch below emits an auction_events_raw row under this id"), is to add timings.set_auction_id(observation.auction_id); after endpoints.rs:206 and reword the spec row to "no auction observation was built (assets, EC endpoints, no matched slots)". If null-for-disabled is the intent, skip the stamp here and at :7346 when !orchestrator.is_enabled() and accept unjoinable skip rows. Either way, add a test: nothing covers the disabled branch's join key today.
| { | ||
| DispatchAuctionOutcome::Dispatched(dispatched) => { | ||
| // The outcome can also carry skipped-provider diagnostics; | ||
| // only a real provider result/request establishes dispatch. |
There was a problem hiding this comment.
🔧 wrench: The navigation timeline and the stream placements are not covered by any test.
With these mutations applied together, all 2,956 core tests still pass:
if provider_launchedhere →if true(dispatch stamped for routing-only dispatches)- the
collect_stream_auctionguards at:4433and:4451→provider_launched || true - delete
timings.set_auction_id(observation.auction_id)at:5106(the navigation join key is never asserted) InStream→PreHeaderat:2542(template-hit streaming) and:2660(non-HTML streaming)- delete
&& timings.snapshot().auction_dispatched_ms.is_some()at:7459 - delete
drop(origin_span);at:5390
Why they survive: no test drives handle_publisher_request with an attached collector; the collect-site tests use immediate_no_bid_for_test, which hard-codes provider_launched = true; active_page_bids_omits_timings_when_no_provider_dispatches (:24629) never attaches a collector, so it passes on the has_request_timings gate alone; and origin_span_is_recorded_when_the_origin_send_fails (:9626) passes slots: &[], so the abandonment await it says it guards never runs, and origin_ms.is_some() would hold even if the span lived to the end of the function.
Also in core: the TimedKvStore tests (platform/timed_kv.rs:86-109, ec/kv.rs:1721-1740) only assert kv_ms.is_some(), which a single span satisfies, and key_exists, list_keys_with_prefix, count_keys_with_prefix and delete are never exercised.
Suggested tests: navigation with an attached collector across Dispatched / Skipped / DispatchFailed, asserting snapshot.auction_id equals the summary row's id; a collect from empty_for_test asserting resolved and committed stay None; placement assertions for both streaming paths; the page-bids test with a collector attached, asserting auctionDiagnostics is absent; an auction behind a delaying telemetry sink, asserting origin_ms stays below the delay; and an inner KV store that sleeps per call, asserting kv_ms grows per method.
| // fall back to a plain assignment, where no SPA hook exists to race with. | ||
| if let Some(auction_diagnostics) = auction_diagnostics { | ||
| let diagnostics = serde_json::to_string(auction_diagnostics) | ||
| .expect("BrowserAuctionDiagnostics should serialize"); |
There was a problem hiding this comment.
⛏ nitpick: expect messages use the "should ..." form (AGENTS.md). Same at :6262.
| .expect("BrowserAuctionDiagnostics should serialize"); | |
| .expect("should serialize browser auction diagnostics"); |
| //! freeze point: by the time a response reaches this layer -- after | ||
| //! `RouterService::oneshot` inside `EdgeZeroAxumService::call` has | ||
| //! converted any dispatch error into a plain response -- every response is |
There was a problem hiding this comment.
⛏ nitpick: The -- punctuation from the #1074 thread (r4186272563) is still here and at :340 ("NotFound branch -- exactly the"); the disposition table lists it as implemented.
| //! freeze point: by the time a response reaches this layer -- after | |
| //! `RouterService::oneshot` inside `EdgeZeroAxumService::call` has | |
| //! converted any dispatch error into a plain response -- every response is | |
| //! freeze point: by the time a response reaches this layer (after | |
| //! `RouterService::oneshot` inside `EdgeZeroAxumService::call` has | |
| //! converted any dispatch error into a plain response), every response is |
| # Wrangler config generated by the Cloudflare integration-test harness from | ||
| # wrangler.toml at run time; regenerated on every run. |
There was a problem hiding this comment.
⛏ nitpick: The harness renders this file from wrangler.ci.toml (crates/trusted-server-integration-tests/tests/environments/cloudflare.rs:26-27), not wrangler.toml.
| # Wrangler config generated by the Cloudflare integration-test harness from | |
| # wrangler.toml at run time; regenerated on every run. | |
| # Wrangler config generated by the Cloudflare integration-test harness from | |
| # wrangler.ci.toml at run time; regenerated on every run. |
| selectRequest(runtimeSlotNumber: number, requestNumber: number): void { | ||
| if (this.destroyed) return; | ||
| this.selectedRequest = { runtimeSlotNumber, requestNumber }; | ||
| this.historySlotToOpenOnce = runtimeSlotNumber; |
There was a problem hiding this comment.
⛏ nitpick: Badges always point at the latest request, so this opens "Request history" on every badge click, while gpt-diagnostics.md:451 says only selecting an earlier request opens it. Probe: after two requests, selectRequest(1, 2) (the latest) opens the history.
| this.historySlotToOpenOnce = runtimeSlotNumber; | |
| const requests = this.store | |
| .snapshot() | |
| .slots.find((slot) => slot.runtimeSlotNumber === runtimeSlotNumber)?.requests; | |
| const isLatestRequest = requests?.[requests.length - 1]?.requestNumber === requestNumber; | |
| this.historySlotToOpenOnce = isLatestRequest ? undefined : runtimeSlotNumber; |
Prettier, eslint, the gpt_diagnostics suites and build-all.mjs pass with this change; no existing test covers the latest-request case, so one is worth adding.
| dictionaryLink.href = | ||
| 'https://iabtechlab.github.io/trusted-server/guide/integrations/gpt-diagnostics-dictionary'; | ||
| dictionaryLink.target = '_blank'; | ||
| dictionaryLink.rel = 'noopener'; |
There was a problem hiding this comment.
⛏ nitpick: Without noreferrer, a publisher page with Referrer-Policy: unsafe-url sends its full URL, query included, to github.io when someone opens the dictionary.
| dictionaryLink.rel = 'noopener'; | |
| dictionaryLink.rel = 'noopener noreferrer'; |
This branch has not been deployed
3 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.