feat: add k6 load-testing harness for API and keeper endpoints (#719) - #754
Conversation
|
@JohnArayaE is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@collinsezedike This one's ready for your review. |
| positions: { | ||
| executor: "constant-arrival-rate", | ||
| exec: "readPositions", | ||
| rate: Number(__ENV.RPS || 20), |
There was a problem hiding this comment.
@JohnArayaE the default RPS of 20 is 1,200 req/min, well above the API's actual limit of 100 req/60s per client IP (api/_lib/middleware.ts:9, keyed by clientIp(req)). Running this exactly as the README's example command shows exhausts the limit in about 5 seconds, then most of the 2-minute run returns 429. That fails the script's own check on line 47 (not rate limited under normal read load), and the recorded latency numbers end up measuring 429 rejection speed rather than the steady-state read path this script is meant to characterize. Please lower the default RPS or document that an -e RPS=... override is required.
| health: { | ||
| executor: "constant-arrival-rate", | ||
| exec: "health", | ||
| rate: Number(__ENV.HEALTH_RPS || 10), |
There was a problem hiding this comment.
@JohnArayaE same issue as positions.js: the default HEALTH_RPS of 10 is 600 req/min against the same 100 req/60s per-IP budget. A default run fails the "health: 200" check on line 81 for roughly two-thirds of requests after the first ~10 seconds. Please lower the default or document that the override is required.
| const rateLimited = new Rate("rate_limited_responses"); | ||
| const serverErrors = new Counter("server_errors_5xx"); | ||
|
|
||
| const VUS = Number(__ENV.VUS || 20); |
There was a problem hiding this comment.
@JohnArayaE default VUS of 20 with a 1s sleep per iteration is also around 1,200 req/min, well above the 100 req/60s per-IP limit. The README explains at length why HTTP 500 is the expected outcome at these defaults (simulation failing on unfunded accounts), but most requests never reach simulation, they're rejected as 429 first, so the default run doesn't actually exercise the path the README describes. Please lower the default VUS or document the override.
| accounts with testnet USDC via <https://testnet.blend.capital> (Blend's own | ||
| faucet; the default `fundFromBlendFaucet()` flow has not reliably granted | ||
| USDC in practice — see | ||
| [`testnet-deployment.md`](../../apps/docs/operations/testnet-deployment.md#getting-testnet-usdc`)), |
There was a problem hiding this comment.
@JohnArayaE there's a stray backtick inside this link's URL, right before the closing parenthesis on this line. It becomes part of the href/fragment, which then won't match the actual anchor on that page (getting-testnet-usdc), so the link won't jump to the right section. Removing that backtick should fix it.
| } | ||
|
|
||
| export function loadAccounts(openFn) { | ||
| const data = JSON.parse(openFn(ACCOUNTS_FILE)); |
There was a problem hiding this comment.
@JohnArayaE every script calls loadAccounts(open), and open only actually gets invoked here as openFn(ACCOUNTS_FILE), not as a literal open() call written directly in the top-level script. k6's own docs and issue grafana/k6#3020 describe open() as resolving relative to "the file it's called in", with acknowledged inconsistency around indirect calls like this. I don't have k6 installed to confirm, but if it resolves relative to this file, ACCOUNTS_FILE = "./accounts.json" would look in scripts/load-test/lib/accounts.json instead of scripts/load-test/accounts.json, where prepare-accounts.mjs actually writes it. Could you run this end to end locally and confirm the account file is actually found?
Lower default RPS/VUS so scripts stay under the API's rate limit out of the box, fix a broken link in README.md, and stop passing k6's open() by reference through config.js (verified locally with k6). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133y6DqJtinBxCEKQcRLNSw
…hnArayaE/meridian into feat/719-load-testing-harness
|
@collinsezedike This one's ready for your review. |
| // race outcome. | ||
| disabledOrSkipped.add(1); | ||
| } else { | ||
| claimedOrRan.add(1); |
There was a problem hiding this comment.
The concurrentInvocation probe misclassifies a config-error 500 from the default accrue action as a successful claim/run. In api/v1/keepers/[action].ts, alert and the migration action both check isAlertKeeperConfigured/isMigrationKeeperConfigured first and return {status: "disabled"} when the relevant secret key is unset, but accrue has no equivalent guard: it calls loadBlendAccrualKeeperConfig(process.env) directly inside a try/catch, which throws and returns a 500 with {error: ...}, no status field. Since KEEPER_ACTION defaults to accrue and the README documents MERIDIAN_KEEPER_SECRET_KEY as optional, a default run against a deployment without that key set will count every response as claimedOrRan instead of disabledOrSkipped, so the race probe silently tests nothing while looking like it passed. Please treat a 500 response the same way a {status: "disabled"} response is treated here, or otherwise detect this case, so the probe fails loudly instead of reporting a false claimed/ran count.
| maxDuration: "60s", | ||
| // Starts after the health scenario finishes so the two don't share | ||
| // the strict rate-limit budget. | ||
| startTime: "35s", |
There was a problem hiding this comment.
startTime: "35s" is hardcoded, but health's own duration is the DURATION env var, default 30s. Running with -e DURATION=60s makes the two scenarios overlap for 25 seconds, sharing the rate-limit budget this delay exists to avoid, which can push the race probe's requests into 429s instead of cleanly exercising the submission lease. Please derive startTime from DURATION (plus a small buffer) instead of hardcoding it.
Detect accrue's config-error 500 (no isConfigured guard, unlike alert/rebalance) so it's no longer counted as claimed/ran, and fail the check loudly instead of silently passing. Derive concurrentInvocation's startTime from DURATION instead of a hardcoded 35s. Verified locally with k6 against a mock server covering all three response shapes (unconfigured/ok/fail). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133y6DqJtinBxCEKQcRLNSw
|
@collinsezedike This one's ready for your review. |
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
@JohnArayaE Thank you for the contribution. Feel free to pick up another open issue whenever you're ready.
64c6d47 to
6aa64bb
Compare
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
@JohnArayaE Thank you for the contribution. Feel free to pick up another open issue whenever you're ready.
2 participants
Summary
scripts/load-test/for/v1/tx/deposit,/v1/tx/withdraw,/v1/positions, and/v1/keepers/*, plus a README on how to run it against a throwaway testnet deployment.rate-limit-fallback.jscharacterizes the in-memory rate-limit fallback's per-instance behavior under concurrent load (api/_lib/middleware.ts).keepers.jsprobes the keeper submission-lease race under concurrent invocations (packages/stellar-sdk-helpers/src/keeper-state.ts).prepare-accounts.mjsgenerates and Friendbot-funds a pool of throwaway testnet accounts for the scripts to use.Test plan
pnpm lint && pnpm typecheck && pnpm testpass locallyrate-limit-fallback.jsagainst a real preview deployment (Upstash unconfigured) to confirm it surfaces the per-instance behavior described in the issueCloses #719