fix(bench): move collector config out of workflows/ + unthrottle telemetrygen (PR-N1.2) - #72
Conversation
…metrygen (PR-N1.2) Two first-dispatch failures from PR-N1, both fixed here: 1. `.github/workflows/bench-collector.yaml` made GitHub try to parse the OTel collector config as a workflow definition on every push to main (everything under workflows/ is scanned as a workflow). Moved to `.github/bench-collector.yaml` — still version-controlled beside bench.yml, no longer workflow-scanned — and updated the `--config` path. 2. telemetrygen crawled at 1 log/sec: its `logs` subcommand hardcodes `Rate = 1` (config.go), and only `--rate 0` selects the unbounded `rate.Inf` path (logs.go). 1M records at 1/sec is ~11.5 days — the 4h hang on the first dispatch. Added `--rate 0`, plus a `timeout 600` backstop so any future throttle/stall fails the step in 10 min with the collector log dumped, instead of burning a runner to GitHub's 6h ceiling. Captures the real exit code via `|| tg_rc=$?` (the `if ! …; then` form reports $? as 0 inside the branch). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBenchmark workflow infrastructure is reorganized: the OpenTelemetry Collector config moves from ChangesBenchmark Infrastructure Improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes the benchmark workflow’s telemetry capture path so the OTel collector config is no longer parsed as a workflow and telemetry generation completes within a bounded time.
Changes:
- Moves the collector config reference from
.github/workflows/to.github/. - Adds
--rate 0to unthrottletelemetrygen logs. - Wraps telemetry generation with a 10-minute timeout and collector-log dump on failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/bench.yml |
Updates collector config path and hardens telemetrygen execution with unbounded rate plus timeout handling. |
.github/bench-collector.yaml |
Updates comments to document why the collector config lives outside .github/workflows/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What
Fixes the two failures PR-N1 (#70) hit on its first real dispatch.
1. Collector config was in the workflow-scan directory
.github/workflows/bench-collector.yaml(the OTel collector config from PR-N1) made GitHub try to parse it as a workflow definition on every push tomain— everything under.github/workflows/is scanned as a workflow — producing a failing run on each push (example: run 26706450810).Moved to
.github/bench-collector.yaml(one level up, still version-controlled besidebench.yml, no longer workflow-scanned —.github/FUNDING.ymlalready lives there as precedent). Updated the--configpath inbench.ymland both files' self-describing comments.2. telemetrygen crawled at 1 log/sec
The second dispatch (run 26706451070) got past install but the capture step ran for ~4h before I cancelled it. The worker log showed
exported batched logs count: 100every ~100 seconds = 1 log/sec.Root cause, traced to source at the pinned
v0.153.0:cmd/telemetrygen/pkg/logs/config.go:54→c.Rate = 1(hardcoded default of 1 log/sec).cmd/telemetrygen/pkg/logs/logs.go:53-55→ only--rate 0selects the unboundedrate.Infpath.1,000,000 records at 1/sec ≈ 11.5 days. Added
--rate 0. Withrate.Inf, 1M records is seconds.3. Backstop against future hangs
Wrapped telemetrygen in
timeout 600. Any future re-introduction of throttling (or a genuine collector stall) now fails the step in 10 min — with/tmp/collector.logdumped — instead of burning a runner to GitHub's 6h job ceiling. The real exit code is captured via|| tg_rc=$?rather thanif ! …; then(which reports$?as 0 inside the branch;124= the timeout backstop).CLAUDE.md §6.6
cargo fmt --all --check,cargo test --all-features(322/0/44),cargo clippy,mdbook build— all green (no Rust changes; baseline preserved).yaml.safe_load.Out of scope
The dispatch + the §9 row in
docs/benchmarks.mdfollow this merge (PR-N2), once we have a non-hung run producing real A1/C1/C2 numbers onci-runner+telemetrygen-synthetic-v1.🤖 Generated with Claude Code
Summary by CodeRabbit