Establish performance baselines and regression detection - #3441
Establish performance baselines and regression detection#3441dheerajodha wants to merge 5 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
🤖 Finished Review · ✅ Success · Started 1:06 PM UTC · Completed 1:25 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Labels: PR adds a GitHub Actions workflow with command injection vulnerabilities in shell/Python interpolation Next steps:
Previous run (4)ReviewFindingsHigh
Medium
Low
Labels: PR adds CI benchmark workflow and benchmark infrastructure Next steps:
Previous run (5)ReviewFindingsMedium
Low
|
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 7:59 AM UTC · Completed 8:17 AM UTC Commit: |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ci
enhancement
|
🤖 Finished Review · ✅ Success · Started 12:10 PM UTC · Completed 12:28 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
Possible security concern
github_actions
|
🤖 Finished Review · ✅ Success · Started 12:54 PM UTC · Completed 1:09 PM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 1:24 PM UTC · Completed 1:42 PM UTC Commit: |
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
Add baseline.json with current benchmark metrics, thresholds.json with configurable regression limits (15% RSS, 20% time), and compare.sh to detect regressions. The CI workflow now compares results against the baseline and fails when thresholds are exceeded. Job summary shows current vs baseline with % change. A new `make benchmark_baseline` target regenerates the baseline from a local benchmark run. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Restore continue-on-error on the job so benchmarks inform but don't block merges - Add continue-on-error on the compare step so regressions are reported in the summary without failing the job - Replace stderr suppression (2>/dev/null) with capture to file in the Makefile baseline target - Fix default workers fallback from 35 to 10 in the Makefile - Update baseline with latest CI run measurements Resolves: EC-1819 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace grep -oP with python3 for macOS compatibility - Add validation for empty parsed metrics in compare.sh and Makefile - Remove continue-on-error from job and compare step so regressions fail the check; keep it on the bench run step for crash resilience - Rename execution_time_ns to ns_per_op for accuracy - Rename make target from benchmark_baseline to generate_baseline - Document baseline workflow in benchmark/README.md Resolves: EC-1819 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pass benchmark output to Python via environment variables instead of interpolating into triple-quoted strings, preventing command injection if the benchmark binary emits crafted output. Skip baseline comparison when the benchmark step crashes to avoid conflating infrastructure failures with regressions. Guard against division by zero in baseline comparison. Rename generate_baseline to generate-baseline to match the Makefile's hyphenated naming convention. EC-1819 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update AGENTS.md and the benchmark skill with stress benchmark make targets, baseline comparison workflow, and baseline regeneration. EC-1819 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 Finished Review · ✅ Success · Started 12:32 PM UTC · Completed 12:48 PM UTC Commit: |
| .PHONY: benchmark | ||
| benchmark: benchmark_simple ## Run benchmarks | ||
|
|
||
| .PHONY: generate-baseline |
There was a problem hiding this comment.
[medium] logic-error
The generate-baseline target runs go run . without setting EC_STRESS_WORKERS or EC_STRESS_COMPONENTS. The Go benchmark defaults to 35 workers, but the inline Python records workers=10 (bash fallback ${EC_STRESS_WORKERS:-10}). CI uses EC_STRESS_WORKERS=10 for comparison. This produces a baseline with incorrect metadata and metrics from a different workload than CI, making regression detection unreliable.
Suggested fix: Set EC_STRESS_COMPONENTS=10 EC_STRESS_WORKERS=10 in the generate-baseline recipe to match CI values.
| peak_rss=$(echo "$line" | grep -oP '[\d.]+ peak-RSS-bytes' | awk '{print $1}') | ||
| alloc=$(echo "$line" | grep -oP '[\d.]+ allocated-bytes/op' | awk '{print $1}') | ||
| heap=$(echo "$line" | grep -oP '[\d.]+ heap-bytes-from-system' | awk '{print $1}') | ||
| read -r ns_op peak_rss alloc heap < <(BENCH_LINE="$line" python3 -c " |
There was a problem hiding this comment.
[low] scope-creep
The PR refactors existing grep/awk benchmark output parsing to python3 in the job summary step, which is tangential to the stated goal but establishes consistency with compare.sh.
| .PHONY: benchmark | ||
| benchmark: benchmark_simple ## Run benchmarks | ||
|
|
||
| .PHONY: generate-baseline |
There was a problem hiding this comment.
[low] inline-scripting-convention
The generate-baseline target embeds ~15 lines of inline Python. The codebase convention is to use standalone scripts for complex logic (e.g., prepare_data.sh, push_data.sh).
| b = json.load(open(os.environ['BASELINE_PATH'])) | ||
| t = json.load(open(os.environ['THRESHOLDS_PATH'])) | ||
| print(ns, rss, b['ns_per_op'], b['peak_rss_bytes'], t['peak_rss_percent'], t['ns_per_op_percent']) | ||
| " |
There was a problem hiding this comment.
[low] edge-case
Zero-value guard uses misleading awk variable names (b for $baseline_rss, t for $baseline_ns).
| if [[ ! -f "$THRESHOLDS" ]]; then | ||
| echo "No thresholds file found, skipping comparison." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
[low] edge-case
If benchmark output contains multiple BenchmarkStress lines, grep captures all; Python re.search matches only the first. Works correctly for expected single-result case but is fragile.
| ./stress 2>benchmark-stderr.txt | tee benchmark-output.txt | ||
|
|
||
| - name: Compare against baseline | ||
| id: compare |
There was a problem hiding this comment.
[low] error-handling
The compare step is silently skipped when the benchmark fails (continue-on-error: true on the bench step), with no indication in the job summary.
| @@ -197,6 +197,28 @@ benchmark_data: benchmark/simple/data.tar.gz ## Prepare data for benchmark | |||
| .PHONY: benchmark | |||
| benchmark: benchmark_simple ## Run benchmarks | |||
|
|
|||
There was a problem hiding this comment.
[low] naming-convention
Target name generate-baseline uses hyphens; sibling benchmark targets use underscores (benchmark_simple, benchmark_data).
fullsend-ai-review
Bot
removed
the
requires-manual-review
1 participant
What:
Store benchmark baselines in the repo and compare CI results against them. Fail the benchmark check when regressions exceed configurable thresholds.
Why:
EC-1818 added a report-only stress benchmark to CI. This follow-up closes the loop by detecting regressions automatically, without baselines and thresholds, the benchmark runs but nobody notices when performance degrades.
Tickets:
EC-1819