Add policy engine benchmark tests #826
Conversation
WalkthroughThis pull request adds comprehensive benchmarking infrastructure to the gateway policy-engine, including three new benchmark test files measuring the chain executor, ext_proc kernel, and CEL evaluator components, along with updated README documentation describing how to run these benchmarks. Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![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
🤖 Fix all issues with AI agents
In `@gateway/policy-engine/internal/kernel/extproc_bench_test.go`:
- Around line 604-620: The benchmark has a data race on writeCounter in the
"ParallelWithWrites" subtest; make writeCounter an int64 and use atomic
operations from sync/atomic to increment and read it safely (e.g., call
atomic.AddInt64(&writeCounter, 1) inside the RunParallel loop and use the
returned value to decide when to call kernel.RegisterRoute), and add the
sync/atomic import. Reference symbols: writeCounter,
b.Run("ParallelWithWrites"), kernel.RegisterRoute, kernel.GetPolicyChainForKey.
🧹 Nitpick comments (3)
gateway/policy-engine/internal/executor/chain_bench_test.go (1)
60-74: Mock evaluator has non-thread-safe counter - acceptable for current usage.The
sometimesFalseCELEvaluatoruses a non-atomic counter which would cause data races in parallel execution. This is fine since it's only used inBenchmarkExecuteRequestPolicies_CELSkippingwhich is serial, but add a comment to prevent future misuse.📝 Add thread-safety note
// sometimesFalseCELEvaluator skips policies based on index for realistic testing. +// Note: Not thread-safe. Only use in serial benchmarks. type sometimesFalseCELEvaluator struct { counter int }gateway/policy-engine/internal/kernel/extproc_bench_test.go (1)
107-173: Mock policies duplicate those inchain_bench_test.go.Consider extracting shared mock policies to a common test helper package (e.g.,
internal/testutil/) to reduce duplication. This is optional since test code duplication is less critical than production code.gateway/policy-engine/internal/pkg/cel/evaluator_bench_test.go (1)
19-25: Consider addingTestMainfor consistent benchmark isolation.The other benchmark files disable metrics via
TestMainfor clean measurements. For consistency, consider adding similar setup here if the CEL evaluator triggers any metrics.📝 Add TestMain for consistency
import ( + "os" "testing" policy "github.com/wso2/api-platform/sdk/gateway/policy/v1alpha" + "github.com/wso2/api-platform/gateway/policy-engine/internal/metrics" ) + +func TestMain(m *testing.M) { + metrics.SetEnabled(false) + metrics.Init() + os.Exit(m.Run()) +}
| b.Run("ParallelWithWrites", func(b *testing.B) { | ||
| // Simulates scenario with occasional route updates | ||
| var writeCounter int64 | ||
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
| b.RunParallel(func(pb *testing.PB) { | ||
| for pb.Next() { | ||
| // Every 1000th operation is a write | ||
| if writeCounter%1000 == 0 { | ||
| kernel.RegisterRoute("dynamic-route", chain) | ||
| } | ||
| _ = kernel.GetPolicyChainForKey("route-50") | ||
| writeCounter++ | ||
| } | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Data race on writeCounter in parallel benchmark.
writeCounter++ is not atomic and will race across goroutines in b.RunParallel. While the benchmark intent is to occasionally trigger writes, the race detector will flag this.
🔧 Use atomic counter
+import "sync/atomic"
+
b.Run("ParallelWithWrites", func(b *testing.B) {
// Simulates scenario with occasional route updates
- var writeCounter int64
+ var writeCounter atomic.Int64
b.ReportAllocs()
b.ResetTimer()
b.RunParallel(func(pb *testing.PB) {
for pb.Next() {
// Every 1000th operation is a write
- if writeCounter%1000 == 0 {
+ if writeCounter.Add(1)%1000 == 0 {
kernel.RegisterRoute("dynamic-route", chain)
}
_ = kernel.GetPolicyChainForKey("route-50")
- writeCounter++
}
})
})📝 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.
| b.Run("ParallelWithWrites", func(b *testing.B) { | |
| // Simulates scenario with occasional route updates | |
| var writeCounter int64 | |
| b.ReportAllocs() | |
| b.ResetTimer() | |
| b.RunParallel(func(pb *testing.PB) { | |
| for pb.Next() { | |
| // Every 1000th operation is a write | |
| if writeCounter%1000 == 0 { | |
| kernel.RegisterRoute("dynamic-route", chain) | |
| } | |
| _ = kernel.GetPolicyChainForKey("route-50") | |
| writeCounter++ | |
| } | |
| }) | |
| }) | |
| } | |
| b.Run("ParallelWithWrites", func(b *testing.B) { | |
| // Simulates scenario with occasional route updates | |
| var writeCounter atomic.Int64 | |
| b.ReportAllocs() | |
| b.ResetTimer() | |
| b.RunParallel(func(pb *testing.PB) { | |
| for pb.Next() { | |
| // Every 1000th operation is a write | |
| if writeCounter.Add(1)%1000 == 0 { | |
| kernel.RegisterRoute("dynamic-route", chain) | |
| } | |
| _ = kernel.GetPolicyChainForKey("route-50") | |
| } | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In `@gateway/policy-engine/internal/kernel/extproc_bench_test.go` around lines 604
- 620, The benchmark has a data race on writeCounter in the "ParallelWithWrites"
subtest; make writeCounter an int64 and use atomic operations from sync/atomic
to increment and read it safely (e.g., call atomic.AddInt64(&writeCounter, 1)
inside the RunParallel loop and use the returned value to decide when to call
kernel.RegisterRoute), and add the sync/atomic import. Reference symbols:
writeCounter, b.Run("ParallelWithWrites"), kernel.RegisterRoute,
kernel.GetPolicyChainForKey.
1 participant
Purpose
Related Issue: #825
Summary by CodeRabbit
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.