Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mefarazath
Copy link
Contributor

@mefarazath mefarazath commented Oct 16, 2024

This PR introduces changes to,

  • Run opa filter benchmarks with and without decision logs
  • Run the benchmarks in parallel to possibly detect contention issues and overall performance under load
  • Measure percentiles in benchmark tests (inspired by Print percentiles in benchmark  #2699)

@mefarazath mefarazath force-pushed the benchmark-with-decision-logging branch 6 times, most recently from 4cbf4ab to 476b6d5 Compare October 21, 2024 08:16
@mefarazath mefarazath marked this pull request as ready for review October 21, 2024 09:28
@mefarazath
Copy link
Contributor Author

mefarazath commented Oct 21, 2024

Sample benchmark run measuring percentiles

go test -benchmem -run=^$ -bench ^BenchmarkAuthorizeRequest$ -benchtime=3s
BenchmarkAuthorizeRequest/minimal/no-decision-logs-10         	  653377	      4862 ns/op	   26142 B/op	     487 allocs/op
BenchmarkAuthorizeRequest/minimal/no-decision-logs/P50-10     	  653377	     31459 ns/op
BenchmarkAuthorizeRequest/minimal/no-decision-logs/P99.5-10   	  653377	    396833 ns/op
BenchmarkAuthorizeRequest/minimal/no-decision-logs/P99.9-10   	  653377	    710041 ns/op

BenchmarkAuthorizeRequest/minimal/with-decision-logs-10       	   62737	     51580 ns/op	   79947 B/op	    1331 allocs/op
BenchmarkAuthorizeRequest/minimal/with-decision-logs/P50-10   	   62737	     97250 ns/op
BenchmarkAuthorizeRequest/minimal/with-decision-logs/P99.5-10 	   62737	  15023917 ns/op
BenchmarkAuthorizeRequest/minimal/with-decision-logs/P99.9-10 	   62737	  15544875 ns/op

BenchmarkAuthorizeRequest/with-body/no-decision-logs-10       	  335612	     78599 ns/op	   35122 B/op	     513 allocs/op
BenchmarkAuthorizeRequest/with-body/no-decision-logs/P50-10   	  335612	    590375 ns/op
BenchmarkAuthorizeRequest/with-body/no-decision-logs/P99.5-10 	  335612	   3332458 ns/op
BenchmarkAuthorizeRequest/with-body/no-decision-logs/P99.9-10 	  335612	   4318250 ns/op

BenchmarkAuthorizeRequest/with-body/with-decision-logs-10     	   61664	     63934 ns/op	   91055 B/op	    1404 allocs/op
BenchmarkAuthorizeRequest/with-body/with-decision-logs/P50-10 	   61664	    240916 ns/op
BenchmarkAuthorizeRequest/with-body/with-decision-logs/P99.5-10	   61664	  14367416 ns/op
BenchmarkAuthorizeRequest/with-body/with-decision-logs/P99.9-10	   61664	  15672583 ns/op

BenchmarkAuthorizeRequest/jwt-validation/no-decision-logs-10  	          211809	     16336 ns/op	   60280 B/op	     964 allocs/op
BenchmarkAuthorizeRequest/jwt-validation/no-decision-logs/P50-10	  211809	    103541 ns/op
BenchmarkAuthorizeRequest/jwt-validation/no-decision-logs/P99.5-10	  211809	   1552625 ns/op
BenchmarkAuthorizeRequest/jwt-validation/no-decision-logs/P99.9-10	  211809	   2376417 ns/op

BenchmarkAuthorizeRequest/jwt-validation/with-decision-logs-10         	   47227	     74450 ns/op	  122129 B/op	    1808 allocs/op
BenchmarkAuthorizeRequest/jwt-validation/with-decision-logs/P50-10     	   47227	    217292 ns/op
BenchmarkAuthorizeRequest/jwt-validation/with-decision-logs/P99.5-10   	   47227	  16026917 ns/op
BenchmarkAuthorizeRequest/jwt-validation/with-decision-logs/P99.9-10   	   47227	  17956916 ns/op

@mjungsbluth mjungsbluth added minor no risk changes, for example new filters performance labels Oct 21, 2024
@szuecs
Copy link
Member

szuecs commented Oct 21, 2024

best would be to show benchstat results like #2876
you need 2 files for example old.txt and new.txt to compare old vs new.

@AlexanderYastrebov
Copy link
Member

best would be to show benchstat results like #2876

See https://pkg.go.dev/golang.org/x/perf/cmd/benchstat

@mefarazath
Copy link
Contributor Author

mefarazath commented Oct 21, 2024

@szuecs @AlexanderYastrebov

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.

➜  benchmark-results benchstat v0.68.0.txt after_fix.txt                                                                                                                              7ms
goos: darwin
goarch: arm64
pkg: github.com/zalando/skipper/filters/openpolicyagent/opaauthorizerequest
cpu: Apple M1 Max
                                                            │  v0.68.0.txt   │            after_fix.txt             │
                                                            │     sec/op     │    sec/op     vs base                │
AuthorizeRequest/jwt-validation/with-decision-logs-10           70.05µ ±  1%   22.51µ ±  2%  -67.87% (p=0.000 n=10)
AuthorizeRequest/jwt-validation/with-decision-logs/P50-10       201.6µ ±  3%   192.5µ ±  2%   -4.52% (p=0.000 n=10)
AuthorizeRequest/jwt-validation/with-decision-logs/P99-10     15299.9µ ±  1%   654.3µ ±  4%  -95.72% (p=0.000 n=10)
AuthorizeRequest/jwt-validation/with-decision-logs/P99.5-10   15798.8µ ±  1%   780.5µ ±  4%  -95.06% (p=0.000 n=10)
AuthorizeRequest/jwt-validation/with-decision-logs/P99.9-10    16.547m ±  1%   1.226m ± 33%  -92.59% (p=0.000 n=10)
geomean                                                         818.0µ         345.9µ        -57.71%

                                                      │  v0.68.0.txt  │            after_fix.txt             │
                                                      │     B/op      │     B/op      vs base                │
AuthorizeRequest/jwt-validation/with-decision-logs-10   119.34Ki ± 0%   89.59Ki ± 0%  -24.93% (p=0.000 n=10)
geomean                                                  59.12Ki        49.91Ki       -15.58%

                                                      │ v0.68.0.txt │             after_fix.txt             │
                                                      │  allocs/op  │  allocs/op   vs base                  │
AuthorizeRequest/jwt-validation/with-decision-logs-10   1.809k ± 0%   1.520k ± 0%  -15.98% (p=0.000 n=10)
geomean                                                  966.2         864.7       -10.50%
¹ all samples are equal

@AlexanderYastrebov
Copy link
Member

Couple of nits should be fixed, otherwise looks ok.
Please also squash into a single commit with a detailed commit message.

@mefarazath mefarazath force-pushed the benchmark-with-decision-logging branch from 476b6d5 to b640adc Compare October 28, 2024 20:25
@mefarazath
Copy link
Contributor Author

mefarazath commented Nov 1, 2024

@AlexanderYastrebov addressed the comments.

@MustafaSaber
Copy link
Member

👍

- 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]>
@mefarazath mefarazath force-pushed the benchmark-with-decision-logging branch from f52093e to d492727 Compare November 6, 2024 20:02
@MustafaSaber
Copy link
Member

👍

@MustafaSaber
Copy link
Member

Stopped the merging as requested from @mefarazath, once the PR is ready again will remove the do-not-merge label and review again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge minor no risk changes, for example new filters performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants