github-action-benchmark icon indicating copy to clipboard operation
github-action-benchmark copied to clipboard

Golang Benchmarks don't take Package Name into consideration

Open gaby opened this issue 1 year ago • 10 comments

When running benchmarks in a repo with a lot of packages, if two benchmarks have the same name this github-action doesn't take into account the package name cause the comparison to fail randomly.

This can be seen on this run: https://github.com/gofiber/fiber/actions/runs/10873073504

Where BenchmarkAppendMsgitem shows up multiple times with different values even though they are in different packages.

gaby avatar Sep 15 '24 18:09 gaby

Hey @gaby Thanks for reporting this! I wonder how we could deal with this. I think it will create a similar issue of backward compatibility as we faced with PR #177

We could append the package name to the name of the benchmark so it would be eg

BenchmarkAppendMsgitem (github.com/gofiber/fiber/v3/middleware/cache)

This should create a unique name to compare As a remedy to not break existing metrics we could treat benchmarks with only one package as special cases and not append the package name in those cases. Or add both cases: old (without suffix) and new (with suffix) so that it can be then dropped in the next release. WDYT?

ktrz avatar Sep 15 '24 18:09 ktrz

@ktrz Adding both cases is probably the way to go now. Other option I can think is having a config option to specify if you want benchmarks with suffix or not.

gaby avatar Sep 15 '24 18:09 gaby

@ktrz Any progress on this issue? Thanks!

gaby avatar Oct 08 '24 12:10 gaby

@ktrz what is the status? the package name after the name of the benchmark would be good and would solve the problem

ReneWerner87 avatar Nov 15 '24 08:11 ReneWerner87

problem output

PASS
ok  	github.com/gofiber/fiber/v3/log	0.173s
PASS
ok  	github.com/gofiber/fiber/v3/middleware/adaptor	0.184s
PASS
ok  	github.com/gofiber/fiber/v3/middleware/basicauth	0.173s
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3/middleware/cache
BenchmarkAppendMsgitem
BenchmarkAppendMsgitem-12    	63634455	        19.01 ns/op	3103.57 MB/s	       0 B/op	       0 allocs/op
BenchmarkAppendMsgitem-12    	66411781	        18.42 ns/op	3202.97 MB/s	       0 B/op	       0 allocs/op
PASS
ok  	github.com/gofiber/fiber/v3/middleware/cache	2.649s
PASS
ok  	github.com/gofiber/fiber/v3/middleware/compress	0.219s
PASS
ok  	github.com/gofiber/fiber/v3/middleware/cors	0.188s
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3/middleware/csrf
BenchmarkAppendMsgitem
BenchmarkAppendMsgitem-12    	1000000000	         0.2926 ns/op	3417.23 MB/s	       0 B/op	       0 allocs/op
BenchmarkAppendMsgitem-12    	1000000000	         0.2883 ns/op	3468.54 MB/s	       0 B/op	       0 allocs/op
PASS
ok  	github.com/gofiber/fiber/v3/middleware/csrf	0.842s
PASS

current code

https://github.com/benchmark-action/github-action-benchmark/blob/6bae118c112083251560ad8b3a1ff2e43aa23351/src/extract.ts#L342-L392

possible improvement

function extractGoResult(output: string): BenchmarkResult[] {
    const lines = output.split(/\r?\n/g);
    const results: BenchmarkResult[] = [];
    let currentPackage = '';

    // Regular expression to extract the package path
    const packageRegex = /^pkg:\s+(.+)$/;

    // Regular expression to extract benchmark information
    const benchmarkRegex =
        /^(?<name>Benchmark\w+[\w()$%^&*-=|,[\]{}"#]*?)(?<procs>-\d+)?\s+(?<times>\d+)\s+(?<remainder>.+)$/;

    for (const line of lines) {
        // Check if the line contains the package path
        const packageMatch = line.match(packageRegex);
        if (packageMatch) {
            currentPackage = packageMatch[1];
            continue;
        }

        // Check if the line contains benchmark information
        const benchmarkMatch = line.match(benchmarkRegex);
        if (benchmarkMatch?.groups) {
            const { name, procs, times, remainder } = benchmarkMatch.groups;
            const procsNumber = procs ? procs.slice(1) : null;
            const pieces = remainder.split(/\s+/);

            // For backward compatibility with Go benchmarks that had multiple metrics in the output,
            // but were not extracted properly before v1.18.0
            if (pieces.length > 2) {
                pieces.unshift(pieces[0], remainder.slice(remainder.indexOf(pieces[1])));
            }

            for (let i = 0; i < pieces.length; i += 2) {
                const value = parseFloat(pieces[i]);
                const unit = pieces[i + 1];
                const fullName = `${currentPackage}.${name}${i > 0 ? ' - ' + unit : ''}`;
                const extra = `${times} times${procsNumber ? `\n${procsNumber} procs` : ''}`.trim();
                results.push({ name: fullName, value, unit, extra });
            }
        }
    }

    return results;
}

I just don't know if the complete package changes the output too much you could use only part of the package name or write it in a separate property

ReneWerner87 avatar Nov 16 '24 14:11 ReneWerner87

for now I have applied a hack by hand in our workflow until you fix the problem in the action and thus help all consumers with this problem

https://github.com/gofiber/fiber/blob/9a2ceb722027464842e248a38efce2c3fd67d1c6/.github/workflows/benchmark.yml#L38-L67

image

ReneWerner87 avatar Dec 01 '24 14:12 ReneWerner87

@ktrz Any update on this?

gaby avatar Mar 29 '25 13:03 gaby

I came across this and in general I would like this to be implemented with an move to use the go test -bench -json output as input. It provides the package name every line but unfortunately you will still have to regex the timings.

{"Time":"2025-05-04T16:20:02.3122798+02:00","Action":"output","Package":"test.com/package/name","Test":"BenchmarkAdd/5000","Output":" 10000\t 106952 ns/op\n"}

But I believe this is more reliable to parse than the generic cli output.

donmahallem avatar May 04 '25 14:05 donmahallem

Hi,

I have the same problem. Thank you for the manual fix @ReneWerner87. I am also considering renaming the benchmark in the packages, so that they do not share the same name.

chmduquesne avatar Sep 10 '25 16:09 chmduquesne

@ktrz is there any update? can we overtake my idea with the package name ?

ReneWerner87 avatar Nov 19 '25 14:11 ReneWerner87