Skip to content

test: add proxy burn-in and CRUD benchmark - #455

Open
freshtonic wants to merge 1 commit into
mainfrom
feat/proxy-burn-in
Open

test: add proxy burn-in and CRUD benchmark#455
freshtonic wants to merge 1 commit into
mainfrom
feat/proxy-burn-in

Conversation

@freshtonic

Copy link
Copy Markdown
Contributor

Summary

  • add a dedicated cipherstash-proxy-burn-in workspace package with deterministic conformance coverage
  • add a timed soak workload with --duration-seconds, concurrent CRUD, release-profile Proxy build/launch, and one-second RSS sampling
  • copy pg-proto's type-lab and commerce schema/seed migrations
  • add a Criterion cargo bench target that runs realistic transactional CRUD against the same schema
  • document setup, connection overrides, reports, and benchmark usage

Verification

  • cargo fmt --all -- --check
  • RUSTC_WRAPPER= cargo check --workspace --all-targets
  • RUSTC_WRAPPER= cargo test -p cipherstash-proxy-burn-in
  • RUSTC_WRAPPER= cargo clippy -p cipherstash-proxy-burn-in --all-targets -- -D warnings
  • RUSTC_WRAPPER= cargo bench -p cipherstash-proxy --bench proxy_crud --no-run

Live conformance, soak, and measured benchmark execution require the test PostgreSQL service and CipherStash credentials; no test containers were running in the local verification environment.

Signed-off-by: James Sadler <james@cipherstash.com>
@tobyhede

Copy link
Copy Markdown
Contributor

Proxy already has benchmark setup that might be worth extending if it doesn't do what you need

tests/benchmark/ (in main, CI-wired via mise run benchmark/benchmark:continuous) — pgbench-driven, black-box, comparative: Proxy vs raw Postgres vs pgbouncer vs pgcat, plaintext vs encrypted.
Plots results to PNG/CSV, CI handles as regression
Protocol-level throughput/latency comparison.

@tobyhede

Copy link
Copy Markdown
Contributor

Review: Standards & Spec (vs origin/main, single commit 179b9dc)

Two-axis review — does the code follow the repo's documented standards, and does it match what the PR description asked for — with every finding independently re-verified against the actual code before reporting.

Worth fixing

1. benches/proxy_crud.rs reimplements what cipherstash-proxy-burn-in already provides

proxy_crud.rs:6-7 re-includes the migration SQL via a relative path:

const SCHEMA: &str = include_str!("../../cipherstash-proxy-burn-in/migrations/0001_schema.sql");
const SEED: &str = include_str!("../../cipherstash-proxy-burn-in/migrations/0002_seed.sql");

but cipherstash-proxy-burn-in::lib already exports these as pub const SCHEMA_MIGRATION / SEED_MIGRATION. Same story for connect() (near-duplicate of database::connect), the CRUD shape (realistic_crud mirrors soak::crud_cycle's insert → read → update → cascading-delete skeleton), and the connection-string defaults (duplicated verbatim between main.rs and proxy_crud.rs).

Adding cipherstash-proxy-burn-in as a dev-dependency of cipherstash-proxy would fix all of these at once — no dependency cycle results, and nearly all of burn-in's deps (clap, serde, tokio, tokio-postgres) are already direct deps of cipherstash-proxy, so it shouldn't meaningfully affect bench build time.

2. README doesn't document the credentials the release Proxy binary needs

The soak workload spawns a real release-profile Proxy binary that inherits the parent process's environment, but the README only says credentials "must already be available in the environment" — no variable names, no pointer to mise.local.toml. packages/showcase/README.md:438-445 sets a good precedent here (lists CS_WORKSPACE_CRN, CS_CLIENT_ACCESS_KEY, CS_DEFAULT_KEYSET_ID, CS_CLIENT_ID, CS_CLIENT_KEY explicitly) — worth matching that so a new contributor can actually get soak running from the README alone.

Minor, not blocking

  • conformance.rs repeats the literal 900_001_i32 nine times rather than binding it once (the repo's convention elsewhere — e.g. random_id() in the integration suite — binds once and reuses).

Checked and cleared (no action needed)

  • The RSS sampler's first tick landing at t≈0 rather than t≈1s is expected tokio::time::interval behavior, and it's actually useful here — it becomes initial_rss_bytes, a genuine baseline reading.
  • --max-rss-growth-mib isn't called out in the PR description's bullets, but it's a natural, opt-in extension for a burn-in/soak tool (the whole point is catching leaks) — not scope creep worth flagging.
  • The migrations' "copied from pg-proto" provenance checks out — pg-proto is a real repo by the same author, already a workspace dependency of this codebase.

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.

2 participants