Skip to content

feat(core): add shared request timing collector and middleware - #389

Open
ChristianPavilonis wants to merge 5 commits into
mainfrom
feat/shared-request-timing
Open

ChristianPavilonis wants to merge 5 commits into
mainfrom
feat/shared-request-timing

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Move generic request-timing ownership into EdgeZero: one portable origin, bounded phase slots, lifecycle marks, and application-owned payload under one shared mutex.
  • Add opt-in, insert-if-absent attachment middleware without moving application rendering or privacy policy upstream.
  • Coordinate adoption with Trusted Server #1121. TS migration and independent joint verification are complete against this exact candidate.

Changes

Crate / File Change
edgezero-core/src/request_timing.rs, src/lib.rs Generic RequestTimings<N, D>, consistent snapshots, compound updates, saturating accumulation, drop-time phase spans and explicit lifecycle marks
edgezero-core/src/middleware.rs Preserve an existing concrete handle; otherwise install request-local state unless the exact path is excluded
Core tests and shared adapter contract fixture Exercise typed application facade, one extension identity/origin, contention, invalid slots, poison recovery, cancellation, and portable runtime clock/attachment
Cloudflare/Fastly/Spin contract tests Run the shared timing scenario on each WASM runtime
Middleware guide and design/plan Document ownership, lifecycle limits, approved API contracts and coordinated delivery

Applications retain phase enums, domain facts, header names/order, serialization and exposure policy. In particular, Trusted Server retains Server-Timing rendering; no generic renderer is introduced. Callbacks run synchronously under one nonblocking mutex; panics propagate and poison recovery is best-effort, without rollback or payload validation. Attachment does not finalize requests, measure streamed-body completion, or cover unmatched routes.

Closes

No standalone EdgeZero issue. Related consumer work: IABTechLab/trusted-server#1121.

Test plan

Publication-stage reruns against this tree:

  • cargo test -p edgezero-core — 484 unit tests, two integration tests and one compile-fail doctest passed; 13 existing doctests ignored
  • cargo test --workspace --all-targets
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • cargo check --workspace --all-targets --features "fastly cloudflare spin"
  • cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin
  • git diff --check

Earlier implementation-stage validation, recorded in the committed plan (not rerun during publication):

  • WASM check/clippy matrix: Fastly wasm32-wasip1, Spin wasm32-wasip2, Cloudflare wasm32-unknown-unknown
  • Runtime contract suites: Cloudflare 8 tests in headless Chromium, Fastly 7 in Viceroy 0.17.0, Spin 13 in Wasmtime 44.0.1; Fastly library suite 88 tests
  • Generated-app build and excluded examples/app-demo gates
  • Docs lint/format and VitePress build
  • Deployed-provider smoke testing (local runtime harnesses are not deployed-provider evidence)
  • Trusted Server migration and joint validation at TS commit 2791ceb46908e6af04ccaae0c10b29d35336a9f4

Independent final verification

  • All 22 reported GitHub checks succeeded. Test/format CI used a synthetic merge whose tree was verified identical to candidate 9c03cc59300363ae5339fc970dded08ede563e98.
  • Fresh local core tests, documentation checks/build, and actual Cloudflare/Fastly/Spin WASM contract runs passed. The shared runtime test covers both middleware-created and preinstalled handles.
  • TS pins this exact commit without local overrides. All four TS adapter suites, CLI/parity, native/WASM lints, local integration/EC lifecycle, 899 Vitest tests, focused GPT diagnostics, and complete Next.js/WordPress browser suites passed.
  • Independent API/correctness/simplicity and joint consumer reviews found no remaining issues. The Axum regression demonstrates the original collector-reset failure and its fix. All 14 Trusted Server checks also passed at 2791ceb46908e6af04ccaae0c10b29d35336a9f4 before requesting review.

Coordinated delivery / merge gate

Trusted Server temporarily pins all six EdgeZero dependencies to 9c03cc59300363ae5339fc970dded08ede563e98 for integration and joint PR verification, with no local path override and one resolved EdgeZero core identity. That temporary commit pin is not the final delivery pin.

Trusted Server #1121 needs an actual approved EdgeZero release tag before merging to main. Replace all temporary pins with that release, regenerate and inspect its lockfile, and revalidate. Review readiness does not waive the release requirement. No merge or tag/release has been performed.

