Add EdgeZero canary routing (rollout_pct flag + FNV-1a bucket assignment) (PR 19)#727
Add EdgeZero canary routing (rollout_pct flag + FNV-1a bucket assignment) (PR 19)#727prk-Jr wants to merge 24 commits into
Conversation
…outes_to_edgezero
When edgezero_enabled=true, reads edgezero_rollout_pct (0-100) from the trusted_server_config store. Routes each request to EdgeZero if fnv1a_bucket(client_ip) < rollout_pct, giving the ops team sticky per-user canary control without a deploy. Absent key defaults to 100 (full rollout, backward compatible with edgezero_enabled=true deployments that predate this PR). Invalid values and read errors default to 0 (all legacy, fail-safe).
Set to "0" so local dev and CI stay on the legacy path by default. Ops changes this in the production config store — no re-deploy required.
Documents the two config store keys, canary progression stages (1% → 10% → 50% → 100%), hold-point durations, pass/fail thresholds, and instant rollback procedure.
prk-Jr
changed the title
aram356
requested review from
ChristianPavilonis and
aram356
and removed request for
aram356
prk-Jr
changed the base branch from
feature/edgezero-pr18-phase5-verification
to
feature/edgezero-pr19-spin-adapter
…ro-pr19-cutover-canary
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
One low-severity docs/ops note: the canary runbook references routing log lines that are emitted at debug level, so production verification may require a debug/log-level deployment or equivalent observability setup.
The absent-key branch logged at warn on every request when edgezero_enabled=true but edgezero_rollout_pct was unset, flooding logs at production QPS while traffic flowed fine. Demote to debug; the setup procedure and migration runbook remain the operational safety net. The absent-key default stays 100 (backward compat) so an existing edgezero_enabled=true deployment is not silently rolled back.
For rollout_pct 0 (full rollback) and 100 (full cutover) — which together cover most of the canary's lifetime — the routing decision is fixed, yet every request still allocated the client-IP routing key string and ran the FNV hash. Match on the rollout value and only compute the bucket for a partial rollout. Also rename canary_routes_to_edgezero to routes_to_edgezero: the predicate is just `bucket < rollout_pct` and applies at any rollout value, so "canary" read misleadingly at the call site.
The four documented branches (absent -> 100, valid -> parsed, invalid -> 0, read error -> 0) are safety-critical but only the parse step was indirectly covered. ConfigStoreHandle::new(Arc::new(...)) over the public ConfigStore trait is constructible in unit tests, so add an in-memory stub store and pin every branch, including the fail-safe defaults that matter during an incident.
The routing verification log lines are emitted at log::debug!, but the Fastly
logger is capped at Info in production, so verifying the canary via log tailing
needs a debug-level deployment or another observability signal (the runbook now
says so and points at the real-time stats split as the alternative).
Also reconcile the documented log format with the adapter: at the degenerate
rollout values (0 and 100) the bucket is no longer computed, so those lines read
`routing request through {legacy,EdgeZero} path (rollout_pct=N)` without a bucket.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #727 against feature/edgezero-pr19-spin-adapter, focusing on the Fastly entry-point canary routing, config-store failure behavior, rollout docs, and CI/review context. I did not find any blocking correctness, security, data-loss, authorization, or severe compatibility issues. I left one P2 inline comment about strengthening coverage for the safety-critical entry-point dispatch matrix.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
1 inline comment submitted.
P3 / Low
None.
CI / Existing Reviews
All reported PR checks are passing. Existing inline feedback about absent-key logging, config-store branch tests, degenerate rollout hashing, naming, and debug-level log documentation appears to have been addressed in later commits. Local cargo test-fastly rollout -- --nocapture could not run in this environment because Viceroy failed to satisfy fastly_http_downstream::downstream_bot_analyzed; CI's Fastly job is green.
Pull the rollout-percentage routing matrix out of main() into a pure should_route_to_edgezero(rollout_pct, client_ip) helper so the safety-critical wiring from rollout state to the EdgeZero/legacy choice can be unit-tested directly, not just the parsing/hashing helpers it composes. main() keeps the edgezero_enabled gate and config reads as thin I/O glue. Add tests covering the full dispatch matrix: rollout_pct=0 (always legacy), rollout_pct=100 (always EdgeZero), and partial-rollout bucket hit/miss for both present and absent client IPs.
…er' into feature/edgezero-pr19-cutover-canary
The base branch refactored IpAddr out of main.rs and dropped the import. Merging base in lost the import that should_route_to_edgezero and its tests rely on, breaking the wasm32-wasip1 build. Re-add it.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #727 against feature/edgezero-pr19-spin-adapter, focusing on the Fastly canary dispatch, rollout percentage parsing/defaults, config-store failure behavior, tests, local Fastly config, and the migration runbook. I did not find any blocking correctness, security, data-loss, authorization, or severe compatibility issues. I left one P2 inline comment about making the production canary observability path explicit before operators rely on the runbook.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
1 inline comment submitted.
P3 / Low
None.
CI / Existing Reviews
All reported PR checks are passing. Existing review feedback about absent-key log volume, degenerate rollout hashing, config-store branch coverage, naming, and debug-level log documentation appears addressed by later commits. Local cargo test-fastly rollout -- --nocapture compiled the changed Fastly test binary but could not execute under the installed Viceroy because it lacks fastly_http_downstream::downstream_bot_analyzed; CI's Fastly test job is green.
The runbook told operators to verify the canary via a Fastly real-time-stats traffic split and by log-searching the `routing request through EdgeZero path` lines, but neither signal is usable in production: the route decision is logged only at `debug!` (`should_route_to_edgezero`) and the Fastly logger is pinned to `Info` (`logging::init_logger`, no runtime override), so those lines never reach the production log endpoint, and no real-time-stats split or `x-edgezero-path` marker exists. State plainly that no production per-branch route signal exists yet, base canary verification on aggregate service metrics tracking the staged `rollout_pct` changes, and scope the debug route-decision log lines to local Viceroy validation. Remove the unsupported real-time-stats fallback and the unachievable "debug-level logging deployment" guidance.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #727 against feature/edgezero-pr19-spin-adapter, focusing on the Fastly canary dispatch, rollout percentage parsing/defaults, config-store failure behavior, local Fastly config, tests, and the EdgeZero migration runbook. I did not find any blocking correctness, security, data-loss, authorization, or severe compatibility issues. I left one P2 inline comment about a runbook validation signal that is not actually emitted with the current logger configuration.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
1 inline comment submitted.
P3 / Low
None.
CI / Existing Reviews
All reported PR checks are passing. Existing review feedback about absent-key logging, degenerate rollout hashing, config-store branch coverage, dispatch-matrix tests, naming, and production route observability appears addressed in later commits. Local cargo test-fastly rollout -- --nocapture compiled the Fastly test binary but could not execute under the installed Viceroy 0.16.4 because it lacks fastly_http_downstream::downstream_bot_analyzed; CI's Fastly job is green.
…idation The migration runbook pointed operators at the should_route_to_edgezero route-decision lines as a local Viceroy validation signal, but the Fastly logger was pinned to Info — gating echo_stdout too — so those debug! lines were emitted nowhere, even under Viceroy. Add an EDGEZERO_LOG_LEVEL env override (default Info) so local Viceroy runs can raise the level to Debug and surface the route-decision lines. Production Fastly Compute does not surface arbitrary process env vars, so the level stays at the safe Info default there. Resolution is a pure, unit-tested helper. Reword the three runbook references to state the override explicitly instead of implying the lines are visible by default.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #727 against feature/edgezero-pr19-spin-adapter, focusing on the Fastly canary routing path, rollout percentage parsing/fallbacks, logger-level changes, tests, local config, and EdgeZero migration documentation. I did not find any blocking correctness, security, data-loss, authorization, or severe compatibility issues. I left one P2 inline comment about stale operational guidance duplicated in the implementation plan.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
1 inline comment submitted.
P3 / Low
None.
CI / Existing Reviews
All reported PR checks are passing. Existing automated-review feedback about absent-key log volume, config-store branch coverage, degenerate rollout hashing, dispatch-matrix tests, production route observability, and local Viceroy debug logs appears addressed in later commits. Local cargo test -p trusted-server-adapter-fastly --target wasm32-wasip1 rollout_pct --no-run passed; a native cargo test -p trusted-server-adapter-fastly rollout_pct -- --nocapture attempt failed at link time on Fastly hostcall symbols, which is expected for the Fastly adapter outside the wasm/Viceroy path and not counted as a PR finding.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of the canary routing changes against the PR base (feature/edgezero-pr19-spin-adapter). The prior review's findings on the per-request warn flood, degenerate-rollout short-circuit, predicate rename, read_rollout_pct test coverage, and debug-log observability are all resolved. Two findings remain open (both carried over) and three new minor items are noted inline. CI is green (12/12).
Blocking
🔧 wrench
- Fail-open default for absent
edgezero_rollout_pct: absent key →100(full cutover) is the only failure branch that does not fail to legacy. Recommend defaulting to0. (main.rsabsent branch + doc table; runbook failure-modes table) - Runbook overstates stickiness: "sticky per user" should read "sticky per client IP" — NAT sharing and IP changes break the per-user assumption. (
docs/internal/EDGEZERO_MIGRATION.md:19)
Non-blocking
🤔 thinking
- Two config-store reads per request —
is_edgezero_enabledthenread_rollout_pct; consolidatable into one read. (main.rs:316)
⛏ nitpick
resolve_max_leveldoc says "raises" but the override can also lower the level. (logging.rs:44)
📝 note
- Env-var override safety rests on a Fastly-platform property — must not be copied verbatim into the axum/spin/cloudflare adapters. (
logging.rs:25)
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
Address PR #727 review findings: - Fail-safe default (blocking): an absent `edgezero_rollout_pct` now resolves to 0 (all legacy), matching every other failure branch (unreadable flag, invalid value, read error). Previously absent meant 100 (full cutover) — the only fail-open branch, where deleting the key triggered an instant full rollout. Updates the read_rollout_pct doc table and test, the runbook failure-modes table, and removes the now-unnecessary setup-ordering precondition and do-not-delete warning. - Runbook stickiness wording (blocking): "sticky per user" -> "sticky per client IP", noting NAT sharing and IP changes can re-bucket a user mid-rollout. - logging doc: the EDGEZERO_LOG_LEVEL override "overrides" the level (it can lower as well as raise it), not just "raises" it. - logging doc: warn that the EDGEZERO_LOG_LEVEL knob is Fastly-only and must not be copied into the axum/spin/cloudflare adapters, where runtime env vars are readable and it would reintroduce a per-request prod debug flood. - Cutover plan: replace the stale monitoring guidance (Fastly log search for route-decision lines) with aggregate-metrics-only verification, mirroring the runbook — production has no per-branch route attribution.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #727 against feature/edgezero-pr19-spin-adapter, focusing on the Fastly canary routing implementation, rollout defaults, config-store failure behavior, local Fastly config, migration runbook, implementation plan docs, and CI/review context. I did not find any blocking correctness, security, data-loss, authorization, or severe compatibility issues. I left two P2 inline comments about stale operational guidance that still contradicts the current fail-safe absent-key behavior.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
2 inline comments submitted.
P3 / Low
None.
CI / Existing Reviews
All reported PR checks are passing. Existing review feedback about fail-open absent-key behavior, sticky-per-user wording, route-decision observability, logging override wording/scope, and stale monitoring guidance appears addressed in code/runbook and replies; however, some duplicated guidance in fastly.toml and the implementation plan remains stale. Locally, cargo test -p trusted-server-adapter-fastly --target wasm32-wasip1 read_rollout_pct --no-run completed successfully.
Address PR #727 follow-up review: the shipped Viceroy config and the implementation plan still documented the old fail-open semantics. - fastly.toml: absent edgezero_rollout_pct now documented as failing safe to legacy (0); removed the obsolete "set 0 before enabling" precondition. - Cutover plan doc: update the background flow, the read_rollout_pct doc-table snippet, the rustdoc summary, the embedded fastly.toml snippet, the embedded runbook (config table, failure-modes table, do-not-delete warning, setup-order precondition), the sticky-routing note, and the invariants list to match the implemented behavior (absent/invalid/unreadable all fail safe to 0; stickiness is per client IP). Added a pointer to docs/internal/EDGEZERO_MIGRATION.md as the canonical runbook so the duplicated copy cannot silently drift again.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #727 against feature/edgezero-pr19-spin-adapter, focusing on the Fastly canary-routing code, fail-safe rollout defaults, logging changes, local config, migration runbook, implementation-plan documentation, CI, and existing review feedback. I did not find any blocking correctness, security, data-loss, authorization, or severe compatibility issues. I left two P2 inline comments on stale plan text that still contradicts the implemented fail-safe and per-client-IP semantics.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
2 inline comments submitted.
P3 / Low
None.
CI / Existing Reviews
All reported PR checks are passing. Existing feedback about absent-key fail-open behavior, route-decision observability, logging override wording/scope, and stale monitoring guidance appears addressed in the runtime code, fastly.toml, and the canonical migration runbook; the remaining issues are duplicated/stale text in the implementation-plan document. Locally, cargo test -p trusted-server-adapter-fastly --target wasm32-wasip1 read_rollout_pct --no-run and cargo fmt --all -- --check passed; cd docs && npm run format -- --check could not run because prettier is not installed in this environment, while CI's docs format check is green.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review Summary
🔧 Requesting changes. The runtime fail-safe rollout behavior is much improved and CI is green, but local canary validation still relies on a log-level mechanism that Viceroy/Compute guest code will not actually see. I also left accept-ready suggestions for stale plan snippets and validation commands so the PR artifacts stay safe to reuse.
Additional note folded into the body because the exact insertion point is unchanged context in the diff: max_level is currently applied only to the inner log_fastly::Logger. Please also set the same level on the outer fern::Dispatch (e.g. fern::Dispatch::new().level(max_level)) so production debug! route decisions are filtered before timestamp/format construction, not just dropped by the child logger.
…er' into feature/edgezero-pr19-cutover-canary
EDGEZERO_LOG_LEVEL is not propagated into the Compute guest by fastly compute serve, so the documented local route-decision validation path never raised the level. Detect the Viceroy environment through the guest-visible FASTLY_HOSTNAME=localhost signal and auto-raise to debug there, keeping production at the safe Info default. The explicit env override is retained for harnesses that populate the guest environment.
max_level was applied only to the inner log_fastly Logger, so the outer fern Dispatch still ran timestamp and format construction for records the child logger later dropped. Set the same level on the Dispatch so below-threshold records are filtered before formatting at production QPS.
Update the migration runbook and implementation plan to describe local route-decision logging via the guest-visible FASTLY_HOSTNAME=localhost signal instead of the EDGEZERO_LOG_LEVEL env var, which the Compute guest never sees. Replace the stale Ok(None) => 100 plan snippet with the fail-safe absent-key arm, correct the stickiness invariant to per-client-IP (NAT and IP changes can re-bucket), switch verification commands to the cargo clippy-fastly alias, and drop the tail/head pipes that masked the real exit status of test, fmt, clippy, build, and docs-format checks.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #727 against feature/edgezero-pr19-spin-adapter, focusing on the Fastly canary routing implementation, rollout defaults/failure behavior, logging-level changes, local config, migration runbook, duplicated implementation-plan guidance, CI, and existing review feedback. I did not find any blocking correctness, security, data-loss, authorization, or severe compatibility issues. I left one P2 inline comment on stale plan guidance that now contradicts the implemented tests/helper names and could mislead future agent handoffs.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
1 inline comment submitted.
P3 / Low
None.
CI / Existing Reviews
All reported PR checks are passing. Existing review feedback about absent-key fail-safe behavior, per-client-IP stickiness, route-decision observability, logging override scope, stale fail-open snippets, and validation command exit-status masking appears addressed in the runtime code, fastly.toml, the canonical migration runbook, and portions of the implementation-plan document. Locally, cargo test -p trusted-server-adapter-fastly --target wasm32-wasip1 rollout_pct --no-run completed successfully.
| 4. Compute `bucket = fnv1a_bucket(client_ip_string)` | ||
| 5. If `bucket < rollout_pct` → `edgezero_main`, else → `legacy_main` | ||
|
|
||
| `ConfigStoreHandle` comes from `edgezero_core::config_store` and cannot be constructed in unit tests (needs Fastly runtime). Test only pure functions. |
There was a problem hiding this comment.
Automated review: P2: The implementation plan still says the config-store rollout helper cannot be unit-tested.
This is now stale and contradicts the PR's final code: read_rollout_pct is unit-tested with an in-memory ConfigStoreHandle stub in crates/trusted-server-adapter-fastly/src/main.rs, and the helper was also renamed from canary_routes_to_edgezero to routes_to_edgezero. Because this file is explicitly framed as an agentic-worker handoff, leaving the old constraint in place can cause future follow-up work to skip the safety-critical config-store branch tests or copy the outdated helper names.
Suggested fix: either mark this plan as a historical snapshot that must not be reused for implementation, or sync the affected sections with the final PR: remove the claim that ConfigStoreHandle cannot be constructed in unit tests, mention the in-memory ConfigStore stub coverage for read_rollout_pct, and update the old canary_routes_to_edgezero references to routes_to_edgezero.
3 participants
Summary
edgezero_rollout_pct(0–100) to thetrusted_server_configstore so EdgeZero traffic can be shifted incrementally without a deployclient_ipfor deterministic, sticky per-user bucket assignment (0–99); routing predicate isbucket < rollout_pctrollout_pct = 0is the immediate no-deploy rollbackChanges
crates/trusted-server-adapter-fastly/src/main.rsparse_rollout_pct,fnv1a_bucket,canary_routes_to_edgezero,read_rollout_pct; updatedmain()with canary dispatch; 11 unit tests (pinned FNV-1a vectors, distribution, boundary routing)fastly.tomledgezero_rollout_pct = "0"to local Viceroy config store with ops commentdocs/internal/EDGEZERO_MIGRATION.mdCloses
Closes #500
Test plan
cargo test-fastly— 65 + 956 + 21 + 3 tests greencargo test-axum— 31 tests greencargo clippy-fastly && cargo clippy-axum— no warningscargo fmt --all -- --check— cleancd crates/js/lib && npx vitest run— 291 tests greencd crates/js/lib && npm run format— cleancd docs && npm run format— cleancargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servewithedgezero_rollout_pct = "1"and"0"(rollback)Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)