[Performance] Improve fleet server benchmarks

[jlind23]

Thanks to #3141 we now have fleet server benchmarks running on every PR.
As of today this is not a blocking check as we found some flakiness in these benchmarks.

Looking at the comparison results here and in the screenshot below, the performance difference is higher than the 10% threshold we defined. We need to understand why there is so much and fix the flakiness.

Outcome

• The outcome of this issue is to make the benchmark run a blocker for the Buildkite pipeline

[jlind23]

@scunningham according to the git history you are the one who introduced these benchmarks a couple of years ago. Do you have any insights to share onto why we might see some performance discrepancies while testing the exact same code twice?

cc @alexsapran

[scunningham]

@scunningham according to the git history you are the one who introduced these benchmarks a couple of years ago. Do you have any insights to share onto why we might see some performance discrepancies while testing the exact same code twice?

cc @alexsapran

Hard to say, much has changed in Fleet Server since I worked on it. Would require some investigation, but I'd be interested in network performance and error rates. When I last tested, most of the performance issue were Elastic related, particularly in auth latency amongst others.

[alexsapran]

Another improvement we can do is the following, the BK annotation changes the status depending on the outcome of the step.
In addition to the BK annotation, we could also post to the https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status with a context, benchmarks and have it green|red.

[michel-laterman]

There's an upstream issue for inconsistent benchmark results when the CPU numbers are pinned, golang/go#31599, but I'm not sure if that effects us here.

While we have some benchmarks defined in *_integration_test.go files, I don't think they run or are part of the (failing) comparisons.

I'll try to make sure timers are reset before the benchmarks are ran in order to see if that cleans it up

[michel-laterman]

I think the errors may be caused by how gobenchdata analyses the benchmarks output.
The 1st test run shows a difference of over 13% for github.com/elastic/fleet-server/v7/internal/pkg/checkin.BenchmarkBulk_1-8.

However if I download the base and next artifacts and compare them with benchstat we see that takes all runs into account:

pkg: github.com/elastic/fleet-server/v7/internal/pkg/checkin
                   │  ./base.out  │             ./next.out             │
                   │    sec/op    │   sec/op     vs base               │
Bulk_1-8             71.19n ±  0%   71.02n ± 0%   -0.24% (p=0.003 n=8)

[jlind23]

I think the errors may be caused by how gobenchdata analyses the benchmarks output.

Any solution that you believe would help us out?

Scratch that, just saw the PR you linked to this issue.

[michel-laterman]

I thought that the issue may be due to the tool, but it seems like there another contributing factor.

I ran make benchmark a few times then manually fed the results into benchstat.
Allocations remained constant, but it looks like timings may vary greatly, for example;

pkg: github.com/elastic/fleet-server/v7/internal/pkg/bulk
                          │   pr.out    │               pr2.out               │              pr3.out               │
                          │   sec/op    │    sec/op     vs base               │   sec/op     vs base               │
MockBulk/1-16               89.63µ ± 4%   92.44µ ± 12%        ~ (p=0.161 n=8)   83.31µ ± 2%   -7.06% (p=0.000 n=8)
MockBulk/8-16               261.0µ ± 3%   335.0µ ±  8%  +28.35% (p=0.000 n=8)   250.5µ ± 2%   -4.04% (p=0.005 n=8)
MockBulk/64-16              1.241m ± 3%   1.397m ± 15%  +12.57% (p=0.000 n=8)   1.238m ± 3%        ~ (p=0.959 n=8)
MockBulk/4096-16            130.3m ± 6%   136.6m ± 13%   +4.82% (p=0.028 n=8)   113.6m ± 6%  -12.85% (p=0.000 n=8)
MockBulk/32768-16            1.180 ± 1%    1.139 ±  3%   -3.50% (p=0.028 n=8)    1.066 ± 5%   -9.69% (p=0.000 n=8)
MultiUpdateMock/1-16        1.540µ ± 4%   1.649µ ±  8%   +7.04% (p=0.021 n=8)   1.686µ ± 6%   +9.44% (p=0.003 n=8)
MultiUpdateMock/8-16        5.319µ ± 4%   6.159µ ±  3%  +15.79% (p=0.000 n=8)   5.345µ ± 2%        ~ (p=0.777 n=8)
MultiUpdateMock/64-16       30.96µ ± 7%   31.97µ ± 10%        ~ (p=0.279 n=8)   27.90µ ± 3%   -9.87% (p=0.001 n=8)
MultiUpdateMock/4096-16     1.846m ± 6%   1.792m ±  9%        ~ (p=0.279 n=8)   1.670m ± 2%   -9.56% (p=0.001 n=8)
MultiUpdateMock/32768-16    15.22m ± 3%   14.24m ±  5%   -6.44% (p=0.000 n=8)   13.39m ± 6%  -12.01% (p=0.001 n=8)
MultiUpdateMock/131072-16   57.05m ± 2%   57.28m ±  4%        ~ (p=0.798 n=8)   52.30m ± 6%   -8.33% (p=0.000 n=8)
geomean                     1.056m        1.111m         +5.27%                 992.4µ        -6.00%

[michel-laterman]

I've merged the PR to make the benchmarks more consistent (hopefully).
But I have not forced the PR to fail if the benchmarks detect an issue, i think there is a big difference between how gobenchdata and benchstat compares the results.
I think benchdiff shows some promise as it uses benchstat to compare, but i have not looked into it too much