From 3e7f1f06ddc55c94ae1655e932f95d9dd8f84c32 Mon Sep 17 00:00:00 2001 From: Owen Shepherd Date: Mon, 10 Feb 2025 03:25:46 +0000 Subject: [PATCH] Add performance improvements to alert comment --- src/write.ts | 107 +++++++++++++++++++++------- test/buildComment.test.ts | 10 ++- test/write.spec.ts | 144 +++++++++++++++++++++++++++++--------- 3 files changed, 203 insertions(+), 58 deletions(-) diff --git a/src/write.ts b/src/write.ts index e1ad4eafd..74840f92f 100644 --- a/src/write.ts +++ b/src/write.ts @@ -94,10 +94,11 @@ interface Alert { ratio: number; } -function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number): Alert[] { +function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number): [Alert[], Alert[]] { core.debug(`Comparing current:${curSuite.commit.id} and prev:${prevSuite.commit.id} for alert`); - const alerts = []; + const losses = []; + const gains = []; for (const current of curSuite.benches) { const prev = prevSuite.benches.find((b) => b.name === current.name); if (prev === undefined) { @@ -107,16 +108,18 @@ function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number const ratio = getRatio(curSuite.tool, prev, current); - if (ratio > threshold) { + if (threshold === 0 || ratio > threshold) { core.warning( `Performance alert! Previous value was ${prev.value} and current value is ${current.value}.` + - ` It is ${ratio}x worse than previous exceeding a ratio threshold ${threshold}`, + ` It is ${ratio}x worse than previous, exceeding ratio threshold ${threshold}`, ); - alerts.push({ current, prev, ratio }); + losses.push({ current, prev, ratio }); + } else if (ratio < 1 / threshold) { + gains.push({ current, prev, ratio }); } } - return alerts; + return [losses, gains]; } function getCurrentRepoMetadata() { @@ -175,7 +178,10 @@ export function buildComment( '', expandableDetails ? '
' : '', '', - `| Benchmark suite | Current: ${curSuite.commit.id} | Previous: ${prevSuite.commit.id} | Ratio |`, + `Previous: ${prevSuite.commit.id}`, + `Current: ${curSuite.commit.id}`, + '', + `| Benchmark suite | Current | Previous | Ratio |`, '|-|-|-|-|', ]; @@ -200,8 +206,49 @@ export function buildComment( return lines.join('\n'); } +function pushResultLines(results: Alert[], output: string[]) { + results.sort((a, b) => a.ratio - b.ratio); + for (const alert of results) { + const { current, prev, ratio } = alert; + const line = `| \`${current.name}\` | ${strVal(current)} | ${strVal(prev)} | \`${floatStr(ratio)}\` |`; + output.push(line); + } +} + +const RESULT_TABLE_HEADER = ['', `| Benchmark suite | Current | Previous | Ratio |`, '|-|-|-|-|']; + +function buildReportComment( + results: Alert[], + benchName: string, + curSuite: Benchmark, + prevSuite: Benchmark, + cc: string[], +): string { + // Do not show benchmark name if it is the default value 'Benchmark'. + const benchmarkText = benchName === 'Benchmark' ? '' : ` **'${benchName}'**`; + const lines = [ + '# Performance Report', + '', + `For benchmark${benchmarkText}.`, + '', + `Previous commit: ${prevSuite.commit.id}`, + `Current commit: ${curSuite.commit.id}`, + ]; + + lines.push(...RESULT_TABLE_HEADER); + pushResultLines(results, lines); + lines.push('', commentFooter()); + + if (cc.length > 0) { + lines.push('', `CC: ${cc.join(' ')}`); + } + + return lines.join('\n'); +} + function buildAlertComment( - alerts: Alert[], + losses: Alert[], + gains: Alert[], benchName: string, curSuite: Benchmark, prevSuite: Benchmark, @@ -209,23 +256,27 @@ function buildAlertComment( cc: string[], ): string { // Do not show benchmark name if it is the default value 'Benchmark'. - const benchmarkText = benchName === 'Benchmark' ? '' : ` **'${benchName}'**`; - const title = threshold === 0 ? '# Performance Report' : '# :warning: **Performance Alert** :warning:'; + const benchmarkText = benchName === 'Benchmark' ? '' : ` for **'${benchName}'**`; const thresholdString = floatStr(threshold); const lines = [ - title, + `# Performance Report${benchmarkText}`, '', - `Possible performance regression was detected for benchmark${benchmarkText}.`, - `Benchmark result of this commit is worse than the previous benchmark result exceeding threshold \`${thresholdString}\`.`, + `Benchmark result(s) exceed ratio of \`${thresholdString}\`.`, '', - `| Benchmark suite | Current: ${curSuite.commit.id} | Previous: ${prevSuite.commit.id} | Ratio |`, - '|-|-|-|-|', + `Previous commit: ${prevSuite.commit.id}`, + `Current commit: ${curSuite.commit.id}`, ]; - for (const alert of alerts) { - const { current, prev, ratio } = alert; - const line = `| \`${current.name}\` | ${strVal(current)} | ${strVal(prev)} | \`${floatStr(ratio)}\` |`; - lines.push(line); + if (losses.length > 0) { + lines.push(...['', '### :snail: The following benchmarks show regressions:']); + lines.push(...RESULT_TABLE_HEADER); + pushResultLines(losses, lines); + } + + if (gains.length > 0) { + lines.push(...['', '### :rocket: The following benchmarks show improvements:']); + lines.push(...RESULT_TABLE_HEADER); + pushResultLines(gains, lines); } // Footer @@ -276,14 +327,22 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be return; } - const alerts = findAlerts(curSuite, prevSuite, alertThreshold); + const [losses, gains] = findAlerts(curSuite, prevSuite, alertThreshold); + const alerts = [...losses, ...gains]; if (alerts.length === 0) { core.debug('No performance alert found happily'); return; } - core.debug(`Found ${alerts.length} alerts`); - const body = buildAlertComment(alerts, benchName, curSuite, prevSuite, alertThreshold, alertCommentCcUsers); + let body = ''; + if (alertThreshold === 0) { + core.debug(`Alert threshold is 0. Leaving report with ${alerts.length} alerts`); + body = buildReportComment(alerts, benchName, curSuite, prevSuite, alertCommentCcUsers); + } else { + core.debug(`Found ${alerts.length} alerts`); + body = buildAlertComment(losses, gains, benchName, curSuite, prevSuite, alertThreshold, alertCommentCcUsers); + } + let message = body; if (commentOnAlert) { @@ -297,9 +356,9 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be if (failOnAlert) { // Note: alertThreshold is smaller than failThreshold. It was checked in config.ts - const len = alerts.length; + const len = losses.length; const threshold = floatStr(failThreshold); - const failures = alerts.filter((a) => a.ratio > failThreshold); + const failures = losses.filter((a) => a.ratio > failThreshold); if (failures.length > 0) { core.debug('Mark this workflow as fail since one or more fatal alerts found'); if (failThreshold !== alertThreshold) { diff --git a/test/buildComment.test.ts b/test/buildComment.test.ts index ea9431c51..801b7a358 100644 --- a/test/buildComment.test.ts +++ b/test/buildComment.test.ts @@ -108,8 +108,11 @@ describe('buildComment', () => { # TestSuite
+ + Previous: testCommitIdPrevious + Current: testCommitIdCurrent - | Benchmark suite | Current: testCommitIdCurrent | Previous: testCommitIdPrevious | Ratio | + | Benchmark suite | Current | Previous | Ratio | |-|-|-|-| | \`TestBench<1>\` | \`0\` testUnit | \`0\` testUnit | \`1\` | | \`TestBench<2>\` | \`1\` testUnit | \`0\` testUnit | \`+∞\` | @@ -195,7 +198,10 @@ describe('buildComment', () => {
- | Benchmark suite | Current: testCommitIdCurrent | Previous: testCommitIdPrevious | Ratio | + Previous: testCommitIdPrevious + Current: testCommitIdCurrent + + | Benchmark suite | Current | Previous | Ratio | |-|-|-|-| | \`TestBench<1>\` | \`0\` testUnit | \`0\` testUnit | \`1\` | | \`TestBench<2>\` | \`0\` testUnit | \`1\` testUnit | \`+∞\` | diff --git a/test/write.spec.ts b/test/write.spec.ts index e88543cbb..8371526bf 100644 --- a/test/write.spec.ts +++ b/test/write.spec.ts @@ -332,12 +332,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe benches: [bench('bench_fib_10', 210), bench('bench_fib_20', 25000)], // Exceeds 2.0 threshold }, error: [ - '# :warning: **Performance Alert** :warning:', + "# Performance Report for **'Test benchmark'**", '', - "Possible performance regression was detected for benchmark **'Test benchmark'**.", - 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.', + 'Benchmark result(s) exceed ratio of `2`.', '', - '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + 'Previous commit: prev commit id', + 'Current commit: current commit id', + '', + '### :snail: The following benchmarks show regressions:', + '', + '| Benchmark suite | Current | Previous | Ratio |', '|-|-|-|-|', '| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |', '| `bench_fib_20` | `25000` ns/iter (`± 20`) | `10000` ns/iter (`± 20`) | `2.50` |', @@ -371,12 +375,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe benches: [bench('benchFib10', 20, '+-20', 'ops/sec')], // ops/sec so bigger is better }, error: [ - '# :warning: **Performance Alert** :warning:', + "# Performance Report for **'Test benchmark'**", + '', + 'Benchmark result(s) exceed ratio of `2`.', + '', + 'Previous commit: prev commit id', + 'Current commit: current commit id', '', - "Possible performance regression was detected for benchmark **'Test benchmark'**.", - 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.', + '### :snail: The following benchmarks show regressions:', '', - '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + '| Benchmark suite | Current | Previous | Ratio |', '|-|-|-|-|', '| `benchFib10` | `20` ops/sec (`+-20`) | `100` ops/sec (`+-20`) | `5` |', '', @@ -409,12 +417,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe benches: [bench('bench_fib_10', 210)], // Exceeds 2.0 threshold }, error: [ - '# :warning: **Performance Alert** :warning:', + '# Performance Report', + '', + 'Benchmark result(s) exceed ratio of `2`.', + '', + 'Previous commit: prev commit id', + 'Current commit: current commit id', '', - 'Possible performance regression was detected for benchmark.', - 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.', + '### :snail: The following benchmarks show regressions:', '', - '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + '| Benchmark suite | Current | Previous | Ratio |', '|-|-|-|-|', '| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |', '', @@ -447,12 +459,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe benches: [bench('bench_fib_10', 210)], // Exceeds 2.0 threshold }, error: [ - '# :warning: **Performance Alert** :warning:', + "# Performance Report for **'Test benchmark'**", '', - "Possible performance regression was detected for benchmark **'Test benchmark'**.", - 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.', + 'Benchmark result(s) exceed ratio of `2`.', '', - '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + 'Previous commit: prev commit id', + 'Current commit: current commit id', + '', + '### :snail: The following benchmarks show regressions:', + '', + '| Benchmark suite | Current | Previous | Ratio |', '|-|-|-|-|', '| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |', '', @@ -588,12 +604,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe // Though first item is truncated due to maxItemsInChart, alert still can be raised since previous data // is obtained before truncating an array of data items. error: [ - '# :warning: **Performance Alert** :warning:', + "# Performance Report for **'Test benchmark'**", + '', + 'Benchmark result(s) exceed ratio of `2`.', + '', + 'Previous commit: prev commit id', + 'Current commit: current commit id', '', - "Possible performance regression was detected for benchmark **'Test benchmark'**.", - 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.', + '### :snail: The following benchmarks show regressions:', '', - '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + '| Benchmark suite | Current | Previous | Ratio |', '|-|-|-|-|', '| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |', '| `bench_fib_20` | `25000` ns/iter (`± 20`) | `10000` ns/iter (`± 20`) | `2.50` |', @@ -603,6 +623,56 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe 'CC: @user', ], }, + { + it: 'adds improvements to table', + config: { ...defaultCfg, maxItemsInChart: 1 }, + data: { + lastUpdate, + repoUrl, + entries: { + 'Test benchmark': [ + { + commit: commit('prev commit id'), + date: lastUpdate - 1000, + tool: 'go', + benches: [bench('bench_fib_10', 100), bench('bench_fib_20', 100)], + }, + ], + }, + }, + added: { + commit: commit('current commit id'), + date: lastUpdate, + tool: 'go', + benches: [bench('bench_fib_10', 300), bench('bench_fib_20', 30)], // Exceeds 2.0 threshold + }, + // Though first item is truncated due to maxItemsInChart, alert still can be raised since previous data + // is obtained before truncating an array of data items. + error: [ + "# Performance Report for **'Test benchmark'**", + '', + 'Benchmark result(s) exceed ratio of `2`.', + '', + 'Previous commit: prev commit id', + 'Current commit: current commit id', + '', + '### :snail: The following benchmarks show regressions:', + '', + '| Benchmark suite | Current | Previous | Ratio |', + '|-|-|-|-|', + '| `bench_fib_10` | `300` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `3` |', + '', + '### :rocket: The following benchmarks show improvements:', + '', + '| Benchmark suite | Current | Previous | Ratio |', + '|-|-|-|-|', + '| `bench_fib_20` | `30` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `0.30` |', + '', + `This comment was automatically generated by [workflow](${serverUrl}/user/repo/actions?query=workflow%3AWorkflow%20name) using [github-action-benchmark](https://github.com/marketplace/actions/continuous-benchmark).`, + '', + 'CC: @user', + ], + }, { it: 'changes title when threshold is zero which means comment always happens', config: { ...defaultCfg, alertThreshold: 0, failThreshold: 0 }, @@ -629,10 +699,12 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe error: [ '# Performance Report', '', - "Possible performance regression was detected for benchmark **'Test benchmark'**.", - 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `0`.', + "For benchmark **'Test benchmark'**.", + '', + 'Previous commit: prev commit id', + 'Current commit: current commit id', '', - '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + '| Benchmark suite | Current | Previous | Ratio |', '|-|-|-|-|', '| `benchFib10` | `100` ops/sec (`+-20`) | `100` ops/sec (`+-20`) | `1` |', '', @@ -667,12 +739,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe error: [ '1 of 1 alerts exceeded the failure threshold `3` specified by fail-threshold input:', '', - '# :warning: **Performance Alert** :warning:', + "# Performance Report for **'Test benchmark'**", + '', + 'Benchmark result(s) exceed ratio of `2`.', '', - "Possible performance regression was detected for benchmark **'Test benchmark'**.", - 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.', + 'Previous commit: prev commit id', + 'Current commit: current commit id', '', - '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + '### :snail: The following benchmarks show regressions:', + '', + '| Benchmark suite | Current | Previous | Ratio |', '|-|-|-|-|', '| `bench_fib_10` | `350` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `3.50` |', '', @@ -792,7 +868,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe const h1 = query('h1'); expect(1).toEqual(h1.length); - expect(':warning: Performance Alert :warning:').toEqual(h1.text()); + expect("Performance Report for 'Test benchmark'").toEqual(h1.text()); const tr = query('tbody tr'); expect(t.added.benches.length).toEqual(tr.length); @@ -1155,12 +1231,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe gitServerUrl: serverUrl, gitHistory: gitHistory(), error: [ - '# :warning: **Performance Alert** :warning:', + "# Performance Report for **'Test benchmark'**", + '', + 'Benchmark result(s) exceed ratio of `2`.', + '', + 'Previous commit: prev commit id', + 'Current commit: current commit id', '', - "Possible performance regression was detected for benchmark **'Test benchmark'**.", - 'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.', + '### :snail: The following benchmarks show regressions:', '', - '| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |', + '| Benchmark suite | Current | Previous | Ratio |', '|-|-|-|-|', '| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |', '',