Repository navigation
Consolidate request timing and GPT diagnostics - #1261
ChristianPavilonis wants to merge 77 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.
|
Published eight targeted review-fix commits, ab58ced through c4cec08. The working tree is clean; EdgeZero pins and Cargo.lock are unchanged. The PR is now draft while required merge gates remain. Resolved 26 addressed threads with commit/check dispositions. Six non-EdgeZero threads remain open for the next contract/design discussion, plus the two unchanged dependency threads. Deferred work is tracked in #1458; #1148 and #1155 now describe the consolidation branch accurately. Three source-PR replies that incorrectly called already-implemented allocation, routes-doc and collector-presence fixes deferred have been corrected. Validation: all 1,237 Vitest tests, JS build/lint/format, docs build/lint/format, Markdown/domain checks, Axum 56, Cloudflare 55, Spin 89, parity 17, CLI 890, native/WASM Clippy outside Fastly, Rustfmt, portable-clock and build-digest checks pass. The page-bids and Axum regressions fail under their targeted mutations and pass after restoration. The rebuilt core binary passes all 3,011 tests without Cargo's build-only TRUSTED_SERVER__GIT_VERSION injection. Normal cargo test still fails 11 inherited environment-loader tests; this is not reported as a green Cargo gate. Native rustdoc retains 32 core warnings. Fastly test/reuse/lint remain blocked by the committed pin's missing lifecycle API, so the new Fastly constructor test and browser/integration runtime are unverified. No local dependency patch or live Tinybird deployment was used. GitHub still reports conflicts with current main. No merge-main, release-candidate update, deployment or review re-request was performed. |
|
Published six contract-fix commits, 7214c60 through 02f020a. All six remaining non-EdgeZero review threads are addressed and resolved; fresh verification leaves only the two dependency threads open. #1458 and #1155 now distinguish implemented changes from remaining validation/follow-ups. The access v2 row now has TS build version semantics, auction_collected_ms and delivery_result. Actual collection waits and planned-provider outcome mapping are shared across routes, including observed mediator work without fabricated provider dispatch or new requests. Access ingestion dispatches without waiting for the reply. Prebid diagnostics moved behind the optional recorder, with exact-instance/lifecycle coverage and a 1,744-byte smaller shim. Badge evidence passes clicks through; its small control sits outside observed creatives, and latest selection no longer opens history. Checks pass: 3,017 core tests in the isolated compiled binary, 1,264 Vitest tests, Axum/Cloudflare/Spin, parity, CLI, build-digest, non-Fastly native/WASM lint, JS/docs build/lint/format, Markdown, clock and domain checks. The actual emitter module passes 19 WASM tests and Clippy using the original dependency identities. The SDK lifetime probe verifies complete asynchronous request bodies after callback completion, including reuse, with a synchronous control. Limits remain explicit: normal Cargo core execution still fails the inherited build-only git_version environment tests; full Fastly/main tests/lint cannot build the committed lifecycle pin, so the new delivery-boundary regressions and browser integration are unverified. Native rustdoc retains 32 core warnings. No live Tinybird deployment or production Fastly guarantee is claimed. Dependencies/lockfile unchanged, worktree clean. PR stays draft and conflicts with current main. No merge-main, RC update, deployment or review re-request. |
Deny cross-site page-bids before diagnostics setup, preserve the disabled timing fast path, and consolidate send contexts and recorder calls. Return access-target setup errors instead of panicking after send, with recording-client regressions. Document route-section limitations and finish review wording corrections.\n\nDependency pins and lockfiles remain unchanged. Full Fastly checks still require the EdgeZero lifecycle API; post-send helper coverage is isolated WASM evidence.
|
Implemented the approved small cleanup batch in 9654deb7a, as one commit. No dependency pins, manifests or lockfiles changed.
Executed checks
Still blocked or unverifiedFull Fastly test/reuse/Clippy checks still fail on the committed EdgeZero pin's missing lifecycle API. Full application delivery-boundary, browser and live Tinybird deployment/ingestion/quarantine/rollback proof remain outstanding. Main conflicts are unchanged. The matching review replies and #1458 distinguish completed cleanup from larger deferred work. Only the two dependency threads are to remain unresolved. The PR stays draft, with no review re-request. |
Preserve request timing and diagnostics alongside main's auction deadlines, trace workflow, and async HTML finalization. Resolve Fastly delivery helper and test API changes, keep trace responses outside ordinary timing finalization, and update trace projections and fixture hosts.
…date GPT browser assertions for the separate keyboard handle.
|
Updated all six EdgeZero pins to Local joint validation passed on Rust 1.98.1, with pinned Viceroy 0.21.0 for Fastly:
The corresponding EdgeZero candidate passes all six local CI gates, its five boundary regressions, the 14 shell guard regressions, docs lint/format/build, fixture host suites/Clippy and the full Fastly smoke suite under Viceroy 0.21.0. Fresh GitHub jobs, including full runtime/browser integration acceptance, are still running against this pushed pair. The coordinated merge gate remains open until they pass. Deployed-provider clock smoke testing is separate from local runtime evidence. Before merging this consumer PR, replace the temporary revisions with an approved EdgeZero release tag and complete final tag-backed validation. |
Status and merge gates
Do not merge yet. This PR remains draft. The committed EdgeZero timing pin lacks the Fastly lifecycle API required by the application entry point. Updating that dependency and the store-binding migration remain separate work. GitHub also reports conflicts with current main; this batch does not merge main or update the RC branch.
Neither the contract nor cleanup batch changes dependency manifests/lockfiles, uses local dependency patches, deploys live Tinybird, or re-requests review.
Changes
Consolidates request-phase timing, private-response
Server-Timing, sampled post-send Fastly access telemetry, auction offsets/UUID joins and GPT diagnostics from #1074, #1076, #1121 and #1154. All four source PRs are closed as superseded.Access emission is off by default. The separate
access_logs_v2datasource has 31 columns, 30-day retention and its own APPEND token. The legacy nine-columnaccess_logs_rawremains intact, without aFORWARD_QUERYor raw-path backfill. A deployed preview v2 schema needs a coordinated migration before this producer is deployed.Agreed timing and delivery contracts
ts_versionis the compiled TS build identifier matchingx-ts-version: pipeline override, exact tag, branch, then six-character commit fallback. It is not the numeric Fastly activation version. No extra SHA or Fastly-version column was added.auction_collected_msnames the persisted collection-return milestone. Internal Rust and browserauctionResolvedMsnames remain unchanged. Collection return is not a separately observed provider transport-completion timestamp./auction, page-bids and HTML use shared orchestration/outcome mapping and actual collection waits, not R minus D. No-work outcomes keep wait/collection/commit null. Actual mediator-only network work can record those fields without inventing provider dispatch. Dispatch/admission choices and outgoing request counts are preserved.origin_msremains caller-blocking origin wait, including only the remaining wait after overlap. Fastly app-build means per-request setup plus cold construction when required, not a cold-start flag. Adapter T0 differences are documented rather than treated as equivalent.send_asyncfollowed by dropping the pending handle. Serialization/setup/start failures are observable; later transport errors, HTTP status and remote ingestion acceptance are not. There is no response wait, retry or durability guarantee.delivery_resultreports local transport handoff ascomplete,partialorerror, independently of HTTP status. It does not prove client receipt. The unused duplicate delivery-byte field was removed; response bytes remain in the timing snapshot.Diagnostics changes
Prebid targeting readers, the bidWon listener and bounded correlation state now live in the optional diagnostics bundle. The always-served shim only calls a narrow optional hook with its exact targeted Prebid instance. The served shim measures 40,983 bytes versus 42,727 at the previous batch, a 1,744-byte reduction under the unchanged artifact budget. Destruction unsubscribes only this runtime's listener when supported; otherwise the listener becomes inert and releases its recorder reference.
Badge evidence is pointer-transparent. A small native Ad #N control is placed outside observed bound creative rectangles and hidden if no viewport-fitting position exists. Selecting the latest request focuses current details without opening history; earlier selections open history.
The earlier eight targeted review fixes remain, including zero-provider diagnostics omission, disabled-auction UUID consistency, private-header gating, Prebid registration retry, Axum readiness, access-target borrowing, legacy-dataset rejection, dictionary-link referrer suppression, fixtures and rollout documentation.
Final small cleanup
9654deb7a denies cross-site page-bids before direct-handler diagnostics preparation, returns early on disabled Server-Timing emission, shares one Fastly send context, and removes duplicate GPT recorder-call branches. Access-target construction is fallible; recording-client tests cover missing credentials, sampled-out responses, independent toggles and best-effort start failures. The guide/example document route-section limitations without new validation restrictions, and remaining Axum/Wrangler wording is corrected.
Validation
Published contract batch:
7214c6058through02f020a80; final cleanup: 9654deb7a. Latest checks use the committed cleanup source, verified by file fingerprints. EdgeZero pins, SDK versions, manifests and Cargo.lock are unchanged.TRUSTED_SERVER__GIT_VERSIONabsent. Normalcargo test -p trusted-server-core --lib --lockedstill fails the same 11 inherited environment-loader tests withunknown field git_version; this is not a green normal Cargo gate.tinybird.rs, platform, backend and management modules compile in a standalone WASM test wrapper with all 281 external dependency identities/checksums matching the original lock. All 94 wrapper tests and wrapper Clippy pass, including 20 tests in the actual Tinybird module, four exact source-excerpt post-send regressions from main.rs, and 70 existing platform/backend/management tests. Coverage includes async start/drop, no response wait, framing/auth/body limits, missing-token setup, independent toggles, startup failures and no retries. The wrapper does not compile or validate the full application entry point.A temporary-file user quota interrupted some logs/checks. Only this session's scratch artifacts were moved into the cache, and interrupted checks were rerun with retained nonempty logs. No source/dependency workaround or unrelated cleanup was used.
The SDK lifetime probe uses only local Viceroy backends. It is not a production Fastly or Tinybird guarantee. Full application Fastly/main, integration and browser verification still require the dependency gate. Real-browser geometry, hit-testing and keyboard behavior have not been exercised; current UI evidence is unit/layout mocks and independently built artifact composition.
Remaining tracked work
#1458 is the checklist with original review links and current dispositions. #1148 retains adapter support/startup diagnostics work. #1155 records delivery-result implementation and its remaining full Fastly/rollout validation.
Remaining follow-ups include route identity on fallback errors, production adapter collector/health coverage, raw-TOML secret preflight, broader historical spec/plan reconciliation and transport-completion instrumentation if needed. This PR does not claim those are implemented merely because an earlier thread was resolved.