-
Notifications
You must be signed in to change notification settings - Fork 61
Add stress benchmark CI workflow #3426
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
3cc775d
26bde5c
7b261b5
4861700
d350631
bb9a516
5306b46
0bb8431
6b8e5bf
45c5b81
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 |
|---|---|---|
| @@ -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: | ||
| - main | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
|
|
||
| stress: | ||
| name: Stress Benchmark | ||
| runs-on: ubuntu-latest | ||
|
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 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. 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] 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. 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] 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 | ||
|
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" | ||
|
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-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: | ||
|
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] 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 | ||
|
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] 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 | ||
|
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 | ||
|
|
||
|
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 The benchmark step runs Suggested fix: Consider separating compilation from execution so build errors remain visible: |
||
| - name: Write job summary | ||
|
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 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() | ||
|
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 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: | | ||
|
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 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 | ||
|
|
||
|
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) | ||
|
qodo-for-conforma[bot] marked this conversation as resolved.
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}') | ||
|
qodo-for-conforma[bot] marked this conversation as resolved.
|
||
| peak_rss=$(echo "$line" | grep -oP '[\d.]+ peak-RSS-bytes' | awk '{print $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] 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" | ||
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.
[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.