feat: add nightly RPC benchmark harness - #3930
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3930 +/- ##
==========================================
- Coverage 75.77% 74.62% -1.15%
==========================================
Files 461 466 +5
Lines 40876 41659 +783
==========================================
+ Hits 30972 31089 +117
- Misses 7738 8386 +648
- Partials 2166 2184 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f75870f to
54fc41f
Compare
54fc41f to
6854c63
Compare
danielntmd
changed the title
6854c63 to
d222dcc
Compare
|
Claude finished @danielntmd's task in 8m 49s —— View job Review: nightly RPC benchmark harness
No Go changes here, so no consensus/sync/concurrency surface. I verified the plumbing this depends on: No blockers. My findings are about what the harness will measure, not whether it runs. Important
Nits
Nothing was changed on the branch — this is review feedback only, as requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| run_tracked_input "$CORPUS_PATH" k6 run --quiet \ | ||
| -e NODE_URL="$NODE_URL" \ | ||
| --vus 1 \ | ||
| --iterations "$ITERATIONS_VALUE" \ |
There was a problem hiding this comment.
important: the measured single scenario replays exactly the same corpus entries the warmup just fetched.
run.js indexes with corpus[exec.scenario.iterationInTest % corpus.length], and iterationInTest restarts at 0 for every k6 run. Warmup does 200 iterations (WARMUP_ITERATIONS=200, line 10) → corpus[0..199]; single defaults to ITERATIONS=200 → corpus[0..199] again, in the same order.
Consequences:
- sequential latency is measured entirely on entries whose state/receipt data was just pulled into the node's caches, so
singlereports best-case cache-hit latency rather than the mixed behaviourconcurrency/throughputsee; - 9,800 of the 10,000 committed corpus entries are never touched by
single, so the corpus size buys nothing for that scenario, and a regression that only affects cold reads is invisible.
If the warm-cache measurement is intentional, please say so explicitly in README.md — otherwise warm from a disjoint slice (e.g. pass an offset via -e CORPUS_OFFSET and index (offset + iterationInTest) % corpus.length, with warmup using offset 0 and single using offset WARMUP_ITERATIONS).
| echo $((value * multiplier)) | ||
| } | ||
|
|
||
| timeout_seconds=$(duration_seconds "$READY_TIMEOUT") || fail "READY_TIMEOUT must be a positive duration using s, m, or h" |
There was a problem hiding this comment.
important: CONCURRENCY_DURATION and THROUGHPUT_DURATION bypass the configuration stage entirely.
duration_seconds is only applied to READY_TIMEOUT; the two scenario durations are passed straight through to k6 (--duration "$CONCURRENCY_DURATION_VALUE" at line 475, -e DURATION= at line 486). A typo such as CONCURRENCY_DURATION=30 (k6 reads bare integers as nanoseconds) or 30x therefore:
- is not rejected at
configurationwith exit 2, but - surfaces much later as
fail "$scenario did not produce a valid summary"withfailure.stage = "concurrency"and exit 1 — i.e. after waiting up toREADY_TIMEOUT(30 m default) and running the whole warmup, and reported as a measurement failure rather than a misconfiguration.
The helper already exists, so this is a two-line fix:
duration_seconds "$CONCURRENCY_DURATION_VALUE" >/dev/null || fail "CONCURRENCY_DURATION must be a positive duration using s, m, or h"
duration_seconds "$THROUGHPUT_DURATION_VALUE" >/dev/null || fail "THROUGHPUT_DURATION must be a positive duration using s, m, or h"Worth adding cases to test-runner.sh too — it covers READY_TIMEOUT=30x but neither scenario duration.
| case "$actual_block_number" in | ||
| ''|*[!0-9]*) fail "node returned an invalid block number: $actual_block_number" ;; | ||
| esac | ||
| if [ "$actual_block_number" != "$EXPECTED_BLOCK_NUMBER" ]; then |
There was a problem hiding this comment.
nit: string comparison of two values you've already proven to be decimal digits. EXPECTED_BLOCK_NUMBER=0800000 passes the digit check at line 338 and then spuriously fails here against 800000. Since both sides are validated as digits-only, use arithmetic comparison:
| if [ "$actual_block_number" != "$EXPECTED_BLOCK_NUMBER" ]; then | |
| if [ "$actual_block_number" -ne "$EXPECTED_BLOCK_NUMBER" ]; then |
| fi | ||
| readonly JUNO_COMMIT | ||
|
|
||
| required_env=" |
There was a problem hiding this comment.
nit (two related points on this pre-flight block):
-
No manifest is written for pre-flight failures.
mkdir -p "$RESULTS_DIR"/trap on_exit EXITonly happen at lines 261/281, after this loop and after theJUNO_COMMITchecks. So the single most likely operational failure — a deployment forgetting one of these eight variables — produces exit 2, a line on stderr, and nomanifest.jsonat all, while the README states the manifest records failures. Consumers need a separate "no manifest" code path. SinceRESULTS_DIRis defaulted at line 44, movingmkdir -p/trapabove this loop would let pre-flight failures land in the manifest too (stage: "preflight", whichcurrent_stageis already initialised to). -
SNAPSHOT_ID,SNAPSHOT_SHA256,JUNO_IMAGE_DIGESTandRUNNER_IMAGE_DIGESTare required but never validated or verified — they're only echoed into the manifest as provenance.test-runner.shpassesJUNO_IMAGE_DIGEST=sha256:juno, which is exactly the shape of a typo that would silently poison the provenance record for a whole run of nightly results. A cheap format check at theconfigurationstage would catch it:
printf '%s' "$SNAPSHOT_SHA256" | grep -Eq '^[0-9a-f]{64}$' || fail "SNAPSHOT_SHA256 must be a 64-character hex digest"
for digest_name in JUNO_IMAGE_DIGEST RUNNER_IMAGE_DIGEST; do
eval "digest=\$$digest_name"
printf '%s' "$digest" | grep -Eq '^sha256:[0-9a-f]{64}$' || fail "$digest_name must be sha256:<64 hex>"
doneGiven the whole point of the harness is reproducibility, unverifiable provenance fields are the weakest link in the manifest.
| { | ||
| failedChecks: metric_value("checks"; "fails"), | ||
| requestFailures: metric_value("rpc_request_failures"; "count"), | ||
| httpRequestFailures: metric_value("http_req_failed"; "passes"), | ||
| vuFailures: metric_value("vu_failures"; "count"), | ||
| droppedIterations: metric_value("dropped_iterations"; "count"), | ||
| completedIterations: metric_value("iterations"; "count") | ||
| } |
There was a problem hiding this comment.
nit: the extracted metric set is failure counters only — no latency. manifest.json therefore records the full configuration and provenance of a performance run but none of its performance, so anything trending nightly results has to open single.json/concurrency.json/throughput.json and re-derive the percentiles that --summary-trend-stats 'avg,min,med,p(90),p(99),max' was passed to produce. Adding them here is a few lines and makes the manifest self-contained:
httpReqDuration: {
avg: metric_value("http_req_duration"; "avg"),
med: metric_value("http_req_duration"; "med"),
p90: metric_value("http_req_duration"; "p(90)"),
p99: metric_value("http_req_duration"; "p(99)"),
max: metric_value("http_req_duration"; "max")
},(the existing metric_value guard already handles absent metrics by returning 0, and trend values are floats, which is fine here since nothing shell-side does arithmetic on them).
Two smaller observations while here:
httpRequestFailures: metric_value("http_req_failed"; "passes")is correct for k6's Rate metric but reads backwards —passescounts failed requests. A one-line comment would save the next reader a trip to the k6 docs.httpRequestFailuresis the only extracted field the runner never checks;failedChecks/requestFailuresalready catch connection errors viaisSuccess, so it's pure record-keeping — worth a comment saying so, otherwise it looks like a missed assertion.
Review: nightly RPC benchmark harness (Go runner)The shell runner has been replaced with Go since my last pass, so I'm re-reviewing from scratch.
View job run · branch |
| if c.readyTimeout, err = parseRunnerDuration(c.readyTimeoutRaw); err != nil { | ||
| return fmt.Errorf("READY_TIMEOUT must be a positive duration using s, m, or h") | ||
| } | ||
| c.readyPollInterval, _ = parseRunnerDuration(c.readyPollIntervalRaw) |
There was a problem hiding this comment.
important: the READY_POLL_INTERVAL parse error is discarded here, which moves a configuration error out of the configuration stage — and sometimes drops it entirely.
parseRunnerDuration returns 0 on failure, so READY_POLL_INTERVAL=30x:
- is not rejected by
validate()(exit 2,failure.stage = "configuration"), and - surfaces in
runReadiness(runner.go:350) asfail("READY_POLL_INTERVAL must be a positive duration using s, m, or h")with exit 1 andfailure.stage = "readiness"— i.e. the wrong exit code and the wrong stage for a consumer branching on that contract; - and if the node answers
/ready/rpcon the first probe, the loopbreaks before ever reaching line 350, so the misconfiguration is silently accepted for the whole run.
Every other knob (ITERATIONS, VUS, THROUGHPUT_VUS, RATES, EXPECTED_BLOCK_NUMBER, READY_TIMEOUT) is validated up front; this is the one that isn't. Suggest treating it like READY_TIMEOUT and deleting the runtime check:
| c.readyPollInterval, _ = parseRunnerDuration(c.readyPollIntervalRaw) | |
| if c.readyPollInterval, err = parseRunnerDuration(c.readyPollIntervalRaw); err != nil { | |
| return fmt.Errorf("READY_POLL_INTERVAL must be a positive duration using s, m, or h") | |
| } |
test-runner.sh covers READY_TIMEOUT=30x but not this one, and TestConfigValidationFailures has no case for it either.
| if !digitsPattern.MatchString(r.actualBlockNumber) { | ||
| return r.fail("node returned an invalid block number: " + r.actualBlockNumber) | ||
| } | ||
| if r.actualBlockNumber != r.config.expectedBlockNumber { |
There was a problem hiding this comment.
important: string comparison of two values that have already been proven to be decimal digits, which can produce a self-contradictory manifest.
EXPECTED_BLOCK_NUMBER=0800000 passes digitsPattern at config.go:157 and parses cleanly to expectedBlock = 800000. Against a node at block 800000 this branch then fails the run with block number mismatch: expected 0800000, got 800000, while manifest.json records node.expectedBlockNumber: 800000 and node.blockNumber: 800000 — a failure manifest whose own two numbers are equal.
config.expectedBlock is already parsed for exactly this, so compare numerically (actualBlockNumber is guaranteed digits-only by the check two lines above):
| if r.actualBlockNumber != r.config.expectedBlockNumber { | |
| actualBlock, err := strconv.ParseUint(r.actualBlockNumber, 10, 64) | |
| if err != nil { | |
| return r.fail("node returned an invalid block number: " + r.actualBlockNumber) | |
| } | |
| if actualBlock != r.config.expectedBlock { |
(and drop the now-redundant digitsPattern guard above, or keep it — ParseUint also rejects a negative/overflowing value that the regex would let through only as +Inf-style garbage).
| exitCode, commandErr := r.runK6(args, r.stdout) | ||
| metrics, err := promoteScenarioSummary(temporary, result) | ||
| if err != nil { | ||
| return r.fail(name + " did not produce a valid summary") | ||
| } |
There was a problem hiding this comment.
important: commandErr is discarded on the path where it carries the most information.
If k6 can't start at all — binary missing from PATH, corpus unreadable, --duration rejected as a bad config, OOM-kill — runK6 returns a meaningful error and no summary is written, so promoteScenarioSummary fails and the run reports "concurrency did not produce a valid summary". The actual cause (exec: "k6": executable file not found in $PATH, or k6's own config error) is dropped on the floor and never reaches manifest.json, which is the only artifact a nightly job leaves behind. k6's stderr does go to r.stderr, but the manifest — the thing the deployment parses — records only the generic string.
Cheap fix: fold it into the reason, e.g.
metrics, err := promoteScenarioSummary(temporary, result)
if err != nil {
if commandErr != nil {
return r.fail(fmt.Sprintf("%s did not produce a valid summary (exit %d): %v", name, exitCode, commandErr))
}
return r.fail(fmt.Sprintf("%s did not produce a valid summary: %v", name, err))
}Two related spots:
runWarmup(runner.go:468-471) foldscommandErr != nil || exitCode != 0intofail("warmup recorded check, request, or VU failures"), so a k6 exit that had nothing to do with checks is reported as a check failure.runScenarioalready separates these two cases (lines 501 and 504) — worth mirroring that here.validateTarget(runner.go:405,414,422) collapses transport errors, non-2xx, JSON-RPC errors and unmarshal errors into"could not read Juno version"/"could not read chain ID"/"could not read block number". Connection-refused vs. a malformed response are very different operational signals for a nightly run;%w-wrapping the underlying error into the reason costs nothing.
| values := make([]float64, len(keysForSummaryMetrics())) | ||
| keys := keysForSummaryMetrics() | ||
| for i, key := range keys { | ||
| values[i], err = metric(key[0], key[1]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| return &summaryMetrics{ | ||
| FailedChecks: values[0], RequestFailures: values[1], | ||
| HTTPRequestFailures: values[2], VUFailures: values[3], | ||
| DroppedIterations: values[4], CompletedIterations: values[5], | ||
| }, nil |
There was a problem hiding this comment.
nit: the struct is built by positional index into a slice whose order lives in a different function, so reordering keysForSummaryMetrics silently relabels every metric — FailedChecks would start reporting dropped_iterations and the run would keep passing. Nothing in the compiler or the tests catches that (TestParseSummaryMetricsSupportsBothK6Shapes uses distinct values 2/3/4/5/6, so it would catch a swap — but only if someone remembers to keep those distinct, and dropped_iterations is 0 there, so a swap involving it is invisible).
Named locals remove the coupling entirely:
get := func(name, key string) float64 { ... } // with the error captured once
return &summaryMetrics{
FailedChecks: get("checks", "fails"),
RequestFailures: get("rpc_request_failures", metricCountKey),
HTTPRequestFailures: get("http_req_failed", "passes"),
VUFailures: get("vu_failures", metricCountKey),
DroppedIterations: get("dropped_iterations", metricCountKey),
CompletedIterations: get("iterations", metricCountKey),
}, errSmaller points in this file:
- lines 51–52 call
keysForSummaryMetrics()twice; it should be a package-levelvar(or gone, per above). metric()returns0for an absent metric, so if a future k6 renames or stops emittingchecks/rpc_request_failures, every assertion inrunScenarioreads zero and the run passes green. Given the harness exists to detect regressions, "metric missing" and "metric is zero" arguably shouldn't be the same value — at minimum worth a comment saying the fallback is deliberate.{"http_req_failed", "passes"}reads backwards: for a k6Rate,passescounts the failed requests. One comment saves the next reader a trip to the k6 docs.HTTPRequestFailuresis the only extracted field nothing ever asserts on (FailedChecks/RequestFailures/VUFailuresare all checked inrunner.go:504); if that's deliberate record-keeping, say so, otherwise it looks like a missed check.
| if !commitPattern.MatchString(junoCommit) { | ||
| return nil, fmt.Errorf("embedded Juno commit is invalid") | ||
| } | ||
|
|
||
| required := []string{ | ||
| "NODE_URL", | ||
| "READY_URL", | ||
| "EXPECTED_CHAIN_ID", | ||
| "EXPECTED_BLOCK_NUMBER", | ||
| "SNAPSHOT_ID", | ||
| "SNAPSHOT_SHA256", | ||
| "JUNO_IMAGE_DIGEST", | ||
| "RUNNER_IMAGE_DIGEST", | ||
| } | ||
| values := make(map[string]string, len(required)) | ||
| for _, name := range required { | ||
| value, _ := getenv(name) | ||
| if value == "" { |
There was a problem hiding this comment.
nit (two related points, both carried over from the shell version):
-
loadConfigfailures are now the only path that produces no manifest.main.go:12prints to stderr and exits 2 before arunnerexists, so the single most likely operational failure — a deployment forgetting one of these eight variables — leaves/resultsempty, while the README says the manifest records failures. Everything downstream of this (corpus validation, configuration, readiness, …) does write a manifest now, so consumers need a separate "no manifest at all" code path just for this case.RESULTS_DIRis resolvable independently of the required set, so a minimal manifest withstage: "preflight"is reachable here. -
SNAPSHOT_ID,SNAPSHOT_SHA256,JUNO_IMAGE_DIGESTandRUNNER_IMAGE_DIGESTare required but never format-checked — they're only echoed intomanifest.jsonas provenance.test-runner.sh:95passesJUNO_IMAGE_DIGEST=sha256:juno, which is exactly the shape of a typo that would silently poison the provenance record for a whole run of nightly results. A cheap check invalidate()catches it at the right stage:
if !sha256Pattern.MatchString(c.snapshotSHA256) {
return fmt.Errorf("SNAPSHOT_SHA256 must be a 64-character hex digest")
}
for name, digest := range map[string]string{"JUNO_IMAGE_DIGEST": c.junoImageDigest, "RUNNER_IMAGE_DIGEST": c.runnerImageDigest} {
if !digestPattern.MatchString(digest) { // ^sha256:[0-9a-f]{64}$
return fmt.Errorf("%s must be sha256:<64 hex>", name)
}
}Given the harness's whole value proposition is reproducibility, unverifiable provenance fields are the weakest link in the manifest.
Add a fixed mainnet RPC corpus, a reproducible k6 runner, and CI publication of matched Juno and runner images for the nightly ArgoCD benchmark.
32b6f69 to
6584da8
Compare
|
Claude finished @danielntmd's task in 3m 34s —— View job Review: nightly RPC benchmark harness —
|
| if r.config.runIDFile != "" { | ||
| if err := writeAtomic(r.config.runIDFile, []byte(r.config.runID+"\n")); err != nil { | ||
| fmt.Fprintf(r.stderr, "write run ID: %v\n", err) | ||
| return 2 | ||
| } | ||
| } | ||
| if err := r.cleanKnownOutputs(); err != nil { | ||
| fmt.Fprintf(r.stderr, "clean result outputs: %v\n", err) | ||
| return 2 | ||
| } |
There was a problem hiding this comment.
nit (not raised before — a residual of the "every failure writes a manifest" fix): moving the required-variable checks into validateProvenance() closed the big hole, but two reachable early exits still return 2 with no manifest.json, and both happen after RESULTS_DIR already exists — so writing one is possible here, unlike the MkdirAll failure above.
RUN_ID_FILE(line 108) is a documented deployment integration point —README.md:99-102says the deployment sets it "for a deployment-side publisher". A wrong path, a missing parent directory, or a read-only mount is exactly the sort of thing that goes wrong once in a deployment's lifetime, and it produces an empty results volume rather than aconfiguration-stage manifest. NotewriteAtomicwrites its temp file intofilepath.Dir(RUN_ID_FILE), so this fails for a directory that doesn't exist even if the final path would have been writable.cleanKnownOutputs(line 113) fails on a stale root-ownedmanifest.jsonfrom a previous run under a different UID, or a permissions change on the volume — again silently manifest-less.
Both are the same class the last commit deliberately fixed for loadConfig; r.fail(...) is already reachable at this point (failExitCode is still 2 and currentStage is "preflight"), so the fix is mechanical:
if r.config.runIDFile != "" {
if err := writeAtomic(r.config.runIDFile, []byte(r.config.runID+"\n")); err != nil {
return r.fail(fmt.Sprintf("write run ID: %v", err))
}
}
if err := r.cleanKnownOutputs(); err != nil {
return r.fail(fmt.Sprintf("clean result outputs: %v", err))
}with startSignalHandler hoisted above them if you want the signal contract to hold across these too. MkdirAll genuinely can't write a manifest, so leaving that one as a bare stderr line is right — worth saying so in README.md since it becomes the only no-manifest exit.
| return fmt.Errorf("marshal manifest: %w", err) | ||
| } | ||
| data = append(data, '\n') | ||
| return writeAtomic(filepath.Join(r.config.resultsDir, "manifest.json"), data) |
There was a problem hiding this comment.
Why is it named atomic? If it's referring to an atomic operation, I don't think it's actually atomic.
There was a problem hiding this comment.
Ah, naming changed during migration, will rename since it does guarantee stronger guarantees implied by atomic.
| @@ -0,0 +1,698 @@ | |||
| package main | |||
There was a problem hiding this comment.
| package main | |
| package k6runner |
Or something along these lines
There was a problem hiding this comment.
We should keep it as main for now as bench/rpc/cmd/corpus-gen and cmd/junoalso follows this; renaming it would require exporting an entry point and more directory churn.
If we have another consumer, then we can rename and create those internal file structure boundaries.
| # Build with make juno | ||
| RUN make juno | ||
| # Dedicated benchmark images provide an exact source version. Existing image | ||
| # builds retain the Makefile's git-describe version. |
There was a problem hiding this comment.
Have you considered updating Makefile to something like this:
JUNO_VERSION ?= $(shell git describe --tags)
juno: rustdeps
@go build $(GO_TAGS) -a -ldflags="-X main.Version=$(JUNO_VERSION)" -o build/juno ./cmd/juno/
There was a problem hiding this comment.
I started with this approach, but failed the full suite CI (mandated by makefile change) on ARM and macOS.
I'll restore and try again.
|
Claude finished @danielntmd's task in 4m 50s —— View job Review: nightly RPC benchmark harness —
|
| review comment | status |
|---|---|
writeAtomic isn't actually atomic |
fixed — renamed to writeFileViaRename / temporaryFilePath / temporaryFilePattern, and the rename is complete (grep for writeAtomic|atomicTemporary across *.go/*.md/*.sh returns nothing, so no stale doc or shell reference). The new name is the accurate one: rename(2) within a filesystem is atomic for a concurrent reader, so the manifest can never be observed half-written; what the function never had is fsync on the temp file and parent dir, i.e. crash durability, which is not something a benchmark manifest needs |
duplicate make juno recipe in the Dockerfile |
fixed — JUNO_VERSION ?= $(shell git describe --tags) in the Makefile, Dockerfile reduced to make juno JUNO_VERSION="${JUNO_VERSION}", so $(GO_TAGS) and the platform CGO_LDFLAGS are no longer forked and can't drift. juno-cached picked up the same variable, and Makefile was added to the workflow's pull_request paths. This closes the Dockerfile:49-56 finding I'd carried across four passes |
keep package main |
acknowledged, agreed — bench/rpc/cmd/corpus-gen and cmd/juno are both main, and a library package would need an exported entry point for one consumer |
go vet ./bench/rpc/... is clean. go test needs an approval I don't have in this run, so I read the tests rather than executing them; CI covers them.
Nits (new)
Makefile:33—?=isifeq ($(origin V),undefined), and GNU Make gives a set-but-empty environment variable originenvironment, soJUNO_VERSION=does not fall back togit describe— it stamps-X main.Version=silently. That lands on the branch this PR didn't change:Dockerfile:49takeselse make junoexactly whenJUNO_VERSIONis empty, so if BuildKit puts a valuelessARGinto theRUNenv as"", the guard is self-defeating. I believe BuildKit omits valueless args (dispatchRunonlyAddEnvs non-nil values), which would make this latent rather than live — butdockerandmakeboth needed approvals I don't have in this job, so please confirm rather than take my word for it. Anifeq ($(strip $(JUNO_VERSION)),)guard makes the question moot and collapsesDockerfile:49-53to a single un-branchedRUN..github/workflows/benchmark-image.yaml:10— addingMakefileto the paths filter exposes that thetestjob builds onlybench/rpc/Dockerfile; the mainDockerfile, and therefore the wholeJUNO_VERSIONplumbing this commit rewrote, is built only inpublish(gated onmain, and it pushes). So Build RPC Benchmark Images passing here says nothing about whethermake juno JUNO_VERSION=sha-<sha>works. The failure mode is recoverable but late —publishsucceeds,:nightlymoves, and the first signal is nightly runs failing invalidateTarget.make -n juno JUNO_VERSION=sha-test | grep -Fasserts the substitution for free, without compiling. Comment-only: I can't modify.github/workflows/.
Still open — re-verified against the current tree, not carried over
run.js:51— stillcorpus[exec.scenario.iterationInTest % corpus.length]with no offset, so the 200-iteration warmup and the 200-iterationsinglescenario both replaycorpus[0..199]: sequential latency is measured entirely warm, and 9,800 of the 10,000 committed entries never touch that scenario.config.go:226—parseDurationistime.ParseDuration+> 0with no lower bound, soREADY_POLL_INTERVAL=1msis accepted andrunReadinesspolls and logs a line every tick (~1.8 M requests and 1.8 M log lines over the default 30 m timeout, aimed at a node still restoring a snapshot). Same thread:READY_TIMEOUT=30used to mean 30 s and is now rejected (worth checking argocd#9780 before merge), the message no longer names the accepted forms, and neither variable is inREADME.md's override table — re-confirmed it lists onlyITERATIONS,VUS,CONCURRENCY_DURATION,THROUGHPUT_VUS,RATES,THROUGHPUT_DURATION.throughput.js:19— stillmaxVUs: THROUGHPUT_VUSalongsidepreAllocatedVUs: THROUGHPUT_VUS, sodropped_iterationscannot separate node saturation from runner VU starvation (3000 req/s with 50 VUs requires latency < 16.7 ms).throughput.js:20— stillramping-arrival-ratewithstagesfrom the defaultstartRate: 0, so 1000/2000/3000 are never held steady, and--summary-exportemits one aggregate; the manifest advertisesratesnext to percentiles that belong to none of them.metrics.go:57-77—checks,rpc_request_failures,http_req_failed,vu_failuresanditerationsare all still in therequiredtable whiledropped_iterationsis the one optional metric, so a scenario that executes zero iterations fails asmissing metric checks.failsinstead of reaching thedroppedIterations != 0branch — the one input the harness exists to characterise.http_req_failed'spassescounting failures still deserves a comment now that it's asserted on.runner.go:107-116— two early exits still return 2 with nomanifest.json, both afterRESULTS_DIRexists so a manifest is writable: an unwritableRUN_ID_FILE(a documented integration point,README.md:99-102) and acleanKnownOutputsfailure.r.fail(...)is already reachable there.manifest.go— records no latency at all, so a performance run's manifest carries its full configuration and provenance but none of its performance.runner.go:570-586—tailWritertruncation is silent and can start mid-rune, so a clippedfailure.reasonis indistinguishable from a complete one and may open with a U+FFFD.runner_test.go:426-438—TestTailWriter'sFatalfreports only lengths, equal by construction, and the sub-cap accumulate path (the shortexecutable file not foundcase the capture exists to preserve verbatim) is never executed.test-runner.sh— negative cases still assert only "non-zero"; nothing pins the deliberate exit 2 (configuration,corpus-validation) vs 1 (readinessonwards) split the deployment side will branch on.bench/rpc/corpus/v0_10/getTransactionByHash.jsonis 1.6 MB on a single line; every regeneration adds another ~1.6 MB to history permanently.
No blockers, and nothing outside bench/ touches consensus, sync, or RPC-serving paths — the Makefile edit is version-string-only and platform-independent, so if the full suite trips on macOS/ARM again it won't be from this. Nothing was changed on the branch; this is review feedback only.
· branch danielntmd/benchmark-image
|
Claude finished @danielntmd's task in 5m 19s —— View job Review: nightly RPC benchmark harness —
|
GitHub Actions
Failure
|
superseded by #3944 |
2 participants
Summary
Adds the Juno-side foundation for reproducible nightly RPC performance benchmarks against a fixed mainnet snapshot.
Changes
See https://github.com/NethermindEth/argocd/pull/9780