bench: generate both columns of every comparison from one shape table - #35
bench: generate both columns of every comparison from one shape table#35shreemaan-abhishek wants to merge 2 commits into
Conversation
The old harness hand-wrote one target location per client, so the two sides drifted into different API shapes and the published ratio credited that drift to the C-vs-Lua difference. Every case is now a driver plus a shape, and both columns of a pair are built from the same shape table, so they send the same headers, size the same pool and consume the body the same way by construction rather than by review. A fairness audit diffs the raw request each client actually put on the wire and fails the run if the caller headers disagree. benchmark/cases.txt is the single source of truth: run.sh generates one location per name and the smoke check diffs it against the table loaded in the worker, so the two cannot drift. The matrix covers read mode, connection lifetime, method, transport, peer address, response framing and header count. Also adds repeats with median reporting, a saturation gate that discards any window where the target worker was not CPU-bound, CSV output, fold.sh to render it, a separate fixed-rate latency phase (latency measured at saturation is queue depth, not latency), and connection-reuse accounting taken from the upstream so it stays off the measured worker's hot path.
📝 WalkthroughWalkthroughThe PR replaces the benchmark with a configurable HTTP/1.1 matrix for FFI and ChangesBenchmark matrix
APISIX integration assessment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Runner
participant TargetNginx
participant BenchLua
participant UpstreamNginx
participant FoldScript
Runner->>TargetNginx: Generate and load benchmark case routes
Runner->>TargetNginx: Execute throughput or latency cases
TargetNginx->>BenchLua: Run selected case
BenchLua->>UpstreamNginx: Send HTTP or TLS request
UpstreamNginx-->>BenchLua: Return response and counters
BenchLua-->>TargetNginx: Return fixed benchmark response
TargetNginx-->>Runner: Produce wrk2 metrics
Runner->>FoldScript: Process CSV results
FoldScript-->>Runner: Emit Markdown benchmark tables
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![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
🧹 Nitpick comments (7)
benchmark/README.md (1)
249-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the markdownlint warnings.
Two code fences have no language (lines 249 and 333), and
## Resultsappears twice (lines 20 and 327). Usetextfor both fences and rename the second heading, for example## Result files.Also applies to: 327-327, 333-333
🤖 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 `@benchmark/README.md` at line 249, Fix the markdownlint issues in benchmark/README.md by adding the text language to the code fences at the referenced locations and renaming the second “## Results” heading to a distinct heading such as “## Result files,” while preserving the surrounding content.Source: Linters/SAST tools
benchmark/results-full.md (1)
240-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the
perf difffence.markdownlint reports MD040 for this fence. Use
text.🤖 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 `@benchmark/results-full.md` at line 240, Update the fenced code block in benchmark/results-full.md to declare the text language by adding `text` after the opening fence, resolving markdownlint MD040 without changing the block contents.Source: Linters/SAST tools
benchmark/fold.sh (1)
147-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead loop and the stray printf argument.
Line 218 is an empty loop body and has no effect. Line 147 passes an extra empty argument to a format string that has no conversion specifier.
♻️ Proposed cleanup
- printf "The outbound cost ratio subtracts that baseline from both ", - "" + printf "The outbound cost ratio subtracts that baseline from both " printf "client paths and compares what is left.\n"- for (i = 0; i < n; i++) { } for (name in discarded)Also applies to: 218-218
🤖 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 `@benchmark/fold.sh` around lines 147 - 148, Remove the unused empty argument from the printf call near the outbound cost ratio message, and delete the empty loop body at the referenced loop location. Keep the surrounding output and loop behavior unchanged.t/003-benchmark-runner.t (1)
99-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the remaining shapes to the axes list.
The loop covers 12 shapes.
cases.txtdefines 16. Add the four missing names so a removal fromcases.txtfails here.💚 Proposed change
-for my $shape (qw(oneshot stateful stream short post tls dns chunked trailers - hdr40 cookies req30)) { +for my $shape (qw(oneshot stateful stream readbody short post tls tlsshort + tlsverify tlsverifyshort dns chunked trailers + hdr40 cookies req30)) {🤖 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 `@t/003-benchmark-runner.t` around lines 99 - 103, Add the four shapes defined in cases.txt but absent from the qw(...) list in the matrix coverage loop, preserving the existing 12 names and ordering where practical. Keep the assertions keyed through %by_shape so removing any defined shape from cases.txt causes this coverage check to fail.benchmark/run.sh (2)
169-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the intentional word splitting.
BENCH_CASESis a space-separated list, so the unquoted expansion is required here. Add a directive so the intent is explicit and shellcheck stays quiet.♻️ Proposed change
+ # BENCH_CASES is a space-separated list; splitting is the point + # shellcheck disable=SC2086 printf '%s\n' $requested🤖 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 `@benchmark/run.sh` at line 169, Add a ShellCheck directive immediately before the unquoted $requested expansion in the printf statement to explicitly suppress the intentional word-splitting warning, while preserving the existing space-separated BENCH_CASES behavior.Source: Linters/SAST tools
755-767: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shape dedupe loop into one helper.
fairness_auditandlatency_phasecontain the same loop that strips the driver prefix and deduplicates shapes. One helper keeps the two phases from drifting when a new driver prefix is added.♻️ Proposed helper
# shapes present in CASES, in first-seen order case_shapes() { local name shape seen s local -a shapes=() for name in "${CASES[@]}"; do [[ "$name" == "baseline" ]] && continue shape="${name#*.}" seen=0 for s in "${shapes[@]:-}"; do [[ "$s" == "$shape" ]] && seen=1 done (( seen )) || shapes+=("$shape") done printf '%s\n' "${shapes[@]:-}" }Both phases then read
mapfile -t shapes < <(case_shapes).Also applies to: 886-898
🤖 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 `@benchmark/run.sh` around lines 755 - 767, Extract the duplicated shape-prefix stripping and first-seen deduplication logic from fairness_audit and latency_phase into a shared case_shapes helper. Have the helper iterate over CASES and emit shapes in order, then update both phases to populate their shapes arrays from its output via mapfile, preserving baseline exclusion and ordering.benchmark/conf/target.nginx.conf (1)
103-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a silent case invocation for
/gcdelta.
bench.auditwrites one line per call, so this endpoint returns 21 audit lines followed by the delta. The runner depends ontail -n 1to recover the value. A failing case inside the loop also cannot set its 502 status, because the firstngx.sayalready flushed the headers.A
bench.probe(name)helper that runs the case and returns without writing would make the response one line and let this location report failures.🤖 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 `@benchmark/conf/target.nginx.conf` around lines 103 - 124, Add a silent bench.probe helper and update the /gcdelta handler to use it for the warm-up and repeated measurements instead of bench.audit, preserving the single delta line response. Ensure probe failures propagate before any ngx.say call so the case can set its 502 status.
🤖 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 `@benchmark/fold.sh`:
- Around line 119-142: Sort the headline rows generated from seen_tp before
iterating, replacing the unordered for (name in seen_tp) traversal with the same
alphabetical ordering approach used by the detail and latency tables. Also apply
deterministic sorting to the discarded list iteration at lines 219-221,
preserving the existing filtering and output behavior.
In `@benchmark/README.md`:
- Around line 196-210: Update the benchmark documentation matrix to include the
tlsverify and tlsverifyshort shapes with their TLS verification behaviors, and
add BENCH_LATENCY_REPEATS to the configuration table using its default value of
3. Keep the entries aligned with the definitions in cases.txt, bench.lua, and
run.sh.
- Line 90: Reconcile the benchmark entries for tlsshort and ffi.stateful p99.9
in the README with the authoritative results in results-full.md: replace the
withdrawn figures and recommendations with the corrected trust-store and rerun
results, or clearly mark both entries as superseded and direct readers to
results-full.md. Ensure the affected sections, including lines 94–114, no longer
present the outdated claims as current.
In `@benchmark/run.sh`:
- Around line 626-639: Extend the case exclusion pattern in the non-keepalive
warning logic to also match the verify-short benchmarks `ffi.tlsverifyshort` and
`resty.tlsverifyshort`, while preserving the existing `*.short` and `*.tlsshort`
exclusions. This prevents the reused-almost-nothing warning for these
deliberately non-keepalive cases.
In `@docs/ai-proxy-integration.md`:
- Around line 10-15: Update the APISIX file references in the documentation to
use one repository-relative path convention, removing the extra “apisix/” prefix
from the `ai-transport/http.lua` reference so it matches
`apisix/plugins/ai-drivers/openai-base.lua`.
- Around line 18-31: Update both fenced code blocks in the documentation: add
the lua language identifier to the APISIX request trace block and add http or
text to the raw headers block, including the additional block referenced by the
review. Leave the block contents unchanged.
- Around line 111-118: Update the proxy handling around httpc:connect and
client.connect so configured proxy_opts cannot be silently ignored: either
reject proxy_opts before c_connect or clearly document proxy-configured
deployments as unsupported, with rejection preferred. Add coverage that
distinguishes a proxied connection from a direct connection and verifies the
configured behavior.
---
Nitpick comments:
In `@benchmark/conf/target.nginx.conf`:
- Around line 103-124: Add a silent bench.probe helper and update the /gcdelta
handler to use it for the warm-up and repeated measurements instead of
bench.audit, preserving the single delta line response. Ensure probe failures
propagate before any ngx.say call so the case can set its 502 status.
In `@benchmark/fold.sh`:
- Around line 147-148: Remove the unused empty argument from the printf call
near the outbound cost ratio message, and delete the empty loop body at the
referenced loop location. Keep the surrounding output and loop behavior
unchanged.
In `@benchmark/README.md`:
- Line 249: Fix the markdownlint issues in benchmark/README.md by adding the
text language to the code fences at the referenced locations and renaming the
second “## Results” heading to a distinct heading such as “## Result files,”
while preserving the surrounding content.
In `@benchmark/results-full.md`:
- Line 240: Update the fenced code block in benchmark/results-full.md to declare
the text language by adding `text` after the opening fence, resolving
markdownlint MD040 without changing the block contents.
In `@benchmark/run.sh`:
- Line 169: Add a ShellCheck directive immediately before the unquoted
$requested expansion in the printf statement to explicitly suppress the
intentional word-splitting warning, while preserving the existing
space-separated BENCH_CASES behavior.
- Around line 755-767: Extract the duplicated shape-prefix stripping and
first-seen deduplication logic from fairness_audit and latency_phase into a
shared case_shapes helper. Have the helper iterate over CASES and emit shapes in
order, then update both phases to populate their shapes arrays from its output
via mapfile, preserving baseline exclusion and ordering.
In `@t/003-benchmark-runner.t`:
- Around line 99-103: Add the four shapes defined in cases.txt but absent from
the qw(...) list in the matrix coverage loop, preserving the existing 12 names
and ordering where practical. Keep the assertions keyed through %by_shape so
removing any defined shape from cases.txt causes this coverage check to fail.
🪄 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
Run ID: 17d513a6-9263-449f-add0-e08583f5d186
📒 Files selected for processing (12)
.gitignoreREADME.mdbenchmark/README.mdbenchmark/cases.txtbenchmark/conf/target.nginx.confbenchmark/conf/upstream.nginx.confbenchmark/fold.shbenchmark/lua/bench.luabenchmark/results-full.mdbenchmark/run.shdocs/ai-proxy-integration.mdt/003-benchmark-runner.t
| for (name in seen_tp) { | ||
| if (name == "baseline") continue | ||
| # not split(name, parts, ".") -- awk reads the separator as a regex, so | ||
| # "." matches every character and every field comes back empty | ||
| if (substr(name, 1, 4) != "ffi.") continue | ||
| shape = substr(name, 5) | ||
|
|
||
| f = med_of("qps:ffi." shape) | ||
| r = med_of("qps:resty." shape) | ||
| if (f == "" || r == "") continue | ||
|
|
||
| printf "| `%s` | `%.2f` | `%.2f` | `%.2fx` |", shape, f, r, f / r | ||
|
|
||
| if (base != "" && f > 0 && r > 0) { | ||
| bus = 1000000 / base | ||
| fus = 1000000 / f - bus | ||
| rus = 1000000 / r - bus | ||
| if (fus > 0 && rus > 0) printf " `%.2fx` |", rus / fus | ||
| else printf " n/a |" | ||
| } else if (base != "") { | ||
| printf " n/a |" | ||
| } | ||
| printf "\n" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sort the headline rows so the output is reproducible.
for (name in seen_tp) iterates an awk associative array, and that order is unspecified. The detail table at lines 158-162 and the latency table at lines 193-198 both sort first, so only the headline table is nondeterministic. benchmark/results-full.md lines 45-58 show the effect: the recorded headline table is in arbitrary shape order while the detail table below it is alphabetical.
The output of this script is pasted into README.md and benchmark/README.md, so an unstable row order produces noisy diffs and makes two folds of the same CSV disagree. The discarded list at lines 219-221 has the same problem.
🐛 Proposed fix to sort the headline shapes
- for (name in seen_tp) {
- if (name == "baseline") continue
+ ns = 0
+ for (name in seen_tp) headline[ns++] = name
+ for (i = 0; i < ns; i++)
+ for (j = i + 1; j < ns; j++)
+ if (headline[j] < headline[i]) {
+ t = headline[i]; headline[i] = headline[j]; headline[j] = t
+ }
+
+ for (k = 0; k < ns; k++) {
+ name = headline[k]
+ if (name == "baseline") continue
# not split(name, parts, ".") -- awk reads the separator as a regex, so
# "." matches every character and every field comes back empty
if (substr(name, 1, 4) != "ffi.") continue📝 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.
| for (name in seen_tp) { | |
| if (name == "baseline") continue | |
| # not split(name, parts, ".") -- awk reads the separator as a regex, so | |
| # "." matches every character and every field comes back empty | |
| if (substr(name, 1, 4) != "ffi.") continue | |
| shape = substr(name, 5) | |
| f = med_of("qps:ffi." shape) | |
| r = med_of("qps:resty." shape) | |
| if (f == "" || r == "") continue | |
| printf "| `%s` | `%.2f` | `%.2f` | `%.2fx` |", shape, f, r, f / r | |
| if (base != "" && f > 0 && r > 0) { | |
| bus = 1000000 / base | |
| fus = 1000000 / f - bus | |
| rus = 1000000 / r - bus | |
| if (fus > 0 && rus > 0) printf " `%.2fx` |", rus / fus | |
| else printf " n/a |" | |
| } else if (base != "") { | |
| printf " n/a |" | |
| } | |
| printf "\n" | |
| } | |
| ns = 0 | |
| for (name in seen_tp) headline[ns++] = name | |
| for (i = 0; i < ns; i++) | |
| for (j = i + 1; j < ns; j++) | |
| if (headline[j] < headline[i]) { | |
| t = headline[i]; headline[i] = headline[j]; headline[j] = t | |
| } | |
| for (k = 0; k < ns; k++) { | |
| name = headline[k] | |
| if (name == "baseline") continue | |
| # not split(name, parts, ".") -- awk reads the separator as a regex, so | |
| # "." matches every character and every field comes back empty | |
| if (substr(name, 1, 4) != "ffi.") continue | |
| shape = substr(name, 5) | |
| f = med_of("qps:ffi." shape) | |
| r = med_of("qps:resty." shape) | |
| if (f == "" || r == "") continue | |
| printf "| `%s` | `%.2f` | `%.2f` | `%.2fx` |", shape, f, r, f / r | |
| if (base != "" && f > 0 && r > 0) { | |
| bus = 1000000 / base | |
| fus = 1000000 / f - bus | |
| rus = 1000000 / r - bus | |
| if (fus > 0 && rus > 0) printf " `%.2fx` |", rus / fus | |
| else printf " n/a |" | |
| } else if (base != "") { | |
| printf " n/a |" | |
| } | |
| printf "\n" | |
| } |
🤖 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 `@benchmark/fold.sh` around lines 119 - 142, Sort the headline rows generated
from seen_tp before iterating, replacing the unordered for (name in seen_tp)
traversal with the same alphabetical ordering approach used by the detail and
latency tables. Also apply deterministic sorting to the discarded list iteration
at lines 219-221, preserving the existing filtering and output behavior.
| | `readbody` | `26051.07` | `15462.52` | `1.68x` | `1.99x` | | ||
| | `tls` | `17732.35` | `11861.95` | `1.49x` | `1.63x` | | ||
| | `short` | `8275.94` | `6973.64` | `1.19x` | `1.21x` | | ||
| | `tlsshort` | `872.96` | `1108.38` | `0.79x` | `0.79x` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the tlsshort and ffi.stateful claims with results-full.md.
This PR now publishes two contradictory results for the same two claims.
tlsshort: this file reports0.79xand recommendslua-resty-httpfor fresh handshakes.benchmark/results-full.mdlines 203-210 and 283-286 state that the trust-store fix supersedes thetlsandtlsshortnumbers, and thattlsshortis now1.10x, a win.ffi.statefulp99.9: this file reports 20.940 ms and asks for a re-run withBENCH_LATENCY_REPEATS=3.benchmark/results-full.mdlines 169-184 report that re-run and record the claim as disproven, with samples of 2.720, 2.730 and 2.730 ms.
Either update these two sections or add an explicit pointer to benchmark/results-full.md that marks both claims as superseded. As written, the withdrawn figures are the ones a reader of this file finds.
Also applies to: 94-114
🤖 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 `@benchmark/README.md` at line 90, Reconcile the benchmark entries for tlsshort
and ffi.stateful p99.9 in the README with the authoritative results in
results-full.md: replace the withdrawn figures and recommendations with the
corrected trust-store and rerun results, or clearly mark both entries as
superseded and direct readers to results-full.md. Ensure the affected sections,
including lines 94–114, no longer present the outdated claims as current.
| | shape | axis it prices | | ||
| | --- | --- | | ||
| | `oneshot`, `stateful` | API shape; the headline pair | | ||
| | `readbody` | buffering through the Lua chunk loop instead of the C preread fast path | | ||
| | `stream` | `res.body_reader`, chunk by chunk | | ||
| | `short` | connection setup, with no keepalive pool | | ||
| | `post` | a 4KB request body | | ||
| | `tls` | TLS with a pooled connection | | ||
| | `tlsshort` | a fresh TLS handshake per request | | ||
| | `dns` | resolving the peer by hostname instead of an IP literal | | ||
| | `chunked` | chunked response framing | | ||
| | `trailers` | chunked framing with a trailer section | | ||
| | `hdr40` | 40 response headers | | ||
| | `cookies` | ten repeated `Set-Cookie` headers, which fold into an array | | ||
| | `req30` | 30 request headers, which prices per-request header validation | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the two new TLS verify shapes and BENCH_LATENCY_REPEATS.
benchmark/cases.txt lines 36-39 add tlsverify and tlsverifyshort, and benchmark/lua/bench.lua lines 427-429 register them. The case matrix table does not list either shape, although line 192 states that cases.txt is the single source of truth for the matrix.
The configuration table also omits BENCH_LATENCY_REPEATS, which benchmark/run.sh line 37 defines with default 3 and which line 113 of this file already tells the reader to set.
📝 Proposed additions
| `tls` | TLS with a pooled connection |
| `tlsshort` | a fresh TLS handshake per request |
+| `tlsverify` | pooled TLS with certificate verification enabled |
+| `tlsverifyshort` | a fresh verified TLS handshake per request | | `BENCH_LATENCY_FRACTION` | `0.6` | fraction of the slower path's QPS for that phase |
+| `BENCH_LATENCY_REPEATS` | `3` | repeats per case in the latency phase |
| `BENCH_LATENCY_MODE` | `corrected` | `corrected` or `uncorrected` percentiles |Also applies to: 376-408
🤖 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 `@benchmark/README.md` around lines 196 - 210, Update the benchmark
documentation matrix to include the tlsverify and tlsverifyshort shapes with
their TLS verification behaviors, and add BENCH_LATENCY_REPEATS to the
configuration table using its default value of 3. Keep the entries aligned with
the definitions in cases.txt, bench.lua, and run.sh.
| # a keepalive case that opens roughly one connection per request is not | ||
| # exercising the pool, whatever else the row says | ||
| case "$name" in | ||
| *.short|*.tlsshort) ;; | ||
| *) | ||
| if [[ "$rpc" != "inf" && -n "$rpc" ]] && | ||
| awk -v v="$rpc" 'BEGIN { exit !(v + 0 < 2) }' | ||
| then | ||
| warn "$name reused almost nothing (${rpc} requests per" \ | ||
| "connection); this is not a keepalive result" | ||
| fi | ||
| ;; | ||
| esac | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the new verify-short cases to the non-keepalive exclusion list.
The pattern list excludes *.short and *.tlsshort. ffi.tlsverifyshort and resty.tlsverifyshort end in short but not in .short, so they match neither pattern. Both shapes set keepalive = false in benchmark/lua/bench.lua lines 428-429, so their expected ratio is exactly 1.0 request per connection. The runner therefore prints "reused almost nothing ... this is not a keepalive result" for every window of both cases, which is a false warning on a deliberately non-keepalive shape.
benchmark/results-full.md line 223 records requests_per_connection as 1.0 for all four short-lived cases, which confirms the shape behaviour.
🐛 Proposed fix for the exclusion pattern
case "$name" in
- *.short|*.tlsshort) ;;
+ # every non-keepalive shape ends in "short"
+ *.short|*.tlsshort|*.tlsverifyshort) ;;
*)📝 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.
| # a keepalive case that opens roughly one connection per request is not | |
| # exercising the pool, whatever else the row says | |
| case "$name" in | |
| *.short|*.tlsshort) ;; | |
| *) | |
| if [[ "$rpc" != "inf" && -n "$rpc" ]] && | |
| awk -v v="$rpc" 'BEGIN { exit !(v + 0 < 2) }' | |
| then | |
| warn "$name reused almost nothing (${rpc} requests per" \ | |
| "connection); this is not a keepalive result" | |
| fi | |
| ;; | |
| esac | |
| fi | |
| # a keepalive case that opens roughly one connection per request is not | |
| # exercising the pool, whatever else the row says | |
| case "$name" in | |
| # every non-keepalive shape ends in "short" | |
| *.short|*.tlsshort|*.tlsverifyshort) ;; | |
| *) | |
| if [[ "$rpc" != "inf" && -n "$rpc" ]] && | |
| awk -v v="$rpc" 'BEGIN { exit !(v + 0 < 2) }' | |
| then | |
| warn "$name reused almost nothing (${rpc} requests per" \ | |
| "connection); this is not a keepalive result" | |
| fi | |
| ;; | |
| esac | |
| fi |
🤖 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 `@benchmark/run.sh` around lines 626 - 639, Extend the case exclusion pattern
in the non-keepalive warning logic to also match the verify-short benchmarks
`ffi.tlsverifyshort` and `resty.tlsverifyshort`, while preserving the existing
`*.short` and `*.tlsshort` exclusions. This prevents the reused-almost-nothing
warning for these deliberately non-keepalive cases.
| The plan assumes `apisix/apisix/plugins/ai-transport/http.lua`, "already a seam" | ||
| exposing `_M.request(params, timeout)` and `_M.set_keepalive(res, ...)`, behind | ||
| which an alternate transport is "roughly 30 lines". | ||
|
|
||
| There is no `ai-transport` directory in APISIX at `636100ec`. The outbound call | ||
| lives inline in `apisix/plugins/ai-drivers/openai-base.lua`, in `_M.request()`, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one repository-relative APISIX path convention.
Line 10 uses apisix/apisix/plugins/ai-transport/http.lua, while Line 15 uses apisix/plugins/ai-drivers/openai-base.lua. Remove the extra apisix/ prefix or explain the different root. Readers need one convention to locate the referenced files.
🤖 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 `@docs/ai-proxy-integration.md` around lines 10 - 15, Update the APISIX file
references in the documentation to use one repository-relative path convention,
removing the extra “apisix/” prefix from the `ai-transport/http.lua` reference
so it matches `apisix/plugins/ai-drivers/openai-base.lua`.
| ``` | ||
| _M.request(self, ctx, conf, request_table, extra_opts) | ||
| http.new() / set_timeout | ||
| fetch_gcp_access_token -- auth, may return early | ||
| url.parse(endpoint) -- scheme/host/port/path/query derivation | ||
| construct_forward_headers | ||
| self.request_filter(...) -- per-driver hook that mutates params | ||
| httpc:connect(params) | ||
| core.json.encode(params.body) | ||
| httpc:request(params) | ||
| if status == 429 or 5xx: return -- retry-relevant, before the body is read | ||
| read_response(...) -- SSE loop or buffered read | ||
| httpc:set_keepalive(conf.keepalive_timeout, conf.keepalive_pool) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to fenced code blocks.
markdownlint-cli2 reports MD040 for both blocks. Use lua for the APISIX trace and http or text for the raw headers.
Proposed fix
-```
+```lua
...
-```
+```httpAlso applies to: 134-137
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/ai-proxy-integration.md` around lines 18 - 31, Update both fenced code
blocks in the documentation: add the lua language identifier to the APISIX
request trace block and add http or text to the raw headers block, including the
additional block referenced by the review. Leave the block contents unchanged.
Source: Linters/SAST tools
| ### 3. No proxy support | ||
|
|
||
| `lua-resty-http` has `set_proxy_options` and honours `proxy_opts` in `connect` | ||
| and `request_uri`. This library has none. | ||
|
|
||
| ai-proxy's `openai-base.lua` does not use it, but `request_filter` hooks are | ||
| free to set it and APISIX deployments behind a corporate proxy will want it. | ||
| This is the largest single missing feature relative to `lua-resty-http`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'proxy_opts|set_proxy_options' \
apisix/plugins/ai-drivers/openai-base.lua || true
rg -n -C 12 'proxy_opts|set_proxy_options|function.*connect|connect' \
lib/resty/ngx_http_ffi_client.lua || trueRepository: api7/ngx_http_ffi_client
Length of output: 13329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(openai-base\.lua|ngx_http_ffi_client\.lua|ai-proxy-integration\.md)$'
printf '%s\n' '--- proxy references in tracked Lua/docs files ---'
rg -n -C 8 'proxy_opts|set_proxy_options|request_filter|openai-base|connect\s*\(' \
--glob '*.lua' --glob '*.md' . || trueRepository: api7/ngx_http_ffi_client
Length of output: 13011
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
client = Path("lib/resty/ngx_http_ffi_client.lua").read_text()
doc = Path("docs/ai-proxy-integration.md").read_text()
connect = client[client.index("function client.connect"):
client.index("\n\n\nfunction client.request", client.index("function client.connect"))]
connect_fields = re.search(r'-- connect\{([^}]*)\}', client, re.S).group(1)
assigned = set(re.findall(r'\bcp\.([A-Za-z_][A-Za-z0-9_]*)\s*=', connect))
print("connect_signature_fields:", " ".join(connect_fields.split()))
print("connect_struct_assignments:", sorted(assigned))
print("proxy_opts_in_connect:", "proxy_opts" in connect)
print("set_proxy_options_in_client:", "set_proxy_options" in client)
sequence = doc[doc.index("self.request_filter"):
doc.index("self.request_filter") + 180]
print("documented_hook_sequence:")
print(sequence)
PYRepository: api7/ngx_http_ffi_client
Length of output: 267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '540,670p' lib/resty/ngx_http_ffi_client.lua
printf '%s\n' '--- documented call sequence ---'
sed -n '19,31p' docs/ai-proxy-integration.mdRepository: api7/ngx_http_ffi_client
Length of output: 4356
Security Misconfiguration (CWE-16)
Reachability: Internal · Exploitability: Theoretical
Fail closed when proxy_opts is configured.
When request_filter adds proxy_opts, httpc:connect(params) passes it to client.connect, which ignores it before calling c_connect. The request can bypass the configured proxy. Document proxy-configured deployments as unsupported, or reject proxy_opts before connecting. Add a test that distinguishes proxy use from a direct connection.
🤖 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 `@docs/ai-proxy-integration.md` around lines 111 - 118, Update the proxy
handling around httpc:connect and client.connect so configured proxy_opts cannot
be silently ignored: either reject proxy_opts before c_connect or clearly
document proxy-configured deployments as unsupported, with rejection preferred.
Add coverage that distinguishes a proxied connection from a direct connection
and verifies the configured behavior.
1 participant
Benchmark harness rewrite, plus the results records it produced. No library code changes — nothing under
src/.Companion to #33, which carries the TLS trust-store fix this harness found. That PR touches only
src/; this one should land first, since the fix's numbers are reproduced with the cases added here.Why
The old harness hand-wrote one target location per client, so the two sides drifted into different API shapes and the published ratio credited that drift to the C-vs-Lua difference.
Every case is now a driver plus a shape, and both columns of a pair are generated from the same shape table — so they send the same headers, size the same pool and consume the body the same way by construction rather than by review. A fairness audit diffs the raw request each client actually put on the wire (via a
/mock/echoupstream returningngx.req.raw_header()) and fails the run if the caller-supplied headers disagree.benchmark/cases.txtis the single source of truth:run.shgenerates one location per name and the smoke check diffs it against the case table loaded in the worker, so the two cannot drift apart.What's in it
BENCH_REPEATS, default 5) and a separateBENCH_LATENCY_REPEATS, because a single 30 s window is not evidence for a tail number.fold.shto render it as markdown.stream {}block, since nginx's http server cannot emit them.tlsverify/tlsverifyshortcases. The matrix had never exercisedssl_verify = true— all four call sites hardcodedfalse— so the verifying path was entirely unmeasured.Results records
benchmark/results-full.md— the full-matrix run (29 cases x 5 repeats, 229 rows), plus the TLS re-measurement after the fix: stop rescanning the CA directory on every TLS handshake #33 fix.Two findings worth calling out from it:
ffi.statefulp99.9 claim of 20.94 ms is disproven. It came from a single 30 s window and was flagged at the time as not-yet-evidence. Three repeats give 2.720 / 2.730 / 2.730 ms againstresty.stateful's 3.320 ms — the FFI client wins the tail, it does not lose it.lua-resty-http:resty.trailersp99.9 of 109.570 ms againstffi.trailersat 2.600 ms.Known gaps
README.mdandbenchmark/README.mdstill carry pre-fix numbers and the disproven tail claim above. Corrected data for the whole matrix is inbenchmark/results-full.md; re-baselining the READMEs is deliberately left to a follow-up rather than bundled here.benchmark/results-full.md), so it looks like a harness or environment property rather than a client defect — but it is not yet diagnosed, and no latency claim should be published for the short-lived TLS shapes until it is.Summary by CodeRabbit
New Features
Documentation
Tests