bench(onthebench): shared measurement lib, generic entrant runner, method overrides - #919
Merged
Conversation
…thod overrides Extract the measurement core (mock/floor/window/flamegraph primitives, grid handling, meta fragments) from run-baseline.sh into lib.sh, and add run-entrant.sh, a runner that measures any gateway-shaped process through the exact same code path via a small entrant contract (start hook, request shape override, optional prepare/teardown and identity metadata). Entrant definitions live outside the repository on purpose. Method knobs (grid, reps, window, warmup, floor reps, flamegraph) become BENCH_* environment overrides with unchanged defaults, forwarded by bench.sh, so a spot-check or an added delay tier is an invocation rather than a script edit. Floors derive from the grid: one per 0-delay tier at its max concurrency, one per concurrency on delayed tiers where the instrument ceiling is ~conc/delay. Liveness checks use /proc existence instead of kill -0, which reports EPERM (reading as death) for containerized targets owned by another user. Default-grid behavior of run-baseline.sh is unchanged; same-rig spot-checks against the existing baseline validate the refactor.
📝 WalkthroughWalkthroughThe benchmark harness centralizes configuration, validation, measurements, metadata, grid execution, retries, and flamegraph capture. The baseline runner uses these helpers. A new entrant runner benchmarks external gateway-shaped processes with configurable hooks and target metadata. ChangesBenchmark harness
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EntrantRunner
participant EntrantTarget
participant MockService
participant LoadGenerator
participant Results
EntrantRunner->>EntrantTarget: invoke entrant_start
EntrantRunner->>EntrantTarget: wait for HTTP readiness
EntrantRunner->>MockService: start mock for TTFT tier
EntrantRunner->>LoadGenerator: run floor and concurrency points
LoadGenerator->>EntrantTarget: send benchmark requests
EntrantRunner->>Results: append measurements and metadata
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Extracts shared benchmarking logic and adds support for configurable methods and external gateway entrants.
Changes:
- Adds shared measurement and metadata utilities.
- Introduces a generic entrant runner.
- Supports forwarded
BENCH_*overrides and documents usage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
bench/onthebench/lib.sh |
Adds shared benchmark measurement logic. |
bench/onthebench/run-entrant.sh |
Adds the generic entrant runner. |
bench/onthebench/run-baseline.sh |
Migrates baseline execution to the shared library. |
bench/onthebench/bench.sh |
Forwards method overrides to the rig. |
bench/onthebench/README.md |
Documents overrides, floors, and entrants. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+31
to
+34
| if [ -z "${OTB_LOADGEN_HEADERS:-}" ]; then | ||
| OTB_LOADGEN_HEADERS='[["'"${AUTH_HEADER%%:*}"'","'"${AUTH_HEADER#*: }"'"]]' | ||
| fi | ||
| export OTB_LOADGEN_HEADERS |
| # shellcheck source=lib.sh | ||
| source "$(dirname "${BASH_SOURCE[0]}")/lib.sh" | ||
|
|
||
| [ -n "$ENTRANT_NAME" ] || { echo "FATAL: entrant.sh must set ENTRANT_NAME"; exit 1; } |
Comment on lines
+86
to
+100
| local bin_sha | ||
| bin_sha=$(sudo -n sha256sum "/proc/$GW_PID/exe" 2>/dev/null | cut -d' ' -f1 || true) | ||
| cat > "$OUT/meta.json" <<EOF | ||
| { | ||
| "entrant": "$ENTRANT_NAME", | ||
| "timestamp_utc": "$(date -u +%Y-%m-%dT%H:%M:%SZ)", | ||
| "rig": $(meta_rig_json), | ||
| "cores": $(meta_cores_json), | ||
| "instruments": $(meta_instruments_json), | ||
| "method": $(meta_method_json), | ||
| "target": { | ||
| "binary_sha256": "${bin_sha:-unknown}", | ||
| "rss_idle_kb": ${RSS_IDLE:-null}, "rss_hwm_kb": $1, | ||
| "threads": ${THREADS:-null}, | ||
| "identity": $(type entrant_meta_json >/dev/null 2>&1 && entrant_meta_json || echo null) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/onthebench/bench.sh`:
- Around line 49-53: Replace the short-circuit statement in
bench/onthebench/bench.sh lines 49-53 with an if block that appends each set
BENCH_* override to ENVPASS, allowing unset variables without returning failure.
Also update the symbol-check logic in bench/onthebench/run-baseline.sh lines
30-32 to call require_symbols "$BIN" only when FLAMEGRAPH equals 1, so a zero
value continues to bench_init.
In `@bench/onthebench/lib.sh`:
- Around line 169-178: Make both cpu_ticks reads in the measurement flow
fallible, including the read after loadgen, and detect when the target has
exited; in that case set gw_cpu_pct to null and force valid=false. Ensure
cleanup and JSONL record generation still execute under errexit so retry and
HARNESS_RC handling remain effective. Add a regression test that terminates the
target during a measurement window and verifies the invalid record is preserved.
- Around line 15-21: Validate all BENCH_* overrides before measurement: require
positive WINDOW, WARMUP, REPS, MAX_TRIES, and FLOOR_REPS values, a non-empty
BENCH_GRID containing valid ttft_ms:conc entries, and BENCH_FLAMEGRAPH
restricted to 0 or 1; make invalid settings exit nonzero before reporting
success. Update bench/onthebench/README.md lines 71-77 to document the accepted
values and ranges for every override, explicitly describing BENCH_FLAMEGRAPH as
0|1. Add regression tests covering invalid overrides and confirming they fail
before a benchmark run succeeds; if any validation range is ambiguous, state the
assumption or request clarification rather than silently choosing one.
- Around line 203-212: Update the floor loop around loadgen and the floor JSON
emission to use the same validity fields and rules as measured_window, including
rigrefused, budgetexceeded, and spawnfailed, so none can count toward valid_n.
Normalize missing rps, p50_us, and p99_us values to JSON null, including when
loadgen returns an empty line, while preserving valid numeric metrics. Add
regression coverage for an empty loadgen result and each invalidity flag.
In `@bench/onthebench/run-entrant.sh`:
- Line 49: Define and enforce a JSON-safe ENTRANT_NAME contract in
run-entrant.sh: either JSON-escape the value before meta.json generation or
validate it against a documented safe identifier pattern such as [A-Za-z0-9._-]+
and reject invalid names before the existing nonempty check succeeds. Add
regression coverage for names containing JSON-special characters, and state the
chosen validation/escaping assumption explicitly.
- Around line 122-126: Update the symbol discovery command in the flamegraph
conditional around flamegraph_point to invoke nm through sudo -n for
/proc/$GW_PID/exe, while preserving the existing warning path when privileged
access fails or no text symbols are found.
- Around line 59-60: Update the preparation flow around entrant_prepare so it
runs as a standalone command rather than a pipeline element, while still writing
output to "$OUT/prepare.log" and displaying it on stderr. Preserve the existing
fatal message and exit behavior through an ERR/error-reporting path, and add a
regression case where a non-final command inside entrant_prepare fails to ensure
preparation cannot report success.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 29b1b85b-666f-431a-9fc9-7f3e5c5b0c36
📒 Files selected for processing (5)
bench/onthebench/README.mdbench/onthebench/bench.shbench/onthebench/lib.shbench/onthebench/run-baseline.shbench/onthebench/run-entrant.sh
Comment on lines
+122
to
+126
| if nm "/proc/$GW_PID/exe" 2>/dev/null | grep -q ' [tT] '; then | ||
| flamegraph_point 128 "$ENTRANT_NAME c=128 0-delay (4 pinned cores)" | ||
| else | ||
| echo "WARNING: target binary is stripped or unreadable; skipping flamegraph" >&2 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use privileged symbol discovery for /proc/$GW_PID/exe.
/proc/$GW_PID/exe can belong to another user, and the unprivileged nm "/proc/$GW_PID/exe" at line 122 may fail while the earlier process check succeeds. Use sudo -n nm "/proc/$GW_PID/exe" and retain the warning when symbol access or symbols are unavailable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/onthebench/run-entrant.sh` around lines 122 - 126, Update the symbol
discovery command in the flamegraph conditional around flamegraph_point to
invoke nm through sudo -n for /proc/$GW_PID/exe, while preserving the existing
warning path when privileged access fails or no text symbols are found.
- grid_concs: use if instead of && so the function cannot return nonzero when the last grid element belongs to another tier; under set -e + pipefail the floor-conc assignment killed the default-grid run silently before the first floor window. - entrant_prepare: run the hook in an explicit errexit subshell and check PIPESTATUS[0]; the previous 'pipeline || die' form disabled errexit inside the hook, letting a mid-prepare failure pass as success. - Record and warn about child processes of the measured pid: per-pid CPU%/RSS silently understates multi-process targets; the count lands in meta.json as target.children. - THREADS capture no longer appends a second line when ps fails under pipefail; flamegraph 'enabled' in meta reflects the grid gate as well as the knob; OTB_LOADGEN_HEADERS derives via json.dumps so quotes or backslashes in a header value cannot emit invalid JSON; the entrant flamegraph gate reuses the >100 text-symbol threshold instead of >=1; contract header documents entrant_stop idempotency and which harness variables hook bodies may use.
Contributor
Author
|
Independent cold-context audit ran against 3fbcf1b; all findings addressed in 5b32e4c:
Re-validation performed on the rig: (a) shortened default-grid baseline run ( |
- Validate BENCH_* overrides at startup: grid entries must be ttft_ms:conc with positive concurrency, counts must be positive integers (warmup may be 0), flamegraph knob must be 0 or 1 - zero-rep runs could previously report success while measuring nothing. - Record a target that dies mid-window as an invalid window: CPU tick reads are now fallible, cpu_pct goes null and valid=false, and the JSONL line still lands instead of the runner dying at a missing /proc/<pid>/stat. - Bring floor windows up to gateway-window validity: rigrefused, budgetexceeded and spawnfailed now invalidate a floor, and an empty loadgen line records nulls instead of invalid JSON. - Always derive OTB_LOADGEN_HEADERS from AUTH_HEADER; an ambient value would let measured requests carry credentials the readiness check never exercised. - Constrain ENTRANT_NAME to [A-Za-z0-9._-]+ (spliced verbatim into JSON); validate entrant_meta_json output before it reaches meta.json and fail loudly on hook failure or unparseable output. - Replace remaining statement-position '[ cond ] && cmd' with if-form (the pattern behind the earlier grid_concs regression), and document override value domains in the README.
Contributor
Author
|
Disposition of the 10 inline bot findings, addressed in 9f756fe (all fixes re-validated with a shortened default-grid run on the rig): Fixed
No change, with reasons
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/onthebench/run-entrant.sh`:
- Around line 113-117: Update the JSON validation in the entrant_meta_json
handling block to require that the parsed value is a JSON object, rejecting
strings, arrays, numbers, booleans, and null while preserving the existing
failure message and exit behavior. Keep the identity assignment and valid-object
flow unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df9af1e2-8b2a-4f72-9c67-6265873bad28
📒 Files selected for processing (5)
bench/onthebench/README.mdbench/onthebench/bench.shbench/onthebench/lib.shbench/onthebench/run-baseline.shbench/onthebench/run-entrant.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- bench/onthebench/README.md
- bench/onthebench/lib.sh
- bench/onthebench/run-baseline.sh
Comment on lines
+113
to
+117
| if type entrant_meta_json >/dev/null 2>&1; then | ||
| identity=$(entrant_meta_json) || | ||
| { echo "FATAL: entrant_meta_json failed"; exit 1; } | ||
| printf '%s' "$identity" | python3 -c 'import json,sys; json.load(sys.stdin)' || | ||
| { echo "FATAL: entrant_meta_json returned invalid JSON: $identity"; exit 1; } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline/lines =="
wc -l bench/onthebench/run-entrant.sh
sed -n '1,180p' bench/onthebench/run-entrant.sh | nl -ba
echo "== references to entrant_meta_json =="
rg -n "entrant_meta_json|target\.identity|ENTRANT_META_JSON|identity" bench/onthebench or thebench . || true
echo "== json scalar object behavior probe =="
python3 - <<'PY'
import json, subprocess, textwrap
for value in ["1", '"a"', "[1]", "{}", "null"]:
res = subprocess.run(["python3", "-c",
"import json,sys; json.load(sys.stdin)"],
input=value.encode(), text=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
obj_res = subprocess.run(["python3", "-c",
"import json,sys; value=json.load(sys.stdin); sys.exit(not isinstance(value, dict))"],
input=value.encode(), text=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
print(value, "load_exit", res.returncode, "load_type", type(json.loads(value)).__name__,
"is_dict_exit", obj_res.returncode)
PYRepository: api7/aisix
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lines 1-180 =="
sed -n '1,180p' bench/onthebench/run-entrant.sh | sed 's/^/L /'
echo "== references to entrant_meta_json =="
rg -n "entrant_meta_json|target\.identity|ENTRANT_META_JSON|identity" bench/onthebench || true
echo "== json scalar object behavior probe =="
python3 - <<'PY'
import json, subprocess
for value in ["1", '"a"', "[1]", "{}", "null"]:
res = subprocess.run(["python3", "-c", "import json,sys; json.load(sys.stdin)"],
input=value.encode(), stdout=subprocess.PIPE, stderr=subprocess.PIPE)
obj_res = subprocess.run(["python3", "-c", "import json,sys; value=json.load(sys.stdin); sys.exit(not isinstance(value, dict))"],
input=value.encode(), stdout=subprocess.PIPE, stderr=subprocess.PIPE)
print(value, "load_exit", res.returncode, "parsed_type", type(json.loads(value)).__name__, "is_dict_exit", obj_res.returncode)
PYRepository: api7/aisix
Length of output: 10232
Require a JSON object from entrant_meta_json.
The hook is documented as a JSON object, but the current validation accepts any valid JSON value. A string, array, number, or null makes target.identity an invalid stable type. Reject non-object JSON.
Proposed fix
- printf '%s' "$identity" | python3 -c 'import json,sys; json.load(sys.stdin)' ||
- { echo "FATAL: entrant_meta_json returned invalid JSON: $identity"; exit 1; }
+ printf '%s' "$identity" |
+ python3 -c 'import json,sys; value=json.load(sys.stdin); sys.exit(not isinstance(value, dict))' ||
+ { echo "FATAL: entrant_meta_json must return a valid JSON object: $identity"; exit 1; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if type entrant_meta_json >/dev/null 2>&1; then | |
| identity=$(entrant_meta_json) || | |
| { echo "FATAL: entrant_meta_json failed"; exit 1; } | |
| printf '%s' "$identity" | python3 -c 'import json,sys; json.load(sys.stdin)' || | |
| { echo "FATAL: entrant_meta_json returned invalid JSON: $identity"; exit 1; } | |
| if type entrant_meta_json >/dev/null 2>&1; then | |
| identity=$(entrant_meta_json) || | |
| { echo "FATAL: entrant_meta_json failed"; exit 1; } | |
| printf '%s' "$identity" | | |
| python3 -c 'import json,sys; value=json.load(sys.stdin); sys.exit(not isinstance(value, dict))' || | |
| { echo "FATAL: entrant_meta_json must return a valid JSON object: $identity"; exit 1; } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/onthebench/run-entrant.sh` around lines 113 - 117, Update the JSON
validation in the entrant_meta_json handling block to require that the parsed
value is a JSON object, rejecting strings, arrays, numbers, booleans, and null
while preserving the existing failure message and exit behavior. Keep the
identity assignment and valid-object flow unchanged.
Source: Coding guidelines
This was referenced Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #917. Three changes to the same-rig harness, no default-behavior change:
lib.sh— the measurement core (mock lifecycle + TTFT assertion, rig floor, measured windows with CPU%/RSS sampling, validity policy, flamegraph, meta fragments) extracted fromrun-baseline.shinto one shared file. Both runners below source it, so any two runs are comparable by construction instead of by code review.run-entrant.sh— a generic runner that measures any gateway-shaped process ("entrant") through the exact same code path: same instruments, core split, windows, floors and validity policy. The entrant contract (documented in the script header) is a sourcedentrant.shproviding a start hook that leaves the measured pid inGW_PID, plus optional prepare/teardown hooks, identity metadata, and a request-shape override (REQ_PATH/BODY/AUTH_HEADER) for targets whose only ingress speaks a dialect other than OpenAI chat. Entrant definitions live outside the repository on purpose.BENCH_*method overrides — grid, reps, window, warmup, max tries, floor reps and flamegraph become env-overridable (defaults unchanged, forwarded bybench.sh), so a two-window spot-check or an added delay tier is an invocation rather than a script edit. Overrides are recorded inmeta.json, so a non-default run can never pass silently as the standard grid.Floors now derive from the grid: one floor per 0-delay tier at its max concurrency (the instrument ceiling barely moves with concurrency there), one floor per concurrency on delayed tiers (the ceiling is ~conc/delay). Under the default grid this reproduces exactly the floors the harness always ran.
Two correctness fixes that fall out of generalizing:
/proc/<pid>existence instead ofkill -0, which reports EPERM — reading as death — for a containerized target owned by another user (the RSS sampler would silently record 0).entrant_preparefailures propagate through theteepipeline (pipefail + explicit message) instead of aborting with no context; the pipeline-subshell semantics are documented in the contract (state hand-off through files, not variables).Why
The performance program needs same-rig, same-instrument comparisons of multiple gateway targets, and extra load points (e.g. an added delay tier at higher concurrency), without forking the measurement methodology per target. One chokepoint (
lib.sh) keeps the family from drifting.Validation
bash -non all five scripts.run-baseline.sh(BENCH_GRID="0:128", byte-identical product binary vs the bench: onthebench-style rig harness for same-rig baselines #917 baseline run) — results recorded in the program tracker, not here.run-entrant.shexercised against native and containerized targets in the program's measurement session (entrant definitions are deliberately out-of-repo; the results land in the internal tracker).Summary by CodeRabbit
New Features
BENCH_*environment variables.Improvements