Instrument KV operation timing logs#270
Conversation
aram356
left a comment
There was a problem hiding this comment.
PR Review — Instrument KV operation timing logs
Summary
Adds debug-level backend-timing instrumentation to every KvHandle operation, with a zero-overhead-when-disabled design (Instant::now() and the metadata closure are gated behind log_enabled!(Debug)) and safe metadata that logs only lengths/counts/hit-miss/ttl — never raw keys, values, prefixes, or cursors. The implementation is correct and all CI gates pass; the requested changes are about a stated-but-untested security guarantee and structured-log correctness.
Note: this PR is branched from
main's older layout ofkey_value_store.rs; the in-flightfeature/extensible-clibranch has independently refactored the same file (alphabetized methods,#[inline]), so expect merge conflicts at integration time.
Findings
Blocking
- 🔧 Untested security guarantee (
docs/guide/kv.md:95): the "never logged" promise has no test enforcing it. Add a capturing-logger regression test asserting raw keys/values never appear in output. - 🔧 Mixed-type metadata fields on error (
crates/edgezero-core/src/key_value_store.rs:436,:595,:640): numeric/boolean fields are emitted as the stringunknownon error paths, breaking logfmt/metrics ingestion. Omit them on error sincestatus=erroralready signals failure.
Non-blocking
- ♻️ DRY (
key_value_store.rs:460,:546):get/get_bytesinline the same read-metadata format string; extract akv_read_metadatahelper to mirrorkv_write_metadata. - 🤔 Cloudflare timer clamping (
docs/guide/kv.md:93):performance.now()advances only across I/O and at reduced resolution on Workers; document thatelapsed_msis approximate there. - 📝
read_modify_writeemits two logs (key_value_store.rs:526): RMW delegates toget+put, producingoperation=getandoperation=putlines (noread_modify_writeline). Documented already — noted for operators reading logs.
CI Status (run in an isolated worktree at the PR head)
- fmt: PASS
- clippy (
-p edgezero-core --all-targets --all-features -D warnings): PASS - tests (
cargo test -p edgezero-core): PASS — 320 passed, 0 failed
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Reviewed PR #270 at head 73a718b. The timing instrumentation is scoped to backend KV calls, uses web_time::Instant for WASM compatibility, and keeps log metadata derived rather than raw.
😃 Praise
- The observability docs clearly call out the important boundaries: safe derived metadata only, the Cloudflare timing caveat, typed helper timing scope, and separate
read_modify_writebackend calls. - The metadata helpers consistently avoid raw keys, prefixes, cursors, and values, with regression coverage for sensitive key/value strings.
Non-blocking
- 🤔 Timing emission coverage (
crates/edgezero-core/src/key_value_store.rs:408): the new tests cover metadata formatting, but not actual public-method log emission. A focused recording-logger test for representative success/error paths could assertoperation=...,status=...,elapsed_ms=..., and absence of raw key/value/cursor data.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS
- all-feature check: PASS
- Spin WASM check: PASS
Summary
KvHandlebackend operations across adapters.Related external tracking: IABTechLab/trusted-server#731
Changes
crates/edgezero-core/src/key_value_store.rsKvHandlebackend calls withweb_time::Instantand emits guardedlog::debug!metadata for get/put/TTL/raw/list/delete/exists operations.docs/guide/kv.mdread_modify_writebehavior.Closes
Closes #271
Test plan
cargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo check --workspace --all-targets --features "fastly cloudflare spin"wasm32-wasip1Spin viacargo check -p edgezero-adapter-spin --target wasm32-wasip1 --features spinedgezero-cli devcargo test -p edgezero-core;cargo test -p edgezero-adapter-axum --all-targets;cargo fmt --all -- --checkChecklist
{id}syntax (not:id)edgezero_core(nothttpcrate)