fix(benchmarks): make the k6 suite run to completion and surface skips - #123
Conversation
…ode-aware Two independent defects kept the passthroughRelay goal red and its empty-mode sibling from running at all. 1. Unresolvable target host. passthrough_relay.js defaulted PASSTHROUGH_TARGET_URL to https://passthrough.api-sheriff:8443/get, a name no compose file declares. Convert the api-sheriff service's networks short-form list to map form and add the alias passthrough.test.example, then repoint the script default at it. gateway.yaml already maps passthrough.test.example to PASSTHROUGH_BACKEND under tls.passthrough_sni, and the backend certificate already carries the name as a SAN, so no config change is required. 2. Unpropagated mode selector. benchmarks/pom.xml sets PASSTHROUGH_SNI=mapped and PASSTHROUGH_SNI=empty per execution, but the k6 service's environment pass-through block never listed the variable, so neither run received it. Add a value-less PASSTHROUGH_SNI entry so an unset variable stays unset. Both edits land together: fixing only the alias would move the failure from the k6 run to post-integration-test, where PassthroughBaselineComparator throws on the missing passthroughRelayEmpty summary. Co-Authored-By: Claude <noreply@anthropic.com>
session_mediated.js sits in k6-scripts/ backing no Maven goal and appearing in no documentation, so a reader cannot tell "deliberately not run" from "forgotten". Its benchmark name sessionMediated is already a public constant with test coverage, so the gap is documentation, not dead code. Add a short "Deliberately unwired scripts" subsection naming the script, its benchmark name, and the reason verbatim: BFF session mediation belongs to PLAN-07A, which shipped the server-session variant and explicitly deferred its per-variant benchmark additions because benchmarks/** is PLAN-25's surface. The script itself is untouched and no thirteenth Maven goal is added. Co-Authored-By: Claude <noreply@anthropic.com>
Maven fails fast, so a goal that errors aborts every goal after it and a goal that is never reached leaves no trace in the job output. The job's only artefact check was an `ls` that asserts nothing about what SHOULD be present, so a suite that executed 3 of 12 goals rendered exactly like one that executed all 12 -- which is how goals 5-12 stayed unexecuted and unnoticed. Add an `if: always()` "Summarise benchmark coverage" step that diffs the goals expected to write a summary against the documents k6 actually wrote, renders a ran / DID NOT RUN table into $GITHUB_STEP_SUMMARY, and exits non-zero when a summary is absent while the benchmark step itself reported success. When Maven already failed the step stays green so it never masks the actionable error. Deliberately skipped goals are named with their reason instead of omitted, so "not run" is never indistinguishable from "forgotten": uploadLarge and websocketEcho (both blocked on gateway-side defects) and sessionMediated (owned by PLAN-07A). Verified locally against a synthetic results tree: all present -> exit 0; one missing with a green benchmark step -> exit 1; one missing with a failed benchmark step -> exit 0. Each expected name renders exactly once. Co-Authored-By: Claude <noreply@anthropic.com>
benchmarks/README.adoc documented the lane as designed rather than as it runs. Nothing stated that any goal had never executed, and three counts contradicted benchmarks/pom.xml. Add a "Which goals actually run" section giving every one of the twelve goals a disposition: runs, skipped with the blocking reason, or unwired. uploadLarge and websocketEcho are named as skipped with their gateway-side root causes rather than quietly omitted, matching the skip list the new CI coverage step renders. Reconcile the stale counts: --aspects said "six matrix aspects" where the matrix has eight, and the wrk-to-k6 coverage note said the lane grew "to eight" where the pom declares twelve. Add the distinction that being wired is not the same as executing, which is precisely how goals 5-12 stayed unrun and unnoticed. Extend the discontinuity note to name two further causes separately so they cannot be conflated: goals executing for the first time under this plan (a coverage cause), and the forthcoming PLAN-31 D6/D7 %it elimination (a configuration cause, at a later commit, on the same names). Three-layer documentation verdicts, per the standing clause: - benchmarks/README.adoc: CHANGED (this commit). - doc/user/: NO CHANGE REQUIRED -- zero benchmark references; nothing an operator sees changes. - doc/development/: NO CHANGE REQUIRED -- its three benchmark references are structural (module name, build commands, BFF entry points); none is altered. Co-Authored-By: Claude <noreply@anthropic.com>
…ology that do not exist Both comments assert facts contradicted by the code beside them. benchmark.yml claimed in two places that the lane runs eight executions -- the timeout-minutes rationale and the run-step comment -- while benchmarks/pom.xml declares twelve k6 goals. The wrong count was load-bearing: it was the stated justification for the 75-minute cap. Restate both against the real composition (eight matrix aspects, two retained health benchmarks, two passthrough-relay executions) and rebuild the timeout rationale from the wall time measured locally on 2026-07-28 rather than from an estimate, naming the four native gateway instances the lane has booted since #118. The cap itself stays at 75: the measured run does not approach it. passthrough_relay.js documented PASSTHROUGH_TARGET_URL by pointing at the benchmark compose overlay "(D4)" for both the SNI mapping and the TLS backend. The overlay provides neither -- it adds only nginx-static and an upstream override. Name the three real sources instead: the network alias in the base docker-compose.yml, tls.passthrough_sni in gateway.yaml, and the passthrough-backend service in the base compose file. Comment-only on both files: the PASSTHROUGH_TARGET_URL value is unchanged and the coverage step added by the previous commit is intact. Co-Authored-By: Claude <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughThe benchmark profile now declares a gRPC unary goal and default skip gates for two blocked goals. CI validates expected summary files and documents coverage, while passthrough relay hostname, Docker aliasing, SNI forwarding, and benchmark history guidance are updated. ChangesBenchmark CI and relay execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
PR Reviewer Guide 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.github/workflows/benchmark.yml:
- Around line 27-44: Distinguish the twelve declared goal dispositions from the
ten benchmarks that produce CI results: in .github/workflows/benchmark.yml lines
27-44, base the k6-window budget on ten executed goals and identify uploadLarge
and websocketEcho as skipped; in .github/workflows/benchmark.yml lines 102-108,
describe twelve declared goals/dispositions rather than twelve executions; in
benchmarks/README.adoc lines 489-492, distinguish wired goals from
result-producing benchmarks; and in benchmarks/README.adoc lines 505-508,
exclude the skipped goals from the claim that trend history begins with PLAN-25.
🪄 Autofix (Beta)
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: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 65c7451b-2180-4ecb-a543-add6cb27ca4e
📒 Files selected for processing (4)
.github/workflows/benchmark.ymlbenchmarks/README.adocbenchmarks/src/main/resources/k6-scripts/passthrough_relay.jsintegration-tests/docker-compose.yml
Triage dispositionsIn reply to comment_id:
|
…ed from result-producing Maven runs same-phase exec executions in POM declaration order, and the -Pbenchmark profile ended with uploadSmall, uploadLarge, websocketEcho, grpcUnary. uploadLarge fails (413) and websocketEcho poisons the gateway, so fail-fast aborted the build before grpcUnary was ever reached -- grpcUnary was structurally unreachable in CI despite being in the coverage step's expected set and green against a fresh gateway. Move run-k6-upload-large-benchmark and run-k6-websocket-echo-benchmark to the end of the profile's exec list, after run-k6-grpc-unary-benchmark. Pure reorder: no id, phase, goal, configuration, environmentVariables, timeout or script path changed, and no execution added or removed. Twelve k6 executions remain. Also correct the prose that conflated two states the coverage step already keeps apart: twelve goals are DECLARED, ten are expected to produce a CI result. uploadLarge and websocketEcho are declared but settled as deliberately skipped against open gateway defects, so they start no trend series, and the claim that all eight previously-unexecuted goals begin one was inaccurate for those two. The timeout budget stays sized on all twelve wired goals -- a goal skipped by disposition is still a wired Maven execution that consumes wall time before it fails -- and now says so. Addresses PR #123 review finding 0938d0. Co-Authored-By: Claude <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/benchmark.yml:
- Around line 131-133: Update the benchmark inventory logic around the expected
manifest so it is derived from or validated against the authoritative Maven
executions in benchmarks/pom.xml. Ensure the combined expected and deliberately
skipped entries exactly covers the build-defined goals, rejecting omitted,
renamed, or unknown executions while preserving the distinction between expected
and skipped results.
- Around line 152-160: Update the “Deliberately skipped” handling in the
benchmark workflow to check for uploadLarge-summary.json,
websocketEcho-summary.json, and sessionMediated-summary.json. When any file
exists, mark that benchmark as having run unexpectedly and fail the job or
require an explicit disposition update instead of reporting it as skipped.
- Around line 102-108: Add execution-level skip configuration for the
uploadLarge and websocketEcho benchmark goals in the Maven profile so they are
treated as real skipped executions and cannot fail-fast before later goals.
Apply the same skip handling to sessionMediated if it remains listed as
skip-only. Keep the workflow coverage table synchronized with the POM’s skipped
execution set.
🪄 Autofix (Beta)
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: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 005aa47a-fab7-4ede-9b43-ca680a67bfda
📒 Files selected for processing (4)
.github/workflows/benchmark.ymlbenchmarks/README.adocbenchmarks/src/main/resources/k6-scripts/passthrough_relay.jsintegration-tests/docker-compose.yml
…cumenting it The suite documented uploadLarge and websocketEcho as skipped but implemented no skip: both remained unconditional exec executions, so uploadLarge still breached its threshold at goal 11, Maven still aborted fail-fast, websocketEcho at goal 12 still never ran, and the Performance Benchmark job was still red. The earlier reorder moved the failure from goal 10 to goal 11; it did not remove it. Give each blocked goal a real <skip> bound to its own property, both declared true in benchmarks/pom.xml with the id of the gateway finding that blocks it (0562be for uploadLarge, 218b5c for websocketEcho) and the flag that re-enables it. The reorder is kept as defense-in-depth: if a skip property is ever flipped to false, a resulting abort still cannot discard goals that would otherwise pass. Also make a truncated suite reportable. The coverage guard could only hard-fail when Maven had SUCCEEDED, so in the case that actually occurred - Maven fails and truncates the suite - the branch was suppressed by design and the reader saw a Maven error beside a table of DID NOT RUN rows with nothing stating the two were the same event. Add, on the failure path, an explicit statement distinguishing a truncated suite from a failure after every goal produced a summary. The existing missing>0 AND success hard-fail branch is left intact, and the new path does not exit non-zero: the Maven step's own failure already fails the job. Correct the prose in benchmark.yml and README.adoc that claimed the skipped goals still execute and consume wall time - false once they are genuinely skipped - including the timeout-budget rationale, which is now sized on the ten goals that actually execute rather than on twelve wired ones. The two gateway defects themselves remain reported, not fixed.
… coverage step The truncation-reporting branch re-scanned `expected` purely to rebuild `missing_names`, duplicating the predicate the primary coverage loop already evaluates. Accumulate `missing_names` alongside `missing` in that single loop and drop the second scan. Behaviour-preserving: same predicate, same branch, same value at the point of use. Co-Authored-By: Claude <noreply@anthropic.com>
Triage dispositionsIn reply to comment_id:
|
…ble hint The preference-emitter promoted a recurring within-plan disposition: seven review-bot comments were accepted rather than actioned, and the decisive reason in the largest case was that the bot re-reported its original diff range on a later sweep, grading code it had never seen. Records that as an api-sheriff-parent insight so the next plan ground-truths a bot verdict against current HEAD before opening a fix task, and expects one self-ingested no-op finding per review sweep. Co-Authored-By: Claude <noreply@anthropic.com>
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 `@benchmarks/pom.xml`:
- Around line 31-49: Update benchmarks/pom.xml:31-49 to emit skipped-goal state
in a shared generated manifest, extend benchmarks/pom.xml:529-556 to register
the gRPC unary benchmark, and revise the CI execution gates in
benchmarks/pom.xml:558-618 to derive required versus skipped expected results
from that manifest. Replace the parallel hardcoded expectations in
.github/workflows/benchmark.yml with consumption of the generated manifest so
benchmark additions and skip changes are reflected automatically.
🪄 Autofix (Beta)
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: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f5cae0f0-eb46-4168-80a8-200f47cd98d1
📒 Files selected for processing (4)
.github/workflows/benchmark.yml.plan/project-architecture/api-sheriff-parent/enriched.jsonbenchmarks/README.adocbenchmarks/pom.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- benchmarks/README.adoc
- .github/workflows/benchmark.yml
| <!-- Per-aspect skips for k6 goals blocked by an open gateway defect. Both default to | ||
| true, so -Pbenchmark runs the ten aspects that can pass and genuinely does not | ||
| execute these two. Flip an individual goal back on with | ||
| -Dskip.benchmark.upload.large=false (likewise .websocket.echo) once its finding | ||
| below is fixed; that is also how you reproduce the defect through the harness. --> | ||
|
|
||
| <!-- uploadLarge: the upload anchor's max_body_bytes (64 MiB) is silently capped at the | ||
| Quarkus default quarkus.http.limits.max-body-size (10 MiB), so the 50MB body is | ||
| rejected with 413 before the gateway's security filter is ever consulted. The k6 | ||
| script's 'status is 200' check therefore drops to a 0 rate, breaching its | ||
| threshold and failing the goal. Finding 0562be. Flip to false once the gateway | ||
| derives the HTTP limit from the anchor. --> | ||
| <skip.benchmark.upload.large>true</skip.benchmark.upload.large> | ||
|
|
||
| <!-- websocketEcho: each WebSocket upgrade leaks an edge admission permit — the permit | ||
| is released via ctx.addEndHandler, which a WS upgrade never fires — so once the | ||
| permits are exhausted the gateway 503s all traffic until it is restarted. | ||
| Finding 218b5c. Flip to false once the permit is released on the upgrade path. --> | ||
| <skip.benchmark.websocket.echo>true</skip.benchmark.websocket.echo> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Derive CI coverage expectations from the benchmark registry.
The POM is the runtime source for enabled goals and skip states, while the PR retains separate expected-result declarations in .github/workflows/benchmark.yml. Adding gRPC or changing a skip can therefore cause false CI failures or leave a goal unvalidated. Make the workflow consume a generated manifest from the effective benchmark configuration instead of maintaining a parallel list.
benchmarks/pom.xml#L31-L49: include skipped-goal state in the shared manifest.benchmarks/pom.xml#L529-L556: include the gRPC unary goal in the shared manifest.benchmarks/pom.xml#L558-L618: have CI derive skipped versus required results from these execution gates.
As per path instructions, “Treat a hardcoded list that must mirror a set defined elsewhere ... as a defect unless it is derived from that source at build or run time.”
📍 Affects 1 file
benchmarks/pom.xml#L31-L49(this comment)benchmarks/pom.xml#L529-L556benchmarks/pom.xml#L558-L618
🤖 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 `@benchmarks/pom.xml` around lines 31 - 49, Update benchmarks/pom.xml:31-49 to
emit skipped-goal state in a shared generated manifest, extend
benchmarks/pom.xml:529-556 to register the gRPC unary benchmark, and revise the
CI execution gates in benchmarks/pom.xml:558-618 to derive required versus
skipped expected results from that manifest. Replace the parallel hardcoded
expectations in .github/workflows/benchmark.yml with consumption of the
generated manifest so benchmark additions and skip changes are reflected
automatically.
Source: Path instructions
1 participant
Summary
Makes the
Performance BenchmarkGitHub Actions job (.github/workflows/benchmark.yml, jobbenchmark) run to completion for the first time, and makes a never-executed benchmark goal impossible to mistake for a passing one. Fixes the passthrough-relay benchmark's unresolvable target host, settles the disposition of every one of the twelve k6 goals, adds a fail-fast end-of-job summary, and reconciles stale prose across the benchmark docs.Changes (per deliverable)
1. Fix the passthrough-relay benchmark's unresolvable target host and its unpropagated mode selector
integration-tests/docker-compose.ymladdspassthrough.test.exampleas a network alias on theapi-sheriffservice, andbenchmarks/src/main/resources/k6-scripts/passthrough_relay.jsis repointed so the mapped-mode target drives that name instead of the previously-unresolvablepassthrough.api-sheriff. The existingtls.passthrough_snientry now matches, the front listener L4-relays topassthrough-backend:8443, and the run measures the real relay instead of a 404. This fixes bothpassthroughRelayandpassthroughRelayEmpty.2. Execute benchmark goals 5-12 and settle every one as pass, fix, or recorded skip
Produced zero file mutations by design — its 14 declared steps were an upper bound, and discovery found no harness defect on the remaining goals requiring an edit. All twelve goals were run and each was given a final, recorded disposition (see the per-goal disposition table below). Two real gateway defects were discovered and reported rather than fixed, per the plan's hard escalation boundary (no changes to
api-sheriff/src/main/java/**orintegration-tests/src/test/**).3. Record
session_mediated.jsas a deliberately unwired benchmark owned by PLAN-07Abenchmarks/README.adocgains a=== Deliberately unwired scriptssubsection recordingsession_mediated.jsas explicitly skipped, reason: BFF session mediation belongs to PLAN-07A. It is not wired as a 13th Maven goal.4. Add an end-of-job ran/not-ran summary step to the benchmark workflow
.github/workflows/benchmark.ymlgains an end-of-job summary step (if: always(), since Maven fails fast) that names which of the twelve benchmarks produced a result document and which did not, plus listssession_mediated.jsas deliberately unwired. The job now fails when an expected benchmark never ran, so a partially-executed suite can never read as "all green".5. Update
benchmarks/README.adocand apply the three-layer documentation conventionbenchmarks/README.adocstates which of the twelve goals are real (executing and producing a summary) and which are skipped and why, reconciles the stale "eight executions" / "six matrix aspects" counts against the actual twelve goals and eight aspects, and states that the trend series for every newly-executing benchmark starts empty (naming both causes: first-time execution under this plan, and the forthcoming PLAN-31 D6/D7%itelimination).Three-layer documentation verdict (explicit, per the epic's convention):
benchmarks/README.adoc— CHANGED (see above).doc/user/README.adoc— NO CHANGE. Zero benchmark references in this file.doc/development/README.adoc— NO CHANGE. Its three references to "benchmarks" are structural only (naming thebenchmarksMaven module and its role in the overall build/module layout), not behavioral documentation this plan alters.6. Fix the stale prose in
benchmark.ymland the incorrect doc comment inpassthrough_relay.js.github/workflows/benchmark.yml's job-summary prose no longer claims "eight executions" / "six matrix aspects" against a pom that declares twelve goals. The doc comment atpassthrough_relay.js:32-36, which incorrectly asserted the benchmark overlay provides the SNI mapping and the TLS backend, is corrected to state that the base compose file andgateway.yamlprovide both — not the overlay.Per-goal disposition (all twelve k6 goals)
healthLiveCheckgatewayHealthproxiedStaticpassthroughRelaypassthroughRelayEmptybearerProxiedhttp2graphqluploadSmallgrpcUnarywebsocketEchoGatewayEdgeRoute.java:347-351): the permit is released viactx.addEndHandler, which a WebSocket upgrade never fires. After any WS traffic the gateway silently rejects all requests with 503 until restart. Reproduced twice. This is also whygrpcUnaryappeared to fail 100% in CI — it runs after goal 11 (websocketEcho) in the pom's exec ordering, so it inherits a gateway already wedged by the WS leak. On a fresh gateway,grpcUnaryiserror_rate: 0.uploadLargemax_body_bytes: 67108864(64 MiB) is silently capped at Quarkus' 10 MiB default. Bisected: a 10,000,000-byte body passes; an 11,000,000-byte body returns413in ~8ms.Both skips are outside this plan's write boundary (
api-sheriff/**is hard off-limits) and are reported as findings for gateway-side follow-up, not fixed here.Third finding — recorded, deliberately not fixed
The benchmark overlay's
TOPOLOGY_UPSTREAMoverride is a proven no-op:TopologyResolver's javadoc states there is noTOPOLOGY_<ALIAS>env-precedence path. Benchmarks therefore proxy togo-httpbinrather than the static-fairness backend. Fixing it is in-boundary but would switch four currently-passing goals to a different backend and invalidate the stored PLAN-04proxiedStaticbaseline — reported for a follow-up plan that sequences the switch with a deliberate baseline restart, rather than fixed silently inside this one.Acceptance criterion (post-merge, not this PR)
The acceptance criterion for this plan is the post-merge
Performance Benchmarkrun against this PR's own merge commit — that is the only place the full twelve-goal suite executes end to end (the workflow triggers only on a merged PR, a tag, or manual dispatch, never on the PR itself). A green PR here does not satisfy acceptance. The plan's completion report will carry the post-merge run id, its conclusion, and this same per-goal disposition table.PLAN-25 landing plus its post-merge run completing is a hard precondition for PLAN-08B (the 0.1.0 release cut).
Test Plan
verify -Pbenchmark -pl benchmarks -am(local compose run, dev-loop signal only)verify -Ppre-commit -pl benchmarks -amPerformance BenchmarkGitHub Actions run (the actual acceptance gate — pending merge)Generated by plan-finalize skill
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores