Repository navigation
feat(core): add shared request timing collector and middleware - #389
ChristianPavilonis wants to merge 8 commits into
Conversation
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adds a generic per-request timing collector (RequestTimings<N, D>) and opt-in, insert-if-absent attachment middleware to edgezero-core, with a shared runtime scenario in the three WASM adapter contract suites. I found no correctness bugs. I'm requesting changes on two API-shape items that are cheap now but become breaking changes once Trusted Server pins a release tag. Neither touches Trusted Server's call sites at 2791ceb.
Findings
Blocking
- 🔧
TimingErrornot#[non_exhaustive]: inconsistent withEdgeError/KvError/ConfigStoreError, and adding it after the tag is breaking (request_timing.rs:17) - 🔧 Hoist
t0out of the mutex: makeselapsed()infallible and the origin structurally immutable;elapsed()currently fails under cross-thread contention (request_timing.rs:100)
Non-blocking
- 🤔 Generic lifecycle names:
headers_ready_total/request_elapsed/resp_bytesmirror TS internals (request_timing.rs:51) - 🤔 Cloudflare clock semantics: workerd's
performance.now()advances only across I/O, and the contract test runs in Chromium (docs/guide/middleware.md:169) - ♻️ Shared fixture via cross-crate
#[path], never run natively: Cloudflare/Fastly/Spintests/contract.rs - ⛏
RequestTimingslacksDebug;must_usemessage wording;with_excluded_pathsreplace semantics undocumented - ⛏ Guide: show paired type aliases (a generic mismatch silently yields no handle); drop the reference to an internal test file
- ⛏ Spec/plan status lines, unchecked tasks, local paths and "keep the PR draft" text are stale relative to the PR
- 🌱 Contention drop counter for native runtimes; config-driven exclusions; exposing the middleware-created handle to outer layers
Behavioral claims above were checked with throwaway probe tests against 9c03cc59: generic-mismatch silence, elapsed() failing under cross-thread contention, exclusion-list replacement, and no handle on the response.
📌 Out of Scope
- Deployed-provider smoke testing is still unchecked in the test plan. It matters most for Cloudflare, where the local harness doesn't use workerd's clock.
- The Axum adapter has no
tests/contract.rs, so the shared timing scenario doesn't run there.
CI Status
- fmt: PASS
- clippy (
--all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS (core: 484 unit tests, plus thecompile_faildoctest) - feature check (
fastly cloudflare spin): PASS - Spin
wasm32-wasip2check: PASS - GitHub: all 22 checks green, including Cloudflare/Fastly/Spin WASM tests
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Second pass at 9c03cc59. It covers areas the earlier review didn't reach: registering the middleware from the manifest, how the handle interacts with State<T>, CI coverage of the compile-fail test, how strong the runtime test is, and documentation consistency. I checked each finding against the code or with a throwaway probe. I'm requesting changes so these land together with the two 🔧 items from the earlier review, before the release tag.
Findings
Inline:
- 🤔
edgezero.tomlcan't register the middleware: aconst fnconstructor fixes it (middleware.rs:86) - 🤔
State<RequestTimings<…>>returns a 500 that points atwith_state(docs/guide/middleware.md:226) - 🤔 CI never runs the compile-fail test (request_timing.rs:40)
- 🤔 Overlapping spans on one slot add up past wall-clock time (request_timing.rs:129)
- 🤔 The runtime test can't catch a stopped clock (tests/support/request_timing.rs:37)
- ⛏
span(slot)?in the guide doesn't compile in a handler (docs/guide/middleware.md:233) - ⛏ The
web_timeexample needs aweb-timedependency (docs/guide/middleware.md:27) - ⛏ Assertions that can't fail (middleware.rs:360, plus four more listed there)
- ⛏
TimingSnapshotlacks theD = ()default (request_timing.rs:45) - 🌱
PhaseSpanhas nofinish/cancel(request_timing.rs:260)
Not attached to a diff line:
-
⛏
configuration.mdnow contradicts the corrected middleware guide:docs/guide/configuration.md:49still says "Manifest-driven middleware are applied in order before routes". This PR corrected the same claim inmiddleware.md:61-62. Suggest reusing that sentence: "Routes are matched first. Middleware then run in registration order for a matched route; unmatched 404/405 requests bypass the middleware chain." -
🌱 The middleware fits only two of Trusted Server's four adapters: at
2791ceb, only Cloudflare and Spin register it.- Fastly creates its collector before the app is built, so it can time AppBuild (
main.rs:128-131). - Axum reimplements insert-if-absent plus the
/healthexclusion as a tower service outsideRouterService(timing.rs:78-99). That also covers 404/405 and gives it a final point to render headers.
This is the gap behind the earlier 🌱 about reaching the middleware-created handle after the router returns. An EdgeZero
Servicewrapper aroundRouterServicewould cover unmatched routes and give every adapter one place to finish timing. - Fastly creates its collector before the app is built, so it can time AppBuild (
CI Status
- fmt: PASS
- clippy (
--all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS - doctests (
--workspace --doc): PASS (1 compile-fail test, 13 ignored) - feature check (
fastly cloudflare spin): PASS - Spin
wasm32-wasip2check: PASS - GitHub: 21/21 checks green
prk-Jr
left a comment
There was a problem hiding this comment.
😃 Reviewed all 11 changed files at 9c03cc5, including focused collector/API and middleware/adapter reviews. No actionable defects found.
The implementation preserves existing collectors, keeps phase and payload updates under one mutex, and follows the documented contention, cancellation, and explicit lifecycle contracts.
Local verification passed:
- cargo fmt --all -- --check
- cargo clippy --workspace --all-targets --all-features -- -D warnings
- cargo test --workspace --all-targets
- cargo test -p edgezero-core: 484 unit tests, two integration tests, and the compile-fail doctest passed; 13 existing doctests ignored
- cargo check --workspace --all-targets --features "fastly cloudflare spin"
- cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin
- Fastly WASM contract suite under Viceroy: all seven tests passed
- Docs lint and formatting
Initial sandbox socket-binding and certificate-store failures cleared on rerun outside the sandbox. All 22 GitHub checks were green at the reviewed commit. Cloudflare/Spin runtime suites and Trusted Server joint validation were not rerun as part of this review.
Keep core doctests in the workspace host suite alongside main's CLI test matrix.
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-review at 878be472, which is ab444946 plus the merge of main. Both 🔧 items from the first review are fixed correctly. t0 now sits outside the mutex, so elapsed() is lock-free, and a cross-thread test covers it. TimingError and TimingSnapshot are #[non_exhaustive]. I checked every other claimed fix against the code and each one holds. The deferrals recorded in the spec's "Deferred follow-ups" are reasonable.
I'm requesting changes for the items below. The main ones are the Cloudflare/Spin gap in the guide's advice on where to measure, and the release gate on Trusted Server #1261.
Findings
Inline:
- 🤔 Cloudflare and Spin have no adapter hook for preinstalling a handle (docs/guide/middleware.md:288)
- ⛏ The doctest gate is missing from the local gate scripts and
CLAUDE.md(.github/workflows/test.yml:97) - ⛏ The spec and plan have stale consumer references, the wrong browser name and old evidence counts (spec:8)
- ⛏ Some process and authorization text remains (spec:478)
Not attached to a diff line:
- ⛏ The PR description is stale: it still says Trusted Server migration and joint verification are "complete against this exact candidate". The test plan's counts (484 unit tests; Cloudflare 8, Fastly 7, Spin 13) and the "22 checks … identical to
9c03cc59" note describe the original commit, and the public API has been renamed since then. The Changes table also leaves out.github/workflows/test.ymland theedgezero-macrosfixture. Suggest refreshing it and marking joint validation as pending, as the spec's "Remaining release checkpoints" already does. - 📝 The release gate isn't met yet: Trusted Server #1261 pins all six crates to
ab444946. Its Fastly adapter usesedgezero_adapter_fastly::lifecycle::{Sandbox, run_custom}, which comes from #379 and exists on this branch only after the merge. As a result its CI fails to compile (E0433 attrusted-server-adapter-fastly/src/main.rs:125). The spec's checkpoints put renewed joint validation before the EdgeZero merge, and that validation hasn't happened yet. #1261 needs to re-pin to878be472or later and pass.
CI Status
- fmt: PASS
- clippy (
--all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS - core doctests (
-p edgezero-core --doc): PASS (1 compile-fail test, 13 ignored) - feature check (
fastly cloudflare spin): PASS - Spin
wasm32-wasip2check: PASS - GitHub: 28/28 checks green, including Cloudflare (10 tests, headless Firefox), Fastly/Viceroy (9) and Spin/Wasmtime (14)
…ression tests, synchronize local doctest gates, refresh verification records, and remove session-only narrative. Production behavior unchanged.
…t changing behavior.
|
@aram356 ready for another pass on the EdgeZero changes at Replied to and resolved the four inline threads. I also refreshed the PR description, including the missing workflow/macro fixture changes and the verification status. It no longer claims that the original joint validation covers this candidate. Your non-inline release-gate finding remains open. Trusted Server #1261 still pins all six crates to |
…delivery before closing the origin; add regression coverage.
Summary
Changes
crates/edgezero-core/src/request_timing.rs,src/lib.rsRequestTimings<N, D>, immutable lock-free origin, snapshots, compound updates, saturating accumulation, drop-time spans and explicit lifecycle markscrates/edgezero-core/src/middleware.rscrates/edgezero-core/tests/request_timing_boundary.rscrates/edgezero-macros/tests/app_macro.rs.github/workflows/test.yml,scripts/run_tests.sh,.claude/commands/check-ci.md,CLAUDE.mdelapsed()is infallible and does not take the state mutex.TimingErrorandTimingSnapshotare non-exhaustive. Lifecycle methods aremark_headers_ready(),mark_request_complete()andset_response_bytes().Callbacks run synchronously under one nonblocking mutex. Panics propagate; poison recovery is best-effort, without rollback or payload validation. Attachment does not finalize requests, measure streamed-body completion or cover unmatched routes. Trusted Server retains its phase enum, domain facts, wire names and
Server-Timingrendering.Fastly offers request-extension hooks; Axum can wrap the router in a tower service. Cloudflare and Spin currently require application middleware registered before the attachment middleware. The guide shows that pattern and its error-rendering, exclusion, 404/405 and streaming limits. Request-extension hooks for those adapters remain deferred.
Verification
Current pushed candidate:
018529795767effa1bf0e5761367f4559aa47b30.4dd6bf82: boundary example and three regression tests, synchronized doctest gates, refreshed spec/plan evidence and removal of session-only process text.01852979for Rust 1.95 and 1.98.1, without changing behavior.Historical local and post-merge verification is recorded separately in the implementation plan. The original joint verification at EdgeZero
9c03cc59/ Trusted Server2791cebdoes not validate the revised API or current candidate. Cloudflare CI uses headless Firefox, not the Chromium harness used for earlier local runs.Coordinated delivery / merge gate
Trusted Server #1261 still pins all six EdgeZero dependencies to
ab444946fcacb1d44c020c6079abb6ba29231402. Its current head,686826eaf2bc6b35a4f52f0731ac8ffee9bc78c5, has failing CI. That pin predates the merged Fastly lifecycle API needed by the consumer.Before merging EdgeZero, update the consumer facade and all six pins to one pushed candidate containing the merged lifecycle API, confirm one resolved EdgeZero core identity and rerun joint validation. This remains an unmet merge gate, not completed verification.
Trusted Server must then replace temporary revision pins with an approved EdgeZero release tag, regenerate and inspect its lockfile, and pass final tag-backed validation before merging. No merge or release is performed by this review follow-up.
Related work