-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve opa filter benchmarks #3278
base: master
Are you sure you want to change the base?
Improve opa filter benchmarks #3278
Conversation
4cbf4ab
to
476b6d5
Compare
Sample benchmark run measuring percentiles
|
best would be to show |
|
Apologies for the confusion. This PR simply improves the existing benchmark code to add more measurements to get better insights into latency distribution and does not add anything to improve the performance. Added sub benchmarks also helps us to get an idea for each scenario with and without decision logging enabled. This would gives us a quick way to verify/validate performance improvements proposed to filter and opa with better clarity. As an example I did a comparison between v0.68.0 (current version in skipper) vs a PoC done by OPA team to improve decision logs performance. The p995 and p999 gave a better idea on the impact of the improvements here.
|
|
||
privKey, err := os.ReadFile(keyPath) | ||
if err != nil { | ||
log.Fatalf("Failed to read priv key: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not require.NoError(t, err) here and below?
url, err := url.Parse("http://opa-authorized.test/somepath") | ||
assert.NoError(b, err) | ||
f, err := createOpaFilter(opaControlPlane, decisionLogConsumer, decisionLogging) | ||
assert.NoError(b, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use require.* (require.NoError in this case) to check preconditions and stop test early and assert.* to verify expected outcomes being tested.
Couple of nits should be fixed, otherwise looks ok. |
- Modify benchmark tests to run with decision logs both enabled and disabled in the OPA filter - Execute benchmarks in parallel to identify contention issues and evaluate CPU vertical scaling - Measure response time percentiles for deeper performance insights Signed-off-by: Farasath Ahamed <[email protected]>
476b6d5
to
b640adc
Compare
This PR introduces changes to,