Checklist

  • Changes follow CLAUDE.md conventions
  • No Tokio or UUID dependencies added to core or adapter crates
  • Route params use {id} syntax where applicable; routing unchanged
  • Consumer types imported from edgezero_core, not directly from http
  • Store wiring unchanged (registry requirement not applicable)
  • New code has tests
  • No secrets or credentials committed
  • Independent EdgeZero reviews cleared

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • 🔧 TimingError not #[non_exhaustive]: inconsistent with EdgeError/KvError/ConfigStoreError, and adding it after the tag is breaking (request_timing.rs:17)
  • 🔧 Hoist t0 out of the mutex: makes elapsed() 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_bytes mirror 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/Spin tests/contract.rs
  • ⛏ RequestTimings lacks Debug; must_use message wording; with_excluded_paths replace 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 the compile_fail doctest)
  • feature check (fastly cloudflare spin): PASS
  • Spin wasm32-wasip2 check: PASS
  • GitHub: all 22 checks green, including Cloudflare/Fastly/Spin WASM tests

Comment thread crates/edgezero-core/src/request_timing.rs
Comment thread crates/edgezero-core/src/request_timing.rs Outdated
Comment thread crates/edgezero-core/src/request_timing.rs Outdated
Comment thread crates/edgezero-core/src/request_timing.rs
Comment thread crates/edgezero-core/src/request_timing.rs Outdated
Comment thread docs/guide/middleware.md Outdated
Comment thread docs/guide/middleware.md Outdated
Comment thread docs/guide/middleware.md Outdated
Comment thread docs/superpowers/plans/2026-09-28-request-timing.md Outdated
Comment thread docs/superpowers/specs/2026-09-28-request-timing-design.md Outdated

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.toml can't register the middleware: a const fn constructor fixes it (middleware.rs:86)
  • 🤔 State<RequestTimings<…>> returns a 500 that points at with_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_time example needs a web-time dependency (docs/guide/middleware.md:27)
  • ⛏ Assertions that can't fail (middleware.rs:360, plus four more listed there)
  • ⛏ TimingSnapshot lacks the D = () default (request_timing.rs:45)
  • 🌱 PhaseSpan has no finish/cancel (request_timing.rs:260)

