perf(ci): make the byoo collector genrule remote-cacheable#444
Open
balajinvda wants to merge 3 commits into
Open
perf(ci): make the byoo collector genrule remote-cacheable#444balajinvda wants to merge 3 commits into
balajinvda wants to merge 3 commits into
Conversation
The byoo-otel-collector row is the single critical path of the GHA bazel
matrix at ~19.9 min while every other row finishes in 3-8 min. With
max-parallel 8, total matrix wall clock is essentially byoo's time.
Measured on run 30114563605, job 89552120446: `bazel build` took 18m34s
of the row's 20m56s, and Bazel reported
"383 processes: 317 remote cache hit, 59 internal, 3 local". Everything
in the module is already a cache hit except three actions: the collector
genrule, which is analysed in three configurations (host,
//platforms:linux_x86_64, //platforms:linux_arm64). Two of them ran cold
for 538s and 510s.
Two causes, both fixed here.
1. The genrule carried `local = True`. Bazel's `local` tag is the union
of no-sandbox + no-remote-exec + no-cache, and no-cache meant the
collector was never written to or read from the remote cache. The
whole 250+ module OTel graph therefore recompiled on every run even
when otelcol/ was untouched. Replacing `local = True` with the two
execution requirements the action actually needs (no-sandbox because
it shells out to the host `go`, no-remote-exec because that `go` only
exists locally) keeps execution semantics identical while letting an
unchanged collector resolve to a cache hit.
Verified with a disk cache stand-in: cold build 81.7s, then
`bazel clean` + rebuild is "1 disk cache hit" in 0.87s. Building
--platforms=//platforms:linux_arm64 against the same cache is a miss
and produces an aarch64 ELF, so GOOS/GOARCH in the command line keeps
the two architectures on separate keys. Feeding a different
BYOO_GO_TOOLCHAIN value is also a miss.
2. `go build -p 1` serialized the compile of all 250+ modules. The
stated justification was linker RSS, but peak RSS comes from the
`go build` driver and the final link, neither of which scales with
compile parallelism. Measured cold on otelcol v0.157.0:
-p 1 490s 5.69 GB peak RSS
-p 2 211s 6.09 GB
-p 4 97s 6.08 GB
-p 8 58s 5.27 GB
Parallelism now follows nproc, capped at 8.
The real memory guard is not running two collector builds concurrently,
which CI already enforces with --jobs=1. That flag is kept and its
comment corrected: it is a memory guard, not a throughput choice, and it
is the only lever available because Bazel ignores "resources:memory:N",
"cpu:N", and "exclusive" execution requirements for genrules (verified
on Bazel 8.6.0). The misleading "exclusive" tag is dropped for the same
reason.
The genrule shells out to a `go` that is not a declared Bazel input, so
enabling remote caching without binding the toolchain would let a Go
bump in the bazel-ci image keep serving binaries built by the previous
compiler, including across a Go security patch. CI now passes
--action_env=BYOO_GO_TOOLCHAIN="$(go version)" on both the build and
test steps, which puts the toolchain identity in the action key.
Expected effect: runs where otelcol/ is unchanged (the common case,
including every full-matrix fallback) drop from ~19.9 min to roughly
3-4 min, bounded by downloading the cached binaries. Genuinely cold
runs, such as an ocb regeneration or the first main push after this
lands, drop from ~19.9 min to roughly 7 min.
Refs: #373
Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe otelcol genrule now uses host-derived parallelism, recursively tracks Go sources, and runs with updated execution tags. Bazel CI binds the host Go toolchain version into build and test action keys, while documentation and version metadata are updated. ChangesOtelcol build execution and cache alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@src/compute-plane-services/byoo-otel-collector/otelcol/BUILD.bazel`:
- Around line 146-175: Update the README section describing the otel collector
genrule to replace the outdated local = True claim with the current no-sandbox
and no-remote-exec execution requirements. Document that the action remains
cache-eligible, including the relevant cache-key handling, and keep the
rationale consistent with the BUILD.bazel configuration.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef280243-f601-498e-9c50-2553780f2203
📒 Files selected for processing (2)
.github/workflows/bazel.ymlsrc/compute-plane-services/byoo-otel-collector/otelcol/BUILD.bazel
The README still described the genrule as using local = True and claimed remote-cache reuse via the decommissioned internal cache. Both are now wrong: the tags are no-sandbox + no-remote-exec, the action is cache-eligible, and the host Go toolchain is bound into the action key via --action_env so a compiler bump cannot serve stale binaries. VERSION 0.157.0 -> 0.157.1. The subtree has a gate requiring a VERSION bump whenever any file under it changes, and this PR necessarily changes otelcol/BUILD.bazel. Patch-level only, so the major/minor stays in lockstep with the collector version in otel-collector-build.yaml, which the gate also cross-checks. The produced binary is unchanged in content; only how it is built and cached changes. This also clears the last reference to the retired cache host, which #435 had to leave in place precisely because that PR could not justify a VERSION bump for a comment-only edit. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
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 `@src/compute-plane-services/byoo-otel-collector/README.md`:
- Around line 74-76: Align the cache-input documentation with the genrule’s
actual inputs by updating the README’s “Cache contract” near the documented Go
patterns and the corresponding srcs declaration in otelcol BUILD.bazel. Either
document only the currently declared root-level and logchunkprocessor Go files,
or expand srcs to include every otelcol/**/*.go subtree; keep the README and
BUILD.bazel consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4b3f66dc-9c5f-4841-a91f-01452f60d809
📒 Files selected for processing (2)
src/compute-plane-services/byoo-otel-collector/README.mdsrc/compute-plane-services/byoo-otel-collector/VERSION
The README claimed otelcol/**/*.go invalidates the genrule, but the glob declared only root-level *.go plus logchunkprocessor/**/*.go. Today those are the same file set (4 root + 5 in logchunkprocessor = all 9), so this was not a live bug, but it was a trap: a new subpackage would have been silently excluded from the action key and edits to it would not have invalidated the build. Since the action is now cacheable, a missing input would mean serving a stale binary rather than merely rebuilding. Widened the glob to **/*.go instead of documenting the narrower one, and made the README precise about which files are declared. Verified the resolved input set is unchanged: bazel query labels(srcs, ...) returns the identical 11 labels before and after. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
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.
Why
bazel (byoo-otel-collector)is the critical path of the whole GitHub Bazel matrix. Measured on a full-matrix run (job 89552120446): the row took 20m56s, of whichbazel buildalone was 18m34s. Every other row finishes in 3 to 8 minutes, and withmax-parallelthe matrix wall clock is essentially byoo's time.The Bazel summary from that run shows where the time was not going:
Everything in the module was already a cache hit except three actions:
//otelcol:otelcol-contrib-binin its three configurations. Two root causes, neither of which is the module download (#399 already handled that; the download messages appear at the end of the long action, so they are not the cost).1.
local = Truedisabled caching entirelyBazel's
localtag is the union ofno-sandbox,no-remote-exec, andno-cache. That last component meant the collector was never written to or read from the cache, so it recompiled from scratch on every run even with a fully warm cache.The genrule genuinely needs the first two: it shells out to a host
gothat is not a declared input, so it cannot be sandboxed or dispatched to a remote worker. It does not need the third. The tags are nowno-sandbox+no-remote-exec, which preserves execution semantics and restores cacheability.2.
go build -p 1serialized a 250+ module compileThe
-p 1cap was justified by linker RSS. That justification does not hold: peak RSS comes from thego builddriver holding export data and from the final link, neither of which scales with compile parallelism. Measured cold (warm module cache, otelcol v0.157.0):-pRSS is flat, wall time is 5x. Parallelism now defaults to
nproccapped at 8, still overridable by env.What changed
otelcol/BUILD.bazel: droplocal = Truein favour ofno-sandbox+no-remote-exec; drop theexclusivetag, which is a no-op for this purpose (verified: twoexclusive-tagged genrules still run concurrently under--jobs=4); defaultGO_BUILD_P/GOMAXPROCStonproccapped at 8..github/workflows/bazel.yml: add--action_env=BYOO_GO_TOOLCHAIN="$(go version)"to the byoo build and test steps. This is the correctness guard for making the action cacheable: the genrule shells out to a hostgothat Bazel does not track, so without binding the toolchain into the action key, a Go bump in the CI image would keep serving binaries built by the previous compiler, including across a security patch. Also corrected the--jobs=1comments.--jobs=1is kept. It is load-bearing, but for memory rather than the reason previously stated: each collector build peaks near 6 GB, so two concurrent ones do not fit a 16 GB runner, and Bazel has no per-action memory throttle. It costs nothing elsewhere because the other ~380 actions are cache hits.Customer Release Notes
Not customer visible (CI only).
Testing
Verified independently of the analysis above, on this branch rebased onto current
main:Cacheability, the core claim:
Cross-architecture correctness, since making an untracked-toolchain action cacheable is exactly where poisoning would show up:
//platforms:linux_arm64//platforms:linux_arm64after cleanThe last row is the important one: an arm64 cache entry does not satisfy an amd64 request.
GOOS/GOARCHare literal in theselect()-produced command line and therefore part of the action key.Notes
Expected effect: runs where
otelcol/is unchanged (the common case, including every full-matrix fallback) go from about 20 minutes to roughly 3 to 4, bounded by fetching the cached binaries (~860 MB across three configs under--remote_download_all). Genuinely cold runs go to roughly 7 minutes from the parallelism change alone. Matrix total should move from about 25 minutes to about 10.Deliberately not done here:
--jobsnot raised to 2. It would cut the cold path further, but 2 x ~6 GB does not fit a 16 GB runner alongside the Bazel JVM.*_entrypoint_mode_testtargets depend on the untransitioned layer. It is cheap in time but costs a third cached artifact; pointing those tests at the transitioned layer is a reasonable follow-up.References
Run 30114563605, job 89552120446.
-p 1and--jobs=1both originate from #194.Related Merge Requests/Pull Requests
None.
Dependencies
None.
Summary by CodeRabbit
Performance
Reliability
Documentation
Chores