chore(test): cleanup the QWP benchmark suite - #172
Conversation
📝 WalkthroughWalkthroughThe change adds a benchmark contract and aggregation CLI, introduces an AWS-based network benchmark rig, improves benchmark failure propagation and portability, wires the C self-test into builds and CI, and updates benchmark documentation. ChangesQWP benchmark suite
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Laptop
participant SSM
participant ClientBox
participant ServerBox
participant S3
Laptop->>SSM: send benchmark command
SSM->>ClientBox: execute benchmark
ClientBox->>ServerBox: send benchmark traffic
ClientBox->>S3: upload JSON report
Laptop->>S3: pull benchmark results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@doc/BENCHMARKS.md`:
- Around line 101-104: Update the top-level report field contract to explicitly
document machine and commits as mandatory fields emitted before paths, or
clearly mark them as producer-specific requirements so alternate report emitters
preserve the required provenance data.
In `@tools/qwp_bench/net/provision.sh`:
- Around line 54-56: Update the S3 bucket creation command in provision.sh to
omit --create-bucket-configuration when AWS_REGION is us-east-1, while retaining
the LocationConstraint for all other regions. Keep the existing BUCKET and
AWS_REGION values and bucket creation flow unchanged.
In `@tools/qwp_bench/net/teardown.sh`:
- Around line 4-6: Guard the directory change in teardown.sh by making the cd
command fail explicitly when it cannot enter the script directory. Keep set -uo
pipefail unchanged and ensure ./env.sh is sourced only after a successful cd.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7b497ba-de1c-4353-9606-0c41df903abf
📒 Files selected for processing (33)
.gitignoreCMakeLists.txtci/check_docs.pyci/run_all_tests.pydoc/BENCHMARKS.mddoc/README.mddoc/bench_parity_aggregate.pyexamples/bench_json_c.cexamples/bench_json_c.hexamples/qwp_egress_c.cexamples/qwp_ingress_c.cquestdb-rs/Cargo.tomlquestdb-rs/benches/column_sender.rsquestdb-rs/benches/decoder.rsquestdb-rs/examples/bench_json/mod.rsquestdb-rs/examples/bench_schema/mod.rsquestdb-rs/examples/qwp_egress_polars.rsquestdb-rs/examples/qwp_ingress_polars.rsquestdb-rs/examples/qwp_ingress_row.rsquestdb-rs/src/ingress/column_sender/encoder.rsquestdb-rs/src/ingress/column_sender/mod.rstools/qwp_bench/aggregate.pytools/qwp_bench/net/README.mdtools/qwp_bench/net/box_bootstrap_client.shtools/qwp_bench/net/box_bootstrap_server.shtools/qwp_bench/net/box_channel.shtools/qwp_bench/net/env.shtools/qwp_bench/net/provision.shtools/qwp_bench/net/run_cell.shtools/qwp_bench/net/ssmx.shtools/qwp_bench/net/teardown.shtools/qwp_bench/net/test_run_cell.shtools/qwp_bench/test_aggregate.py
💤 Files with no reviewable changes (1)
- doc/bench_parity_aggregate.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@doc/BENCHMARKS.md`:
- Around line 101-104: Update the top-level report field contract to explicitly
document machine and commits as mandatory fields emitted before paths, or
clearly mark them as producer-specific requirements so alternate report emitters
preserve the required provenance data.
In `@tools/qwp_bench/net/provision.sh`:
- Around line 54-56: Update the S3 bucket creation command in provision.sh to
omit --create-bucket-configuration when AWS_REGION is us-east-1, while retaining
the LocationConstraint for all other regions. Keep the existing BUCKET and
AWS_REGION values and bucket creation flow unchanged.
In `@tools/qwp_bench/net/teardown.sh`:
- Around line 4-6: Guard the directory change in teardown.sh by making the cd
command fail explicitly when it cannot enter the script directory. Keep set -uo
pipefail unchanged and ensure ./env.sh is sourced only after a successful cd.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7b497ba-de1c-4353-9606-0c41df903abf
📒 Files selected for processing (33)
.gitignoreCMakeLists.txtci/check_docs.pyci/run_all_tests.pydoc/BENCHMARKS.mddoc/README.mddoc/bench_parity_aggregate.pyexamples/bench_json_c.cexamples/bench_json_c.hexamples/qwp_egress_c.cexamples/qwp_ingress_c.cquestdb-rs/Cargo.tomlquestdb-rs/benches/column_sender.rsquestdb-rs/benches/decoder.rsquestdb-rs/examples/bench_json/mod.rsquestdb-rs/examples/bench_schema/mod.rsquestdb-rs/examples/qwp_egress_polars.rsquestdb-rs/examples/qwp_ingress_polars.rsquestdb-rs/examples/qwp_ingress_row.rsquestdb-rs/src/ingress/column_sender/encoder.rsquestdb-rs/src/ingress/column_sender/mod.rstools/qwp_bench/aggregate.pytools/qwp_bench/net/README.mdtools/qwp_bench/net/box_bootstrap_client.shtools/qwp_bench/net/box_bootstrap_server.shtools/qwp_bench/net/box_channel.shtools/qwp_bench/net/env.shtools/qwp_bench/net/provision.shtools/qwp_bench/net/run_cell.shtools/qwp_bench/net/ssmx.shtools/qwp_bench/net/teardown.shtools/qwp_bench/net/test_run_cell.shtools/qwp_bench/test_aggregate.py
💤 Files with no reviewable changes (1)
- doc/bench_parity_aggregate.py
🛑 Comments failed to post (3)
doc/BENCHMARKS.md (1)
101-104: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the mandatory
machineandcommitsfields.The Rust report serializer always emits
machineandcommitsbeforepaths(questdb-rs/examples/bench_json/mod.rs, Lines 523-524), but this contract list omits them. Add them here or explicitly document them as producer-specific so alternate report emitters do not omit required provenance fields.🤖 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 `@doc/BENCHMARKS.md` around lines 101 - 104, Update the top-level report field contract to explicitly document machine and commits as mandatory fields emitted before paths, or clearly mark them as producer-specific requirements so alternate report emitters preserve the required provenance data.tools/qwp_bench/net/provision.sh (1)
54-56: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
AWS s3api create-bucket us-east-1 LocationConstraint InvalidLocationConstraint💡 Result:
The InvalidLocationConstraint (or sometimes IllegalLocationConstraintException) error occurs when you explicitly provide us-east-1 as a LocationConstraint when creating an S3 bucket [1][2][3]. In Amazon S3, us-east-1 (US East, N. Virginia) is the default region [4][5][6]. Because of this historical design, the S3 API does not accept us-east-1 as a valid value for the LocationConstraint parameter [1][3]. How to resolve this: 1. When creating a bucket in us-east-1, do not provide any CreateBucketConfiguration parameter or LocationConstraint [1][3]. 2. For all other regions, you must specify the corresponding region in the LocationConstraint parameter [5][7]. Example for creating a bucket in us-east-1: aws s3api create-bucket --bucket YOUR-BUCKET-NAME Example for creating a bucket in a different region (e.g., us-west-2): aws s3api create-bucket --bucket YOUR-BUCKET-NAME --create-bucket-configuration LocationConstraint=us-west-2
Citations:
- 1: boto/boto3#125
- 2: aws/aws-cli#3044
- 3: https://stackoverflow.com/questions/54534424/unable-to-create-a-bucket-in-s3
- 4: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateBucket.html
- 5: https://docs.aws.amazon.com/cli/latest/reference/s3api/create-bucket.html
- 6: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateBucketConfiguration.html
- 7: https://stackoverflow.com/questions/58244128/an-error-occurred-illegallocationconstraintexception-when-calling-the-createbu
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Locate provision.sh ==" fd -a 'provision\.sh$' . | sed 's#^\./##' echo "== Inspect relevant create-bucket context ==" sed -n '1,90p' tools/qwp_bench/net/provision.sh echo "== Search region/config usage ==" rg -n "QNB_AWS_REGION|AWS_REGION|AWS_DEFAULT_REGION|LocationConstraint|create-bucket|us-east-1" -S .Repository: questdb/c-questdb-client
Length of output: 5196
Gate the S3
LocationConstraintforus-east-1.S3 rejects
--create-bucket-configuration LocationConstraint=us-east-1withInvalidLocationConstraint; the flag should only be used for regions other thanus-east-1. This can be hit wheneverQNB_AWS_REGION=us-east-1is used instead of the documentedeu-west-1, so provision will fail partway through setup.🤖 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 `@tools/qwp_bench/net/provision.sh` around lines 54 - 56, Update the S3 bucket creation command in provision.sh to omit --create-bucket-configuration when AWS_REGION is us-east-1, while retaining the LocationConstraint for all other regions. Keep the existing BUCKET and AWS_REGION values and bucket creation flow unchanged.tools/qwp_bench/net/teardown.sh (1)
4-6: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the
cd(noset -ehere to catch a failedcd).Unlike
provision.sh/ssmx.sh, this script runs without-e, so a failedcdwould continue and source/operate from the wrong directory. Add an explicit guard.🛡️ Proposed fix
-cd "$(dirname "$0")" +cd "$(dirname "$0")" || { echo "ERROR: cd to script dir failed" >&2; 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.set -uo pipefail cd "$(dirname "$0")" || { echo "ERROR: cd to script dir failed" >&2; exit 1; } . ./env.sh🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[info] 6-6: Not following: ./env.sh was not specified as input (see shellcheck -x).
(SC1091)
🤖 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 `@tools/qwp_bench/net/teardown.sh` around lines 4 - 6, Guard the directory change in teardown.sh by making the cd command fail explicitly when it cannot enter the script directory. Keep set -uo pipefail unchanged and ensure ./env.sh is sourced only after a successful cd.
jovfer
changed the title
1 participant
Summary
doc/totools/qwp_bench/Why
The merged benchmark suite was useful, but its live contract was scattered across source comments, an outdated aggregator, and an AWS runbook. Several active comments were stale; the aggregator silently omitted current C/row/Arrow paths, could mix incompatible workloads, accepted duplicate JSON members, and could apply one B/row override to multiple workload shapes.
run_cell.shcould report success after the benchmark failed, while a later cleanup, download, sidecar, or listing failure could mask the saved benchmark status and stop the remaining diagnostic sequence. It now attempts every post-benchmark step and returns failures in benchmark, JSON-validation, then post-processing order.Registering
qwp_bench_selftestin the normal cross-platform test graph also exposed POSIX-only timing helpers. The_WIN32path now usesQueryPerformanceCounterandGetProcessTimes; the existing POSIX implementation and measured timing boundaries remain unchanged.Scope
This is stabilization of the existing suite, not a benchmark redesign. It does not add metrics, workloads, timing-region changes, AWS cells, performance claims, automatic deployed-SHA verification, provisioning rollback, or infrastructure-teardown audit changes. The last three remain explicit operator responsibilities in the runbook.
Validation
python3 ci/check_docs.pycargo fmtand plaincargo clippy --testsqwp_bench_selftestwith tests enabledQUESTDB_QWP_BENCH=ON --target qwp_bench_selftestfallback with tests disabledci/run_all_tests.py cppandunitentry points, includingqwp_bench_selftest_WIN32compile and QPC/FILETIME conversion probeNot run
The existing AWS workflow had already been validated; this follow-up changes local validation, failure propagation, and documentation and is accepted with offline checks.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation