ci: stop running the eventbus latency SLO bench in CI#192
Merged
Conversation
Remove the `eventbus-latency-slo` job (and the now-dead `pull_request` trigger that existed only for it) from the Benchmarks workflow. Shared GitHub runners are too inconsistent for latency benchmarking to gate on — the job's measurement passed (~20-40µs p99 vs the 1ms threshold) but the bench process reliably hung to the 15-minute job timeout, producing a perpetual non-required red check. The benchmark itself stays in the codebase (daemoneye-eventbus/benches/ipc_performance.rs::latency_p99_slo) and is runnable locally or via `workflow_dispatch`; the other benchmark jobs (benchmarks, load-tests) were already manual-dispatch only and are unaffected. The Benchmarks workflow is now entirely manual. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
dosubot
Bot
added
size:XS
Summary by CodeRabbit
WalkthroughThe Benchmarks GitHub Actions workflow is switched to manual-dispatch-only and the ChangesManual-Dispatch Benchmark Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Linear, Notion Comment |
Contributor
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Require conventional commit format per https://www.conventionalcommits.org/en/v1.0.0/. Skipped for dependabot and dosubot.
🟢 Full CI must passWonderful, this rule succeeded.All CI checks must pass. Activates for non-bot authors, or dependabot when files exist outside .github/workflows/.
🟢 Do not merge outdated PRsWonderful, this rule succeeded.Make sure PRs are within 3 commits of the base branch before merging
|
There was a problem hiding this comment.
Pull request overview
Removes an unstable latency-SLO benchmark job from the Benchmarks GitHub Actions workflow so CI is no longer impacted by a runner-specific hang/timeout, while keeping benchmarks available for deliberate, manual execution.
Changes:
- Removed the
eventbus-latency-slojob from the Benchmarks workflow. - Removed the
pull_requesttrigger so the workflow is nowworkflow_dispatchonly. - Updated the workflow header comment to reflect the manual-only benchmarking policy.
coderabbitai
Bot
added
testing
Contributor
📄 Knowledge review✏️ Suggested updates2 page suggestions need review.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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:
- Around line 3-8: Add a workflow-level concurrency stanza to serialize manual
runs: create a concurrency block for this workflow using a unique group key that
includes the ref (e.g., "benchmarks-${{ github.ref }}") and enable
cancel-in-progress as appropriate; place this alongside the existing top-level
"on:" block so manual workflow_dispatch runs for the same branch/resource do not
overlap.
🪄 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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5da8517f-f9ae-40e3-b2cb-4bf26d72d43f
📒 Files selected for processing (1)
.github/workflows/benchmarks.yml
coderabbitai
Bot
added
documentation
Serialize manual benchmark dispatches per ref so two concurrent runs don't contend on the shared criterion baseline cache / artifact state. Uses cancel-in-progress: false (matching docs.yml) so an in-flight measurement finishes rather than being killed mid-benchmark. Matches the workflow-level concurrency convention already used by ci.yml / docs.yml / security.yml. Addresses CodeRabbit review feedback on #192. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
priority:normal
ci
and removed
documentation
| ### Latency | ||
|
|
||
| - **Local IPC**: \<1ms p99 for local IPC, validated in CI on Linux and macOS by a dedicated criterion benchmark. Windows and FreeBSD are informational only (not gated in CI). | ||
| - **Local IPC**: \<1ms p99 for local IPC. A dedicated criterion benchmark in `benches/ipc_performance.rs` measures latency, but it is not run automatically in CI due to inconsistency on shared GitHub runners. Manual runs on stable hardware typically measure p99 ≈ 20–40µs. |
| - **Connection Overhead**: Minimal with connection pooling | ||
| - **Memory Usage**: Bounded with configurable limits | ||
| - **Latency**: \<1ms p99 for local IPC, asserted in CI on Linux and macOS by a dedicated criterion bench in `benches/ipc_performance.rs`. Windows and FreeBSD results are informational only. | ||
| - **Latency**: \<1ms p99 for local IPC. A dedicated criterion benchmark in `benches/ipc_performance.rs` measures latency but is not run automatically in CI due to inconsistency on shared GitHub runners. Manual runs on stable hardware typically measure p99 ≈ 20–40µs. |
Member
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status
Waiting for
All conditions
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/benchmarks.yml (1)
130-130:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent action version: use upload-artifact v7 throughout.
Line 130 pins
upload-artifact@ea165f8d(v4.6.2), but line 103 uses@043fb46d(v7.0.1). Mixing major versions of the same action creates unnecessary drift—v7 is current, so unify on that for both jobs.🔧 Proposed fix
- name: Upload load test results - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 if: always() with: name: load-test-results🤖 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 130, Replace the pinned upload-artifact action to use the v7 version consistently: find the occurrence using "uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02" and change it to the same v7 pin used elsewhere (e.g. "uses: actions/upload-artifact@043fb46d" or "uses: actions/upload-artifact@v7") so both jobs use the same major version.
🤖 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.
Outside diff comments:
In @.github/workflows/benchmarks.yml:
- Line 130: Replace the pinned upload-artifact action to use the v7 version
consistently: find the occurrence using "uses:
actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02" and change it
to the same v7 pin used elsewhere (e.g. "uses: actions/upload-artifact@043fb46d"
or "uses: actions/upload-artifact@v7") so both jobs use the same major version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ba67816-bfb7-4166-98fc-17dab337052e
📒 Files selected for processing (1)
.github/workflows/benchmarks.yml
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
|
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
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
Removes the
eventbus-latency-slojob from the Benchmarks workflow. Shared GitHub runners are too inconsistent to gate on for latency benchmarking.The job's measurement always passed (p99 ≈ 20–40µs vs the 1ms threshold), but the
ipc_performancebench binary reliably hung to the 15-minute job timeout on the runners — producing a perpetual non-required red check on every PR that touched the eventbus/collector paths.Changes
eventbus-latency-slomatrix job.pull_requesttrigger that existed solely for that job (the other two jobs,benchmarksandload-tests, were alreadyworkflow_dispatch-only via per-jobifconditions). The Benchmarks workflow is now entirely manual-dispatch.What is preserved
daemoneye-eventbus/benches/ipc_performance.rs::latency_p99_slo. It's still runnable locally (cargo bench --package daemoneye-eventbus --bench ipc_performance -- '^latency_p99_slo$') and viaworkflow_dispatch.benchmarksandload-testsjobs are unchanged (still manual-dispatch).Notes
UNSTABLE, notBLOCKED), so nothing else needs updating.latency_benchmarkawaiting an infinite handler) is still worth landing for anyone running the bench manually, but it's no longer on the CI critical path.AI usage disclosure
Per AI_POLICY.md: change made with Claude Code (
Claude Opus 4.8 (1M Context)). Workflow validated with actionlint.