-
Notifications
You must be signed in to change notification settings - Fork 60
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
22f5a73
2683ecf
64e354e
7248583
6e5c181
78a3d4e
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. |
||
| # 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.
|
||
| 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. |
||
| 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. |
||
| 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 "\ | ||
| 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) | ||
| 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; } |
||
| 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.