Add benchmarks workflow and CI for collector throughput#224
Open
leostar0412 wants to merge 2 commits into
Open
Add benchmarks workflow and CI for collector throughput#224leostar0412 wants to merge 2 commits into
leostar0412 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a complete performance benchmarking infrastructure alongside documentation alignment. It adds pytest-based benchmark tests for measuring commit processing and service-layer write throughput, baseline tracking with automated comparison via CI, and updates all documentation references from legacy paths to a unified ChangesBenchmark Infrastructure and Documentation Updates
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/Service_API.md (1)
43-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the remaining stale
Contributing.mdlink in this file.Line 43 still points to
Contributing.md, which is inconsistent with the canonical rootCONTRIBUTING.mdpath and can break on case-sensitive environments.Suggested fix
-Tables in each file are **generated** from source; see [Contributing.md](Contributing.md#regenerating-service-api-docs). +Tables in each file are **generated** from source; see [CONTRIBUTING.md](../CONTRIBUTING.md#regenerating-service-api-docs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/Service_API.md` at line 43, Update the stale link that points to "Contributing.md" to use the canonical uppercase path "CONTRIBUTING.md" in the docs/Service_API.md content (the line that currently reads "see [Contributing.md](Contributing.md#regenerating-service-api-docs)"); replace both the filename and its fragment target if needed so the link becomes "see [CONTRIBUTING.md](CONTRIBUTING.md#regenerating-service-api-docs)" to avoid case-sensitivity issues.
🧹 Nitpick comments (1)
.github/workflows/benchmarks.yml (1)
31-33: ⚡ Quick winAdd
persist-credentials: falseto the checkout step for GitHub Actions.The checkout action retains git credentials by default. Set
persist-credentials: falseto minimize token exposure in this workflow.Suggested patch
- name: Checkout uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/benchmarks.yml around lines 31 - 33, The Checkout step using actions/checkout@v4 currently retains credentials by default; update the "Checkout" step to include persist-credentials: false (i.e., add the key persist-credentials with value false under the uses: actions/checkout@v4 entry) so the checkout action does not persist git credentials into the workflow environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/benchmarks.yml:
- Line 32: The workflow uses floating action tags (e.g., actions/checkout@v4,
actions/setup-node@v7, actions/cache@v4 and the other action at around line 74)
which must be replaced with immutable commit SHAs; locate each uses: entry for
actions/checkout, actions/setup-node, actions/cache and the other referenced
action and replace the `@v`* tag with the corresponding full commit SHA from that
action's GitHub releases/tags page so the workflow pins to a specific commit SHA
instead of a floating version.
In `@bench.json`:
- Around line 2-38: The bench.json file contains sensitive local machine
metadata under machine_info and commit_info and must not be committed; remove
bench.json from the repo, add its filename or pattern to .gitignore, and move
generation of this artifact to CI/artifacts rather than source control; if this
file was already pushed, purge it from history using a history-rewriting tool
(git filter-repo or BFG) or remove it via git rm --cached and force-push, and
update CI (the job that produces bench.json) to upload it as a build artifact
instead of committing.
In `@benchmarks/baselines.json`:
- Around line 5-10: The baseline median_seconds for the benchmarks are
unrealistically high and make regression checks useless; update the
"median_seconds" values for the affected entries (the first entry with
"median_seconds": 45.0 and the
"benchmarks/test_service_bulk_insert.py::test_service_bulk_commits_and_file_changes"
entry with "median_seconds": 35.0) to the actual recorded medians from this PR’s
bench.json (approximately 0.1369 and 0.1406 respectively) while leaving other
fields (like "n": 50) unchanged so CI regression thresholds are meaningful.
In `@benchmarks/compare_to_baseline.py`:
- Line 77: The error string uses a non-ASCII multiplication character "×" which
triggers Ruff RUF001; update the formatted message where f"(baseline
{float(ref):.6f}s × {args.regression_ratio})" is constructed (referencing
variables ref and args.regression_ratio) to use an ASCII character such as "x"
or "*" instead (e.g. "x") so the text becomes f"(baseline {float(ref):.6f}s x
{args.regression_ratio})".
In `@benchmarks/test_service_bulk_insert.py`:
- Line 28: The current commit hash generation in the 'hashes' list uses
f"svcbulk{i:056d}"[:40], which truncates the variable part and makes all entries
identical; update the expression that builds 'hashes' (the list comprehension
assigned to the variable hashes) so the varying suffix is preserved—for example
use the trailing slice f"svcbulk{i:056d}"[-40:] or otherwise include i in the
kept portion (or replace with a deterministic hash like
hashlib.sha1(f"svcbulk{i}".encode()).hexdigest()[:40]) so each iteration
produces a distinct commit_hash.
In `@CONTRIBUTING.md`:
- Around line 89-90: The documented local benchmark command (the pytest
invocation shown: "uv run pytest benchmarks/ -m benchmark --benchmark-only
--benchmark-json=bench.json -v") is missing the CI-only flag; update that
command in CONTRIBUTING.md to include --benchmark-disable-gc so local runs
disable the GC exactly like the CI workflow, ensuring comparable results.
In `@docs/service_api/cppa_pinecone_sync.md`:
- Line 5: In docs/service_api/cppa_pinecone_sync.md update the remaining legacy
CONTRIBUTING link by replacing the incorrect "../Contributing.md" occurrence
(the link text on Line 25) with the correct "../../CONTRIBUTING.md" so both
references use the canonical uppercase CONTRIBUTING.md path; search for the
string "../Contributing.md" and change it to "../../CONTRIBUTING.md" to ensure
consistency with the earlier fix.
---
Outside diff comments:
In `@docs/Service_API.md`:
- Line 43: Update the stale link that points to "Contributing.md" to use the
canonical uppercase path "CONTRIBUTING.md" in the docs/Service_API.md content
(the line that currently reads "see
[Contributing.md](Contributing.md#regenerating-service-api-docs)"); replace both
the filename and its fragment target if needed so the link becomes "see
[CONTRIBUTING.md](CONTRIBUTING.md#regenerating-service-api-docs)" to avoid
case-sensitivity issues.
---
Nitpick comments:
In @.github/workflows/benchmarks.yml:
- Around line 31-33: The Checkout step using actions/checkout@v4 currently
retains credentials by default; update the "Checkout" step to include
persist-credentials: false (i.e., add the key persist-credentials with value
false under the uses: actions/checkout@v4 entry) so the checkout action does not
persist git credentials into the workflow environment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17f058c6-9386-4b86-aedf-5a5e3b4e4528
⛔ Files ignored due to path filters (1)
requirements-dev.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.github/workflows/benchmarks.ymlCONTRIBUTING.mdREADME.mdbench.jsonbenchmarks/baselines.jsonbenchmarks/compare_to_baseline.pybenchmarks/conftest.pybenchmarks/test_github_commits_throughput.pybenchmarks/test_service_bulk_insert.pyboost_library_tracker/services.pyboost_mailing_list_tracker/services.pyboost_usage_tracker/services.pyconftest.pycppa_pinecone_sync/services.pycppa_slack_tracker/services.pycppa_user_tracker/services.pycppa_youtube_script_tracker/services.pydocs/How_to_add_a_collector.mddocs/Onboarding.mddocs/README.mddocs/Service_API.mddocs/boost_library_docs_tracker.mddocs/cross-app-dependencies.mddocs/service_api/README.mddocs/service_api/boost_usage_tracker.mddocs/service_api/clang_github_tracker.mddocs/service_api/cppa_pinecone_sync.mddocs/service_api/cppa_user_tracker.mddocs/service_api/discord_activity_tracker.mddocs/service_api/github_activity_tracker.mdgithub_activity_tracker/services.pypyproject.tomlpytest.inirequirements-dev.in
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no floating action refs remain in this workflow.
rg -nP '^\s*uses:\s*[^@\s]+@(?!(?:[0-9a-f]{40})\b)' .github/workflows/benchmarks.yml
# Expected after fix: no outputRepository: cppalliance/boost-data-collector
Length of output: 230
Pin all workflow actions to full commit SHAs.
Lines 32, 35, 40, and 74 use floating version tags (@v4, @v7) instead of immutable commit SHAs, which violates supply-chain pinning policy and allows uncontrolled updates to action implementations.
Suggested fix
- uses: actions/checkout@v4
+ uses: actions/checkout@<40-char-sha>
- uses: astral-sh/setup-uv@v7
+ uses: astral-sh/setup-uv@<40-char-sha>
- uses: actions/cache@v4
+ uses: actions/cache@<40-char-sha>
- uses: actions/upload-artifact@v4
+ uses: actions/upload-artifact@<40-char-sha>Replace each @v* tag with the pinned commit SHA from the action's releases page.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 31-32: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 32-32: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/benchmarks.yml at line 32, The workflow uses floating
action tags (e.g., actions/checkout@v4, actions/setup-node@v7, actions/cache@v4
and the other action at around line 74) which must be replaced with immutable
commit SHAs; locate each uses: entry for actions/checkout, actions/setup-node,
actions/cache and the other referenced action and replace the `@v`* tag with the
corresponding full commit SHA from that action's GitHub releases/tags page so
the workflow pins to a specific commit SHA instead of a floating version.
Comment on lines
+2
to
+38
| "machine_info": { | ||
| "node": "Leos-Mac-mini.local", | ||
| "processor": "arm", | ||
| "machine": "arm64", | ||
| "python_compiler": "Clang 21.1.4 ", | ||
| "python_implementation": "CPython", | ||
| "python_implementation_version": "3.13.12", | ||
| "python_version": "3.13.12", | ||
| "python_build": [ | ||
| "main", | ||
| "Mar 10 2026 18:26:32" | ||
| ], | ||
| "release": "25.4.0", | ||
| "system": "Darwin", | ||
| "cpu": { | ||
| "python_version": "3.13.12.final.0 (64 bit)", | ||
| "cpuinfo_version": [ | ||
| 9, | ||
| 0, | ||
| 0 | ||
| ], | ||
| "cpuinfo_version_string": "9.0.0", | ||
| "arch": "ARM_8", | ||
| "bits": 64, | ||
| "count": 10, | ||
| "arch_string_raw": "arm64", | ||
| "brand_raw": "Apple M4" | ||
| } | ||
| }, | ||
| "commit_info": { | ||
| "id": "7bf1b7ea6657990eef44fdb362b762abb16e41ba", | ||
| "time": "2026-05-18T20:05:08-04:00", | ||
| "author_time": "2026-05-18T20:05:08-04:00", | ||
| "dirty": true, | ||
| "project": "boost-data-collector", | ||
| "branch": "develop" | ||
| }, |
There was a problem hiding this comment.
Do not commit raw benchmark result artifacts with local machine metadata.
This file includes host-identifying data (e.g., Line 3) and ephemeral local run state. It should be produced in CI/local runs and stored as an artifact, not tracked in the repo.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench.json` around lines 2 - 38, The bench.json file contains sensitive local
machine metadata under machine_info and commit_info and must not be committed;
remove bench.json from the repo, add its filename or pattern to .gitignore, and
move generation of this artifact to CI/artifacts rather than source control; if
this file was already pushed, purge it from history using a history-rewriting
tool (git filter-repo or BFG) or remove it via git rm --cached and force-push,
and update CI (the job that produces bench.json) to upload it as a build
artifact instead of committing.
Comment on lines
+5
to
+10
| "median_seconds": 45.0, | ||
| "n": 50 | ||
| }, | ||
| "benchmarks/test_service_bulk_insert.py::test_service_bulk_commits_and_file_changes": { | ||
| "median_seconds": 35.0, | ||
| "n": 50 |
There was a problem hiding this comment.
Baseline medians are so high that regression checks become ineffective.
Line 5 (45.0) and Line 9 (35.0) are far above the recorded medians in this PR’s bench.json (~0.1369s and 0.1406s). With a 1.25 ratio, this would allow massive slowdowns without failing CI.
🔧 Proposed fix
- "median_seconds": 45.0,
+ "median_seconds": 0.137,
@@
- "median_seconds": 35.0,
+ "median_seconds": 0.141,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "median_seconds": 45.0, | |
| "n": 50 | |
| }, | |
| "benchmarks/test_service_bulk_insert.py::test_service_bulk_commits_and_file_changes": { | |
| "median_seconds": 35.0, | |
| "n": 50 | |
| "median_seconds": 0.137, | |
| "n": 50 | |
| }, | |
| "benchmarks/test_service_bulk_insert.py::test_service_bulk_commits_and_file_changes": { | |
| "median_seconds": 0.141, | |
| "n": 50 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/baselines.json` around lines 5 - 10, The baseline median_seconds
for the benchmarks are unrealistically high and make regression checks useless;
update the "median_seconds" values for the affected entries (the first entry
with "median_seconds": 45.0 and the
"benchmarks/test_service_bulk_insert.py::test_service_bulk_commits_and_file_changes"
entry with "median_seconds": 35.0) to the actual recorded medians from this PR’s
bench.json (approximately 0.1369 and 0.1406 respectively) while leaving other
fields (like "n": 50) unchanged so CI regression thresholds are meaningful.
| if median > limit: | ||
| failures.append( | ||
| f"{fullname}: median {median:.6f}s exceeds limit {limit:.6f}s " | ||
| f"(baseline {float(ref):.6f}s × {args.regression_ratio})" |
There was a problem hiding this comment.
Replace ambiguous multiplication symbol in error text.
Line 77 uses ×, which Ruff flags as RUF001; this can break CI linting.
🔧 Proposed fix
- f"(baseline {float(ref):.6f}s × {args.regression_ratio})"
+ f"(baseline {float(ref):.6f}s x {args.regression_ratio})"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| f"(baseline {float(ref):.6f}s × {args.regression_ratio})" | |
| f"(baseline {float(ref):.6f}s x {args.regression_ratio})" |
🧰 Tools
🪛 Ruff (0.15.13)
[warning] 77-77: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/compare_to_baseline.py` at line 77, The error string uses a
non-ASCII multiplication character "×" which triggers Ruff RUF001; update the
formatted message where f"(baseline {float(ref):.6f}s ×
{args.regression_ratio})" is constructed (referencing variables ref and
args.regression_ratio) to use an ASCII character such as "x" or "*" instead
(e.g. "x") so the text becomes f"(baseline {float(ref):.6f}s x
{args.regression_ratio})".
| repo = github_repository | ||
| account = github_account | ||
| commit_at = datetime(2024, 6, 1, tzinfo=timezone.utc) | ||
| hashes = [f"svcbulk{i:056d}"[:40] for i in range(n)] |
There was a problem hiding this comment.
Commit hash generation is constant across iterations.
f"svcbulk{i:056d}"[:40] truncates away the changing suffix, so all entries collapse to the same commit_hash for typical n values. This turns the benchmark into repeated updates of one commit instead of N distinct commits.
🔧 Proposed fix
- hashes = [f"svcbulk{i:056d}"[:40] for i in range(n)]
+ hashes = [f"{i:040x}" for i in range(n)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/test_service_bulk_insert.py` at line 28, The current commit hash
generation in the 'hashes' list uses f"svcbulk{i:056d}"[:40], which truncates
the variable part and makes all entries identical; update the expression that
builds 'hashes' (the list comprehension assigned to the variable hashes) so the
varying suffix is preserved—for example use the trailing slice
f"svcbulk{i:056d}"[-40:] or otherwise include i in the kept portion (or replace
with a deterministic hash like
hashlib.sha1(f"svcbulk{i}".encode()).hexdigest()[:40]) so each iteration
produces a distinct commit_hash.
Comment on lines
+89
to
+90
| uv run pytest benchmarks/ -m benchmark --benchmark-only \ | ||
| --benchmark-json=bench.json -v |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check consistency of benchmark GC flag between workflow and docs.
rg -n -- '--benchmark-disable-gc' .github/workflows/benchmarks.yml CONTRIBUTING.md
# Expected after fix: matches in both filesRepository: cppalliance/boost-data-collector
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Get context around the benchmark command in CONTRIBUTING.md (lines 85-95)
echo "=== CONTRIBUTING.md (lines 85-95) ==="
sed -n '85,95p' CONTRIBUTING.md
# Get context around the flag in the CI workflow (lines 60-70)
echo -e "\n=== .github/workflows/benchmarks.yml (lines 60-70) ==="
sed -n '60,70p' .github/workflows/benchmarks.ymlRepository: cppalliance/boost-data-collector
Length of output: 1371
Align local benchmark command with CI benchmark settings.
Line 89–90 omits --benchmark-disable-gc, while the CI workflow includes it. This flag disables garbage collection during benchmarks, ensuring local and CI results are directly comparable.
Suggested doc update
uv run pytest benchmarks/ -m benchmark --benchmark-only \
- --benchmark-json=bench.json -v
+ --benchmark-json=bench.json -v \
+ --benchmark-disable-gc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| uv run pytest benchmarks/ -m benchmark --benchmark-only \ | |
| --benchmark-json=bench.json -v | |
| uv run pytest benchmarks/ -m benchmark --benchmark-only \ | |
| --benchmark-json=bench.json -v \ | |
| --benchmark-disable-gc |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONTRIBUTING.md` around lines 89 - 90, The documented local benchmark command
(the pytest invocation shown: "uv run pytest benchmarks/ -m benchmark
--benchmark-only --benchmark-json=bench.json -v") is missing the CI-only flag;
update that command in CONTRIBUTING.md to include --benchmark-disable-gc so
local runs disable the GC exactly like the CI workflow, ensuring comparable
results.
| Module: `cppa_pinecone_sync.services` | ||
|
|
||
| All creates/updates/deletes for `PineconeFailList` and `PineconeSyncStatus` must go through this module. See [Contributing.md](../Contributing.md). | ||
| All creates/updates/deletes for `PineconeFailList` and `PineconeSyncStatus` must go through this module. See [CONTRIBUTING.md](../../CONTRIBUTING.md). |
There was a problem hiding this comment.
Update the remaining legacy Contributing link in this file.
Line 5 is corrected, but Line 25 still points to ../Contributing.md. Please switch it to ../../CONTRIBUTING.md so this page doesn’t keep one stale path.
Suggested patch
- - [Contributing](../Contributing.md)
+ - [CONTRIBUTING.md](../../CONTRIBUTING.md)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/service_api/cppa_pinecone_sync.md` at line 5, In
docs/service_api/cppa_pinecone_sync.md update the remaining legacy CONTRIBUTING
link by replacing the incorrect "../Contributing.md" occurrence (the link text
on Line 25) with the correct "../../CONTRIBUTING.md" so both references use the
canonical uppercase CONTRIBUTING.md path; search for the string
"../Contributing.md" and change it to "../../CONTRIBUTING.md" to ensure
consistency with the earlier fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
pytest/ project benchmark command as documented).Summary by CodeRabbit
New Features
Documentation