-
Notifications
You must be signed in to change notification settings - Fork 61
Establish performance baselines and regression detection #3441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ef95caa
c77580a
ca57e6b
1cc8af2
6e7e586
a1c3e5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,6 @@ jobs: | |
| name: Stress Benchmark | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
| continue-on-error: true | ||
| env: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] scope-creep The operational impact of converting the benchmark from advisory to blocking is not explicitly called out in the PR description. The PR title implies regression detection, but the enforcement posture change could surprise operators. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] CI-coverage-regression The PR removes job-level continue-on-error and adds it only to the bench step. The new compare step does NOT have continue-on-error: true. A regression detected by compare.sh (exit non-zero) will now fail the overall PR check. Given CI runner variability (CPU contention, memory pressure), legitimate PRs could be blocked by noisy benchmark results. Suggested fix: Add continue-on-error: true to the compare step, or keep job-level continue-on-error until the mechanism is proven stable. |
||
| # Tuned for 4 vCPU / 16 GB CI runners to complete within 5 minutes. | ||
|
dheerajodha marked this conversation as resolved.
|
||
| # Code defaults are 10 components / 35 workers. | ||
|
|
@@ -73,11 +72,23 @@ jobs: | |
|
|
||
| - name: Run stress benchmark | ||
| id: bench | ||
| continue-on-error: true | ||
| run: | | ||
| set -o pipefail | ||
| cd benchmark/stress | ||
| ./stress 2>benchmark-stderr.txt | tee benchmark-output.txt | ||
|
|
||
| - name: Compare against baseline | ||
| id: compare | ||
|
dheerajodha marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] error-handling-gap The Compare against baseline step only runs when steps.bench.outcome == success. The Write job summary step handles the skipped state via if: always() and the steps.compare.outcome == skipped check. The interaction works correctly as designed. |
||
| if: steps.bench.outcome == 'success' | ||
| run: | | ||
| cd benchmark/stress | ||
| if [[ -f baseline.json ]]; then | ||
| ./compare.sh benchmark-output.txt | ||
| else | ||
| echo "No baseline found, skipping comparison." | ||
| fi | ||
|
|
||
| - name: Write job summary | ||
| if: always() | ||
| run: | | ||
|
|
@@ -103,25 +114,58 @@ jobs: | |
| exit 0 | ||
| fi | ||
|
|
||
| ns_op=$(echo "$line" | grep -oP '[\d.]+ ns/op' | awk '{print $1}') | ||
| 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 " | ||
|
dheerajodha marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] code-duplication Benchmark output parsing (regex extraction of ns/op, peak-RSS-bytes) is duplicated across compare.sh, the workflow summary step, and the Makefile target. If the benchmark output format changes, all three must be updated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] code-organization Benchmark output parsing logic (regex extraction of ns/op, peak-RSS-bytes, etc.) is duplicated in three places: the workflow summary step, compare.sh, and the Makefile generate-baseline target. Each uses nearly identical regex patterns and extraction functions. Suggested fix: Extract the parsing into a shared helper (e.g., benchmark/stress/parse.py) and call it from all three locations. |
||
| import os, re | ||
|
dheerajodha marked this conversation as resolved.
dheerajodha marked this conversation as resolved.
|
||
| line = os.environ['BENCH_LINE'] | ||
| def val(p): | ||
| m = re.search(p, line) | ||
| return m.group(1) if m else '0' | ||
| print(val(r'([\d.]+)\s+ns/op'), val(r'([\d.]+)\s+peak-RSS-bytes'), val(r'([\d.]+)\s+allocated-bytes/op'), val(r'([\d.]+)\s+heap-bytes-from-system')) | ||
| ") | ||
|
|
||
| secs=$(awk -v val="${ns_op:-0}" 'BEGIN {printf "%.1f", val / 1000000000}') | ||
| rss_mb=$(awk -v val="${peak_rss:-0}" 'BEGIN {printf "%.0f", val / 1048576}') | ||
| alloc_mb=$(awk -v val="${alloc:-0}" 'BEGIN {printf "%.0f", val / 1048576}') | ||
| heap_mb=$(awk -v val="${heap:-0}" 'BEGIN {printf "%.0f", val / 1048576}') | ||
|
|
||
| has_baseline=false | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] error-handling-gap The job summary's baseline comparison divides by bl_rss and bl_ns without a zero-value guard. compare.sh has this guard (line 69); the workflow summary code path does not. |
||
| if [[ -f benchmark/stress/baseline.json ]]; then | ||
| has_baseline=true | ||
| bl_rss=$(python3 -c "import json; print(json.load(open('benchmark/stress/baseline.json'))['peak_rss_bytes'])") | ||
| bl_ns=$(python3 -c "import json; print(json.load(open('benchmark/stress/baseline.json'))['ns_per_op'])") | ||
|
dheerajodha marked this conversation as resolved.
|
||
| bl_rss_mb=$(awk -v val="$bl_rss" 'BEGIN {printf "%.0f", val / 1048576}') | ||
| bl_secs=$(awk -v val="$bl_ns" 'BEGIN {printf "%.1f", val / 1000000000}') | ||
| rss_change=$(awk -v cur="$peak_rss" -v base="$bl_rss" 'BEGIN {printf "%+.1f", ((cur - base) / base) * 100}') | ||
| time_change=$(awk -v cur="$ns_op" -v base="$bl_ns" 'BEGIN {printf "%+.1f", ((cur - base) / base) * 100}') | ||
| fi | ||
|
|
||
| { | ||
| echo "## Stress Benchmark" | ||
| echo "" | ||
| echo "| Metric | Value | Description |" | ||
| echo "|--------|-------|-------------|" | ||
| echo "| Components | ${EC_STRESS_COMPONENTS} | Snapshot components validated |" | ||
| echo "| Workers | ${EC_STRESS_WORKERS} | Parallel validation workers |" | ||
| echo "| Execution time | ${secs}s | Wall-clock time per iteration |" | ||
| echo "| Peak RSS | ${rss_mb} MB | Max physical memory used |" | ||
| echo "| Allocated memory | ${alloc_mb} MB | Total Go heap allocations |" | ||
| echo "| Heap from system | ${heap_mb} MB | Heap memory requested from OS |" | ||
| if [[ "$has_baseline" == "true" ]]; then | ||
| echo "| Metric | Current | Baseline | Change | Description |" | ||
| echo "|--------|---------|----------|--------|-------------|" | ||
| echo "| Components | ${EC_STRESS_COMPONENTS} | | | Snapshot components validated |" | ||
| echo "| Workers | ${EC_STRESS_WORKERS} | | | Parallel validation workers |" | ||
| echo "| Execution time | ${secs}s | ${bl_secs}s | ${time_change}% | Wall-clock time per iteration |" | ||
| echo "| Peak RSS | ${rss_mb} MB | ${bl_rss_mb} MB | ${rss_change}% | Max physical memory used |" | ||
| echo "| Allocated memory | ${alloc_mb} MB | | | Total Go heap allocations |" | ||
| echo "| Heap from system | ${heap_mb} MB | | | Heap memory requested from OS |" | ||
| else | ||
| echo "| Metric | Value | Description |" | ||
| echo "|--------|-------|-------------|" | ||
| echo "| Components | ${EC_STRESS_COMPONENTS} | Snapshot components validated |" | ||
| echo "| Workers | ${EC_STRESS_WORKERS} | Parallel validation workers |" | ||
| echo "| Execution time | ${secs}s | Wall-clock time per iteration |" | ||
| echo "| Peak RSS | ${rss_mb} MB | Max physical memory used |" | ||
| echo "| Allocated memory | ${alloc_mb} MB | Total Go heap allocations |" | ||
| echo "| Heap from system | ${heap_mb} MB | Heap memory requested from OS |" | ||
| fi | ||
| if [[ "${{ steps.compare.outcome }}" == "failure" ]]; then | ||
| echo "" | ||
| echo "> **⚠️ Performance regression detected.** Update the baseline with \`make generate-baseline\` if this is expected." | ||
| elif [[ "${{ steps.compare.outcome }}" == "skipped" ]]; then | ||
| echo "" | ||
| echo "> **ℹ️ Baseline comparison skipped** because the benchmark step failed." | ||
| fi | ||
| } >> "$GITHUB_STEP_SUMMARY" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -197,6 +197,29 @@ benchmark_data: benchmark/simple/data.tar.gz ## Prepare data for benchmark | |
| .PHONY: benchmark | ||
| benchmark: benchmark_simple ## Run benchmarks | ||
|
|
||
|
dheerajodha marked this conversation as resolved.
|
||
| .PHONY: generate-baseline | ||
|
dheerajodha marked this conversation as resolved.
dheerajodha marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] naming-convention The target generate-baseline uses hyphens while existing benchmark targets use underscores (benchmark_data, benchmark_stress). Other Makefile targets use hyphens (lint-fix, tools-ci), so this is a minor inconsistency rather than a clear violation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] code-organization The generate-baseline target contains an inline Python script spanning ~15 lines of continuation-escaped Make recipe, more complex than other inline logic in the Makefile. This is largely subsumed by the logic-error finding — extracting to a script resolves both issues. Suggested fix: Extract the baseline-generation logic into a dedicated script (e.g., benchmark/stress/generate_baseline.py). |
||
| generate-baseline: benchmark/stress/data.tar.gz ## Generate stress benchmark baseline | ||
|
dheerajodha marked this conversation as resolved.
|
||
| @cd benchmark/stress && \ | ||
| EC_STRESS_COMPONENTS=$${EC_STRESS_COMPONENTS:-10} EC_STRESS_WORKERS=$${EC_STRESS_WORKERS:-10} \ | ||
| go run . 2>benchmark-stderr.txt | tee benchmark-output.txt && \ | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| python3 -c "\ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [high] logic-error The generate-baseline target passes a multi-line Python script to python3 -c via Make recipe line continuations. GNU Make replaces backslash-newline with a single space, collapsing the entire script onto one logical line. Python's def statement is a compound statement that cannot follow a semicolon — line = line[0]; def val(p): is a SyntaxError. This means make generate-baseline will always fail. Suggested fix: Extract the Python to a separate script file (e.g., benchmark/stress/generate_baseline.py), or rewrite to avoid def by using inline lambda/expressions. |
||
| import re, json, sys; \ | ||
|
dheerajodha marked this conversation as resolved.
|
||
| line = [l for l in open('benchmark-output.txt') if l.startswith('BenchmarkStress')]; \ | ||
| line or sys.exit('No BenchmarkStress results found'); \ | ||
| line = line[0]; \ | ||
| def val(p): \ | ||
| m = re.search(p, line); \ | ||
| return m.group(1) if m else ''; \ | ||
| ns = val(r'([\d.]+)\s+ns/op'); rss = val(r'([\d.]+)\s+peak-RSS-bytes'); \ | ||
| (ns and rss) or sys.exit('Failed to parse benchmark metrics'); \ | ||
| json.dump({'peak_rss_bytes': int(float(rss)), 'ns_per_op': int(float(ns)), \ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case The generate-baseline target embeds $(shell ...) expansions inside a Python string literal. Any unexpected character in the output (e.g., a single quote from a future Go version string) could break Python syntax. Consider passing these values via environment variables. |
||
| 'components': int('$${EC_STRESS_COMPONENTS:-10}'), 'workers': int('$${EC_STRESS_WORKERS:-10}'), \ | ||
| 'commit': '$(shell git rev-parse --short HEAD)', 'date': '$(shell date -u +%Y-%m-%d)', \ | ||
| 'go_version': '$(shell go env GOVERSION | sed "s/^go//")' \ | ||
| }, open('baseline.json','w'), indent=2); print()" && \ | ||
| rm -f benchmark-output.txt benchmark-stderr.txt && \ | ||
| echo "Baseline written to benchmark/stress/baseline.json" | ||
|
|
||
| .PHONY: tools-ci | ||
| tools-ci: ## Ensure all tools build cleanly | ||
| @echo "• tkn:" && \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "peak_rss_bytes": 2250485760, | ||
| "ns_per_op": 2567888013, | ||
| "components": 10, | ||
| "workers": 10, | ||
| "commit": "fc37eb13", | ||
| "date": "2026-08-11", | ||
| "go_version": "1.26.3" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| #!/bin/bash | ||
| # Copyright The Conforma Contributors | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # Compares current benchmark results against a stored baseline and exits | ||
| # non-zero if any metric regresses beyond the configured threshold. | ||
| set -o errexit | ||
| set -o nounset | ||
| set -o pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| BASELINE="${SCRIPT_DIR}/baseline.json" | ||
| THRESHOLDS="${SCRIPT_DIR}/thresholds.json" | ||
| BENCHMARK_OUTPUT="${1:-${SCRIPT_DIR}/benchmark-output.txt}" | ||
|
|
||
| if [[ ! -f "$BASELINE" ]]; then | ||
| echo "No baseline found, skipping comparison." | ||
| exit 0 | ||
| fi | ||
|
dheerajodha marked this conversation as resolved.
|
||
|
|
||
| if [[ ! -f "$THRESHOLDS" ]]; then | ||
| echo "No thresholds file found, skipping comparison." | ||
| exit 0 | ||
| fi | ||
|
dheerajodha marked this conversation as resolved.
|
||
|
|
||
| if [[ ! -f "$BENCHMARK_OUTPUT" ]]; then | ||
| echo "No benchmark output found at ${BENCHMARK_OUTPUT}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| line=$(grep '^BenchmarkStress' "$BENCHMARK_OUTPUT" || true) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] error-handling-idiom Error messages on lines 44 and 49 are written to stdout instead of stderr, inconsistent with the sibling push_data.sh and the embedded Python in the same script which write errors to stderr. Suggested fix: Append >&2 to the echo statements on lines 44 and 49. |
||
| if [[ -z "$line" ]]; then | ||
|
dheerajodha marked this conversation as resolved.
|
||
| echo "No BenchmarkStress results found in output." | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case If the benchmark output contains multiple BenchmarkStress lines, grep captures all of them. Python's re.search finds the first match, silently discarding subsequent lines. Consider grep -m1 to be explicit about taking only the first match. Suggested fix: Use grep -m1 '^BenchmarkStress' to explicitly take only the first match, and log a warning if multiple lines exist. |
||
| exit 1 | ||
| fi | ||
|
dheerajodha marked this conversation as resolved.
|
||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] error-handling-gap If the python3 process substitution produces partial output, read could succeed with some variables empty, causing awk to silently treat them as 0. Adding a validation guard after read (checking all six variables are non-empty) would improve robustness. Suggested fix: After the read command, add: [[ -n "$current_ns" && -n "$current_rss" && -n "$baseline_ns" && -n "$baseline_rss" && -n "$threshold_rss" && -n "$threshold_time" ]] || { echo 'Failed to parse metrics'; exit 1; } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case If the benchmark output contains multiple lines starting with BenchmarkStress (e.g., sub-benchmarks), grep returns all of them. Python re.search will match against the concatenated multi-line string, which may produce unexpected results. Suggested fix: Use grep -m1 or pipe through head -1 to ensure only one line is processed. |
||
| read -r current_ns current_rss baseline_ns baseline_rss threshold_rss threshold_time < <( | ||
| BENCH_LINE="${line}" BASELINE_PATH="${BASELINE}" THRESHOLDS_PATH="${THRESHOLDS}" python3 -c " | ||
|
dheerajodha marked this conversation as resolved.
|
||
| import json, os, re, sys | ||
| line = os.environ['BENCH_LINE'] | ||
| def extract(pattern): | ||
| m = re.search(pattern, line) | ||
| return m.group(1) if m else '' | ||
| ns = extract(r'([\d.]+)\s+ns/op') | ||
| rss = extract(r'([\d.]+)\s+peak-RSS-bytes') | ||
|
dheerajodha marked this conversation as resolved.
|
||
| if not ns or not rss: | ||
| print('Failed to parse benchmark metrics from output.', file=sys.stderr) | ||
| sys.exit(1) | ||
| 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']) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| " | ||
|
dheerajodha marked this conversation as resolved.
|
||
| ) | ||
|
|
||
| if awk -v rss="$baseline_rss" -v ns="$baseline_ns" 'BEGIN {exit !(rss==0 || ns==0)}'; then | ||
|
dheerajodha marked this conversation as resolved.
|
||
| echo "Baseline contains zero values, cannot compute regression." | ||
|
dheerajodha marked this conversation as resolved.
|
||
| exit 1 | ||
| fi | ||
|
|
||
| rss_change=$(awk -v cur="$current_rss" -v base="$baseline_rss" 'BEGIN {printf "%.1f", ((cur - base) / base) * 100}') | ||
| time_change=$(awk -v cur="$current_ns" -v base="$baseline_ns" 'BEGIN {printf "%.1f", ((cur - base) / base) * 100}') | ||
|
|
||
| baseline_rss_mb=$(awk -v val="$baseline_rss" 'BEGIN {printf "%.0f", val / 1048576}') | ||
| current_rss_mb=$(awk -v val="$current_rss" 'BEGIN {printf "%.0f", val / 1048576}') | ||
| baseline_secs=$(awk -v val="$baseline_ns" 'BEGIN {printf "%.1f", val / 1000000000}') | ||
| current_secs=$(awk -v val="$current_ns" 'BEGIN {printf "%.1f", val / 1000000000}') | ||
|
|
||
| echo "" | ||
| echo "=== Benchmark Comparison ===" | ||
| echo "" | ||
| printf "%-20s %10s %10s %10s %10s\n" "Metric" "Baseline" "Current" "Change" "Threshold" | ||
| printf "%-20s %10s %10s %9s%% %9s%%\n" "Peak RSS" "${baseline_rss_mb} MB" "${current_rss_mb} MB" "$rss_change" "$threshold_rss" | ||
| printf "%-20s %10s %10s %9s%% %9s%%\n" "Execution time" "${baseline_secs}s" "${current_secs}s" "$time_change" "$threshold_time" | ||
| echo "" | ||
|
|
||
| failed=0 | ||
|
|
||
| rss_exceeded=$(awk -v change="$rss_change" -v thresh="$threshold_rss" 'BEGIN {print (change > thresh) ? 1 : 0}') | ||
| time_exceeded=$(awk -v change="$time_change" -v thresh="$threshold_time" 'BEGIN {print (change > thresh) ? 1 : 0}') | ||
|
|
||
| if [[ "$rss_exceeded" == "1" ]]; then | ||
| echo "FAIL: Peak RSS regressed by ${rss_change}% (threshold: ${threshold_rss}%)" | ||
| failed=1 | ||
| fi | ||
|
|
||
| if [[ "$time_exceeded" == "1" ]]; then | ||
| echo "FAIL: Execution time regressed by ${time_change}% (threshold: ${threshold_time}%)" | ||
| failed=1 | ||
| fi | ||
|
|
||
| if [[ "$failed" == "0" ]]; then | ||
| echo "PASS: No regressions detected." | ||
| fi | ||
|
|
||
| exit "$failed" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| { | ||
| "peak_rss_percent": 15, | ||
| "ns_per_op_percent": 20 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] logic-error
Removing continue-on-error: true from the job level converts the benchmark from an advisory check into a blocking gate. With a single iteration and no statistical averaging or retry mechanism, noisy CI environments may produce false positive regressions that block PRs. The 15% RSS and 20% ns/op thresholds are reasonably generous but may not absorb CI-inherent variance in all cases.