Not attached to a diff line:

  • ⛏ configuration.md now contradicts the corrected middleware guide: docs/guide/configuration.md:49 still says "Manifest-driven middleware are applied in order before routes". This PR corrected the same claim in middleware.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 /health exclusion as a tower service outside RouterService (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 Service wrapper around RouterService would cover unmatched routes and give every adapter one place to finish timing.

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-wasip2 check: PASS
  • GitHub: 21/21 checks green

Comment thread crates/edgezero-core/src/middleware.rs
Comment thread docs/guide/middleware.md Outdated
Comment thread crates/edgezero-core/src/request_timing.rs
Comment thread crates/edgezero-core/src/request_timing.rs
Comment thread crates/edgezero-core/tests/support/request_timing.rs Outdated
Comment thread docs/guide/middleware.md Outdated
Comment thread docs/guide/middleware.md
Comment thread crates/edgezero-core/src/middleware.rs Outdated
Comment thread crates/edgezero-core/src/request_timing.rs Outdated
Comment thread crates/edgezero-core/src/request_timing.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃 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.

Comment thread crates/edgezero-core/tests/request_timing_consumer.rs
Keep core doctests in the workspace host suite alongside main's CLI test matrix.

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.yml and the edgezero-macros fixture. 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 uses edgezero_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 at trusted-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 to 878be472 or 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-wasip2 check: PASS
  • GitHub: 28/28 checks green, including Cloudflare (10 tests, headless Firefox), Fastly/Viceroy (9) and Spin/Wasmtime (14)

Comment thread docs/guide/middleware.md Outdated
remain application-owned. Unmatched routes bypass middleware, and
`RouterService::oneshot` renders handler errors outside the chain. Use adapter or
outer-service hooks for terminal/error and streaming measurements as needed.
Those outer layers must retain a clone of the handle they preinstall; attachment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Cloudflare and Spin have no adapter hook for this: this paragraph sends final, error and streaming measurement to "adapter or outer-service hooks" that preinstall a handle and keep a clone. Fastly has that hook: run_app_with_request_extensions, serve_app_with_request_extensions and lifecycle::run_custom (see adapters/fastly.md:140). Axum can wrap the router in a tower service. Cloudflare and Spin don't, and they're the two adapters this middleware is for:

  • run_app and dispatch_app take the platform request and convert it internally (edgezero-adapter-cloudflare/src/lib.rs:100,127, edgezero-adapter-spin/src/lib.rs:111,135). CloudflareService::dispatch and Spin's request::dispatch do the same.
  • dispatch_with_registries is pub(crate) in both crates (cloudflare/src/request.rs:298, spin/src/request.rs:183). Converting with into_core_request and dispatching by hand loses the manifest store bindings.

On those adapters, the outermost place available today is a middleware registered before RequestTimingMiddleware. I tested this one: it reuses a preinstalled handle without resetting its origin, and it marks headers ready on a 200 and on a handler error (400) before oneshot renders the error. 404/405 requests skip it.

struct TimingBoundary;

#[async_trait(?Send)]
impl Middleware for TimingBoundary {
    async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result<Response, EdgeError> {
        let existing = ctx.request().extensions().get::<Handle>().cloned();
        let timings = existing.unwrap_or_else(Handle::new);
        ctx.request_mut().extensions_mut().insert(timings.clone());
        let result = next.run(ctx).await;
        let _ = timings.mark_headers_ready();
        result
    }
}

Suggest naming the hook for each adapter here, and showing this pattern for Cloudflare and Spin with its limits: it doesn't cover unmatched routes or streamed bodies. A dispatch_app_with_request_extensions for Cloudflare and Spin, matching Fastly's, would close the gap. If it doesn't land in this PR, it belongs in the spec's deferred follow-ups.


- name: Core doctests
if: matrix.suite == 'workspace'
run: cargo test -p edgezero-core --doc

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏ This gate only exists in CI: scripts/run_tests.sh:36, .claude/commands/check-ci.md and the "CI Gates" list in CLAUDE.md all stop at cargo test --workspace --all-targets, which skips doctests. So a local "all gates pass" never runs the compile_fail test that keeps the snapshot payload from escaping. Suggest adding cargo test -p edgezero-core --doc to all three.

Scope: `edgezero-core` and coordinated Trusted Server adoption, shipped together.
Plan: [Implementation plan](../plans/2026-09-28-request-timing.md).

The initial candidate is [EdgeZero #389](https://github.com/stackpop/edgezero/pull/389)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏ These records have gone stale again:

  • Trusted Server PR: #1121 was closed on 2026-10-09 and superseded by #1261, which pins ab444946. This paragraph and plan lines 19-25 still describe #1121 at 2791ceb as the consumer.
  • Browser: spec lines 111 and 389 and plan line 299 say the browser harness is Chromium. CI runs the Cloudflare contract suite in headless Firefox (job log: "Running headless tests in Firefox", 10 passed). Chromium is what ran locally, so "headless browser (Firefox in CI)" would be accurate.
  • Evidence counts: the plan's "Review revision verification" table (lines 359-380) counts 8 Fastly and 9 Cloudflare contract tests, from before the merge of main. CI now runs 9 and 10.

3. Approve the EdgeZero merge/tag through its normal release process, then replace
temporary TS pins with that tag and perform final tag-backed validation.

Publication, merge, releases, GitHub replies and thread resolution require their

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏ Some process text remains: the earlier thread asked to drop the authorization narrative, and a few lines are still here:

  • Here (lines 478-479): "Publication, merge, releases, GitHub replies and thread resolution require their own authorization; this record does not grant it."
  • Line 94: "Explain this narrower scope in the review response…"
  • Plan lines 389-391 and 407-409: "…subject to separate authorization" and "Reply to Aram with evidence… Resolve review threads only with separate authorization."
  • Plan lines 361-365: a SHA256 of the inputs to an uncommitted tree, which no reader can reproduce.

These describe how the work was run, not the design, so they'll read as noise once this lands on main. Suggest deleting them.

…ression tests, synchronize local doctest gates, refresh verification records, and remove session-only narrative. Production behavior unchanged.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants