chore: version selection benchmarking#1064
Conversation
📝 WalkthroughWalkthroughA benchmark test file is introduced for the versionselector package to measure CEL selector performance, comparing compiled CEL program execution against native Go equality checks and evaluating compilation costs with deterministic synthetic deployment metadata. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a Go benchmark suite to measure CEL-based deployment version metadata selector performance, addressing issue #1062.
Changes:
- Introduces benchmarks for steady-state CEL evaluation across selector shapes, corpus sizes, and metadata map sizes.
- Adds a native Go baseline benchmark for the
env == "prod"check to estimate CEL overhead. - Adds a compile-only benchmark intended to measure compilation cost without program-cache hits.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i := range n { | ||
| meta := make(map[string]any, mapSize) | ||
|
|
There was a problem hiding this comment.
for i := range n is invalid because range can’t iterate over an int. This won’t compile; use an indexed loop to fill contexts (e.g., increment i from 0..n-1).
| for k := range mapSize - len(meta) { | ||
| meta[fmt.Sprintf("filler_%d", k)] = fmt.Sprintf("value_%d", r.Intn(1_000_000)) | ||
| } |
There was a problem hiding this comment.
for k := range mapSize - len(meta) is invalid (range can’t iterate over an int, and mapSize - len(meta) is an int expression). This prevents the benchmark from compiling; switch to a standard counted loop, and guard against mapSize < len(meta) if that’s possible.
| for i := 0; i < b.N; i++ { | ||
| matches = 0 | ||
| for _, ctx := range contexts { | ||
| ok, _ := evaluate(program, ctx) |
There was a problem hiding this comment.
The benchmark ignores the error returned by evaluate(program, ctx). If evaluation fails (e.g., due to a non-missing-key runtime error), the benchmark will silently treat it as a non-match and continue, which can invalidate results. Consider failing the benchmark on non-nil error (or at least counting/reporting errors).
| ok, _ := evaluate(program, ctx) | |
| ok, err := evaluate(program, ctx) | |
| if err != nil { | |
| b.Fatalf("evaluate %q: %v", shape.label, err) | |
| } |
| // ristretto cache in compiledEnv by building a fresh env per run. Useful to | ||
| // size cache-miss impact separately from the steady-state Eval cost. |
There was a problem hiding this comment.
The comment claims BenchmarkMetadataSelector_Compile bypasses the cache “by building a fresh env per run”, but compileUncached uses compiledEnv.Env() which returns the existing underlying *cel.Env (no new env is created). Update the comment to reflect it only bypasses the ristretto program cache, or create a new env per iteration if that’s what you want to measure.
| // ristretto cache in compiledEnv by building a fresh env per run. Useful to | |
| // size cache-miss impact separately from the steady-state Eval cost. | |
| // ristretto program cache in compiledEnv. Useful to size cache-miss impact | |
| // separately from the steady-state Eval cost. |
![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 the current code and only fix it if needed.
Inline comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/metadata_selector_bench_test.go`:
- Around line 112-116: The benchmark currently ignores errors from evaluate (ok,
_ := evaluate(...)), which can hide failures; change the call to capture the
error (ok, err := evaluate(program, ctx)) and handle it (e.g., b.Fatalf or
b.Errorf + continue) so the benchmark fails loudly on evaluator errors; keep the
matches increment logic only when ok is true and include the error in the
failure message to aid debugging.
🪄 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: Pro
Run ID: 07deb073-a2c8-4241-96e1-a0dfe3758a83
📒 Files selected for processing (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/metadata_selector_bench_test.go
| for i := 0; i < b.N; i++ { | ||
| matches = 0 | ||
| for _, ctx := range contexts { | ||
| ok, _ := evaluate(program, ctx) | ||
| if ok { |
There was a problem hiding this comment.
Handle evaluator errors instead of dropping them.
At Line 115, the benchmark discards evaluate errors (ok, _ := ...). If eval starts failing (type changes, env changes, expression edge cases), the benchmark silently reports misleading throughput.
Proposed fix
- ok, _ := evaluate(program, ctx)
+ ok, err := evaluate(program, ctx)
+ if err != nil {
+ b.Fatalf("evaluate shape=%q failed: %v", shape.label, err)
+ }
if ok {
matches++
}📝 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.
| for i := 0; i < b.N; i++ { | |
| matches = 0 | |
| for _, ctx := range contexts { | |
| ok, _ := evaluate(program, ctx) | |
| if ok { | |
| for i := 0; i < b.N; i++ { | |
| matches = 0 | |
| for _, ctx := range contexts { | |
| ok, err := evaluate(program, ctx) | |
| if err != nil { | |
| b.Fatalf("evaluate shape=%q failed: %v", shape.label, err) | |
| } | |
| if ok { | |
| matches++ | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/metadata_selector_bench_test.go`
around lines 112 - 116, The benchmark currently ignores errors from evaluate
(ok, _ := evaluate(...)), which can hide failures; change the call to capture
the error (ok, err := evaluate(program, ctx)) and handle it (e.g., b.Fatalf or
b.Errorf + continue) so the benchmark fails loudly on evaluator errors; keep the
matches increment logic only when ok is true and include the error in the
failure message to aid debugging.
resolves #1062
Summary by CodeRabbit