Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 127 additions & 0 deletions .github/workflows/benchmark.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
# 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

---
name: Stress Benchmark

"on":
pull_request:
branches:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] pattern-inconsistency

The workflow triggers only on pull_request without workflow_dispatch, unlike other CI workflows (lint.yaml, checks-codecov.yaml) that include it for manual re-runs. While omitting push is reasonable for a non-blocking benchmark, adding workflow_dispatch would allow ad-hoc performance investigations.

Suggested fix: Add workflow_dispatch to the on triggers.

- main
workflow_dispatch:

permissions:
contents: read

jobs:

stress:
name: Stress Benchmark
runs-on: ubuntu-latest

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] naming-convention

The job ID Stress uses PascalCase, while the majority of workflows in the repo (11 of 15 job IDs) use lowercase. PascalCase is only used in checks-codecov.yaml.

Suggested fix: Consider renaming the job ID to stress for consistency with the dominant convention.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] naming-convention

The job ID stress has no explicit name: field. All other workflows in the repo provide descriptive job names (e.g., Test, Lint, Analyze, Scorecard analysis). Without a name:, the GitHub Actions UI displays the raw job ID stress.

Suggested fix: Add name: Stress Benchmark to the job definition.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] step-pattern-consistency

All other Go-based workflows in the repo include a Restore Cache step using actions/cache/restore after checkout. The benchmark workflow omits this step. Adding the cache would reduce CI wall time without affecting benchmark accuracy.

timeout-minutes: 15
continue-on-error: true
Comment thread
coderabbitai[bot] marked this conversation as resolved.
env:
# Tuned for 4 vCPU / 16 GB CI runners to complete within 6-8 minutes.
# Code defaults are 10 components / 35 workers.
EC_STRESS_COMPONENTS: "10"
EC_STRESS_WORKERS: "10"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] scope-alignment

The workflow sets EC_STRESS_COMPONENTS=40 and EC_STRESS_WORKERS=10, which differ from the code defaults in stress.go (defaultComponents=10, defaultWorkers=35). The rationale for these CI-specific values is not documented.

steps:
- name: Harden Runner
uses: step-security/harden-runner@58077d3c7e43986b6b15fba718e8ea69e387dfcc # v2.15.1
with:
egress-policy: audit
disable-telemetry: true

- name: Checkout repository
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2

- name: Restore Cache
uses: actions/cache/restore@668228422ae6a00e4ad889ee87cd7109ec5666a7 # v5.0.4
with:
key: main
path: '**'

- name: Setup Go environment
uses: actions/setup-go@4b73464bb391d4059bd26b0524d20df3927bd417 # v6.3.0
with:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] permission-expansion

The cache restore step uses path: ** which restores cached content over the entire workspace. Combined with executing scripts from the workspace (prepare_data.sh, the built stress binary), a poisoned cache from the main branch could overwrite these files before execution. This is a pre-existing repo-wide pattern and not specific to this PR.

Suggested fix: Narrow the path to only the specific directories needed by the benchmark rather than using the ** wildcard. This would be a repo-wide improvement, not specific to this workflow.

go-version-file: go.mod

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] missing established step pattern

Every existing workflow in this repo that sets up Go (checks-codecov, lint, codeql, release, update-go-containerregistry) includes a cache restore step using actions/cache/restore between checkout and setup-go. The benchmark workflow omits this step, which will result in slower CI runs as the Go module and build caches are not restored.

Suggested fix: Add a 'Restore Cache' step using actions/cache/restore between 'Checkout repository' and 'Setup Go environment', matching the pattern in other workflows.

cache: false

- name: Setup ORAS
uses: oras-project/setup-oras@1d808f7d7f6995cc68b7bf507bfe5c5446e1dc9d # v2.0.1

- name: Prepare benchmark data
run: |
cd benchmark/stress
./prepare_data.sh

- name: Build stress benchmark
run: go build -o benchmark/stress/stress ./benchmark/stress
Comment thread
qodo-for-conforma[bot] marked this conversation as resolved.

- name: Run stress benchmark
id: bench
run: |
set -o pipefail
cd benchmark/stress
./stress 2>benchmark-stderr.txt | tee benchmark-output.txt

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] error handling

The benchmark step runs go run . 2>/dev/null | tee benchmark-output.txt, which discards all stderr output including Go compilation errors, runtime panics, and benchmark driver diagnostics. If the benchmark fails to compile or panics, the job summary silently reports 'No benchmark results found' with no indication of the root cause.

Suggested fix: Consider separating compilation from execution so build errors remain visible: go build -o stress . && ./stress 2>/dev/null | tee benchmark-output.txt.

- name: Write job summary

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] edge-case

go run . 2>/dev/null suppresses all stderr output, including Go compilation errors, runtime panics, and diagnostic messages from the benchmark driver. If the benchmark fails silently, the summary step reports 'No benchmark results found' with no indication of what went wrong.

Suggested fix: Redirect stderr to a log file instead: go run . 2>benchmark-stderr.txt | tee benchmark-output.txt

if: always()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] error-handling

The Run stress benchmark step redirects stderr to /dev/null (./stress 2>/dev/null), silently discarding all error output including Go panics (the stress binary uses panic() in 6+ locations). Combined with continue-on-error: true, a failing benchmark produces no diagnostic output.

Suggested fix: Remove the 2>/dev/null redirect so errors appear in the CI step log, or redirect stderr to a file and include it in the job summary on failure.

run: |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] error-handling

The pipeline ./stress 2>benchmark-stderr.txt | tee benchmark-output.txt does not use set -o pipefail. If the stress binary fails, tee still exits 0, so the step reports success. While continue-on-error: true and the summary step's guard clauses handle this gracefully, the step's green check could be misleading when investigating failures.

Suggested fix: Add set -o pipefail at the beginning of the run block, or check ${PIPESTATUS[0]} after the pipeline.

if [[ "${{ steps.bench.outcome }}" == "failure" && -s benchmark/stress/benchmark-stderr.txt ]]; then
{
echo "### Stderr"
echo '```'
tail -50 benchmark/stress/benchmark-stderr.txt
echo '```'
} >> "$GITHUB_STEP_SUMMARY"
fi

Comment thread
dheerajodha marked this conversation as resolved.
if [[ ! -f benchmark/stress/benchmark-output.txt ]]; then
echo "## Stress Benchmark" >> "$GITHUB_STEP_SUMMARY"
echo "Benchmark did not produce output." >> "$GITHUB_STEP_SUMMARY"
exit 0
fi

line=$(grep '^BenchmarkStress' benchmark/stress/benchmark-output.txt || true)
Comment thread
qodo-for-conforma[bot] marked this conversation as resolved.
Comment thread
dheerajodha marked this conversation as resolved.
if [[ -z "$line" ]]; then
echo "## Stress Benchmark" >> "$GITHUB_STEP_SUMMARY"
echo "No benchmark results found in output." >> "$GITHUB_STEP_SUMMARY"
exit 0
fi

ns_op=$(echo "$line" | grep -oP '[\d.]+ ns/op' | awk '{print $1}')
Comment thread
qodo-for-conforma[bot] marked this conversation as resolved.
peak_rss=$(echo "$line" | grep -oP '[\d.]+ peak-RSS-bytes' | awk '{print $1}')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] injection

Shell variables (ns_op, peak_rss, alloc, heap) are interpolated into double-quoted awk command strings. The upstream grep -oP constrains values to digits and dots, making awk injection practically infeasible, but the pattern is fragile. Using awk -v to pass variables is a safer idiom at zero cost.

Suggested fix: Use awk -v to pass variables: secs=$(awk -v val="${ns_op:-0}" 'BEGIN {printf "%.1f", val / 1000000000}')

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}')

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}')

{
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 |"
} >> "$GITHUB_STEP_SUMMARY"
Loading