Skip to content

Commit 3500170

Browse files
414owen414owen
committed
Add performance improvements to alert comment
1 parent 5378675 commit 3500170

File tree

3 files changed

+153
-58
lines changed

3 files changed

+153
-58
lines changed

‎src/write.ts‎

Lines changed: 83 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,11 @@ interface Alert {
9494
ratio: number;
9595
}
9696

97-
function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number): Alert[] {
97+
function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number): [Alert[], Alert[]] {
9898
core.debug(`Comparing current:${curSuite.commit.id} and prev:${prevSuite.commit.id} for alert`);
9999

100-
const alerts = [];
100+
const losses = [];
101+
const gains = [];
101102
for (const current of curSuite.benches) {
102103
const prev = prevSuite.benches.find((b) => b.name === current.name);
103104
if (prev === undefined) {
@@ -107,16 +108,18 @@ function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number
107108

108109
const ratio = getRatio(curSuite.tool, prev, current);
109110

110-
if (ratio > threshold) {
111+
if (threshold === 0 || ratio > threshold) {
111112
core.warning(
112113
`Performance alert! Previous value was ${prev.value} and current value is ${current.value}.` +
113-
` It is ${ratio}x worse than previous exceeding a ratio threshold ${threshold}`,
114+
` It is ${ratio}x worse than previous, exceeding ratio threshold ${threshold}`,
114115
);
115-
alerts.push({ current, prev, ratio });
116+
losses.push({ current, prev, ratio });
117+
} else if (ratio < 1 / threshold) {
118+
gains.push({ current, prev, ratio });
116119
}
117120
}
118121

119-
return alerts;
122+
return [losses, gains];
120123
}
121124

122125
function getCurrentRepoMetadata() {
@@ -175,7 +178,10 @@ export function buildComment(
175178
'',
176179
expandableDetails ? '<details>' : '',
177180
'',
178-
`| Benchmark suite | Current: ${curSuite.commit.id} | Previous: ${prevSuite.commit.id} | Ratio |`,
181+
`Previous: ${prevSuite.commit.id}`,
182+
`Current: ${curSuite.commit.id}`,
183+
'',
184+
`| Benchmark suite | Current | Previous | Ratio |`,
179185
'|-|-|-|-|',
180186
];
181187

@@ -200,32 +206,77 @@ export function buildComment(
200206
return lines.join('\n');
201207
}
202208

209+
function pushResultLines(results: Alert[], output: string[]) {
210+
results.sort((a, b) => a.ratio - b.ratio);
211+
for (const alert of results) {
212+
const { current, prev, ratio } = alert;
213+
const line = `| \`${current.name}\` | ${strVal(current)} | ${strVal(prev)} | \`${floatStr(ratio)}\` |`;
214+
output.push(line);
215+
}
216+
}
217+
218+
const RESULT_TABLE_HEADER = ['', `| Benchmark suite | Current | Previous | Ratio |`, '|-|-|-|-|'];
219+
220+
function buildReportComment(
221+
results: Alert[],
222+
benchName: string,
223+
curSuite: Benchmark,
224+
prevSuite: Benchmark,
225+
cc: string[],
226+
): string {
227+
// Do not show benchmark name if it is the default value 'Benchmark'.
228+
const benchmarkText = benchName === 'Benchmark' ? '' : ` **'${benchName}'**`;
229+
const lines = [
230+
'# Performance Report',
231+
'',
232+
`For benchmark${benchmarkText}.`,
233+
'',
234+
`Previous commit: ${prevSuite.commit.id}`,
235+
`Current commit: ${curSuite.commit.id}`,
236+
];
237+
238+
lines.push(...RESULT_TABLE_HEADER);
239+
pushResultLines(results, lines);
240+
lines.push('', commentFooter());
241+
242+
if (cc.length > 0) {
243+
lines.push('', `CC: ${cc.join(' ')}`);
244+
}
245+
246+
return lines.join('\n');
247+
}
248+
203249
function buildAlertComment(
204-
alerts: Alert[],
250+
losses: Alert[],
251+
gains: Alert[],
205252
benchName: string,
206253
curSuite: Benchmark,
207254
prevSuite: Benchmark,
208255
threshold: number,
209256
cc: string[],
210257
): string {
211258
// Do not show benchmark name if it is the default value 'Benchmark'.
212-
const benchmarkText = benchName === 'Benchmark' ? '' : ` **'${benchName}'**`;
213-
const title = threshold === 0 ? '# Performance Report' : '# :warning: **Performance Alert** :warning:';
259+
const benchmarkText = benchName === 'Benchmark' ? '' : ` for **'${benchName}'**`;
214260
const thresholdString = floatStr(threshold);
215261
const lines = [
216-
title,
262+
`# Performance Report${benchmarkText}`,
217263
'',
218-
`Possible performance regression was detected for benchmark${benchmarkText}.`,
219-
`Benchmark result of this commit is worse than the previous benchmark result exceeding threshold \`${thresholdString}\`.`,
264+
`Benchmark result(s) exceed ratio of \`${thresholdString}\`.`,
220265
'',
221-
`| Benchmark suite | Current: ${curSuite.commit.id} | Previous: ${prevSuite.commit.id} | Ratio |`,
222-
'|-|-|-|-|',
266+
`Previous commit: ${prevSuite.commit.id}`,
267+
`Current commit: ${curSuite.commit.id}`,
223268
];
224269

225-
for (const alert of alerts) {
226-
const { current, prev, ratio } = alert;
227-
const line = `| \`${current.name}\` | ${strVal(current)} | ${strVal(prev)} | \`${floatStr(ratio)}\` |`;
228-
lines.push(line);
270+
if (gains.length > 0) {
271+
lines.push(...['', '### :rocket: The following benchmarks show improvements:']);
272+
lines.push(...RESULT_TABLE_HEADER);
273+
pushResultLines(gains, lines);
274+
}
275+
276+
if (losses.length > 0) {
277+
lines.push(...['', '### :snail: The following benchmarks show regressions:']);
278+
lines.push(...RESULT_TABLE_HEADER);
279+
pushResultLines(losses, lines);
229280
}
230281

231282
// Footer
@@ -276,14 +327,22 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be
276327
return;
277328
}
278329

279-
const alerts = findAlerts(curSuite, prevSuite, alertThreshold);
330+
const [losses, gains] = findAlerts(curSuite, prevSuite, alertThreshold);
331+
const alerts = [...losses, ...gains];
280332
if (alerts.length === 0) {
281333
core.debug('No performance alert found happily');
282334
return;
283335
}
284336

285-
core.debug(`Found ${alerts.length} alerts`);
286-
const body = buildAlertComment(alerts, benchName, curSuite, prevSuite, alertThreshold, alertCommentCcUsers);
337+
let body = '';
338+
if (alertThreshold === 0) {
339+
core.debug(`Alert threshold is 0. Leaving report with ${alerts.length} alerts`);
340+
body = buildReportComment(alerts, benchName, curSuite, prevSuite, alertCommentCcUsers);
341+
} else {
342+
core.debug(`Found ${alerts.length} alerts`);
343+
body = buildAlertComment(losses, gains, benchName, curSuite, prevSuite, alertThreshold, alertCommentCcUsers);
344+
}
345+
287346
let message = body;
288347

289348
if (commentOnAlert) {
@@ -297,9 +356,9 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be
297356

298357
if (failOnAlert) {
299358
// Note: alertThreshold is smaller than failThreshold. It was checked in config.ts
300-
const len = alerts.length;
359+
const len = losses.length;
301360
const threshold = floatStr(failThreshold);
302-
const failures = alerts.filter((a) => a.ratio > failThreshold);
361+
const failures = losses.filter((a) => a.ratio > failThreshold);
303362
if (failures.length > 0) {
304363
core.debug('Mark this workflow as fail since one or more fatal alerts found');
305364
if (failThreshold !== alertThreshold) {

‎test/buildComment.test.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,11 @@ describe('buildComment', () => {
108108
# TestSuite
109109
110110
<details>
111+
112+
Previous: testCommitIdPrevious
113+
Current: testCommitIdCurrent
111114
112-
| Benchmark suite | Current: testCommitIdCurrent | Previous: testCommitIdPrevious | Ratio |
115+
| Benchmark suite | Current | Previous | Ratio |
113116
|-|-|-|-|
114117
| \`TestBench<1>\` | \`0\` testUnit | \`0\` testUnit | \`1\` |
115118
| \`TestBench<2>\` | \`1\` testUnit | \`0\` testUnit | \`+∞\` |
@@ -195,7 +198,10 @@ describe('buildComment', () => {
195198
196199
<details>
197200
198-
| Benchmark suite | Current: testCommitIdCurrent | Previous: testCommitIdPrevious | Ratio |
201+
Previous: testCommitIdPrevious
202+
Current: testCommitIdCurrent
203+
204+
| Benchmark suite | Current | Previous | Ratio |
199205
|-|-|-|-|
200206
| \`TestBench<1>\` | \`0\` testUnit | \`0\` testUnit | \`1\` |
201207
| \`TestBench<2>\` | \`0\` testUnit | \`1\` testUnit | \`+∞\` |

‎test/write.spec.ts‎

Lines changed: 62 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -332,12 +332,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe
332332
benches: [bench('bench_fib_10', 210), bench('bench_fib_20', 25000)], // Exceeds 2.0 threshold
333333
},
334334
error: [
335-
'# :warning: **Performance Alert** :warning:',
335+
"# Performance Report for **'Test benchmark'**",
336336
'',
337-
"Possible performance regression was detected for benchmark **'Test benchmark'**.",
338-
'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.',
337+
'Benchmark result(s) exceed ratio of `2`.',
339338
'',
340-
'| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |',
339+
'Previous commit: prev commit id',
340+
'Current commit: current commit id',
341+
'',
342+
'### :snail: The following benchmarks show regressions:',
343+
'',
344+
'| Benchmark suite | Current | Previous | Ratio |',
341345
'|-|-|-|-|',
342346
'| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |',
343347
'| `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
371375
benches: [bench('benchFib10', 20, '+-20', 'ops/sec')], // ops/sec so bigger is better
372376
},
373377
error: [
374-
'# :warning: **Performance Alert** :warning:',
378+
"# Performance Report for **'Test benchmark'**",
379+
'',
380+
'Benchmark result(s) exceed ratio of `2`.',
381+
'',
382+
'Previous commit: prev commit id',
383+
'Current commit: current commit id',
375384
'',
376-
"Possible performance regression was detected for benchmark **'Test benchmark'**.",
377-
'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.',
385+
'### :snail: The following benchmarks show regressions:',
378386
'',
379-
'| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |',
387+
'| Benchmark suite | Current | Previous | Ratio |',
380388
'|-|-|-|-|',
381389
'| `benchFib10` | `20` ops/sec (`+-20`) | `100` ops/sec (`+-20`) | `5` |',
382390
'',
@@ -409,12 +417,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe
409417
benches: [bench('bench_fib_10', 210)], // Exceeds 2.0 threshold
410418
},
411419
error: [
412-
'# :warning: **Performance Alert** :warning:',
420+
"# Performance Report",
413421
'',
414-
'Possible performance regression was detected for benchmark.',
415-
'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.',
422+
'Benchmark result(s) exceed ratio of `2`.',
416423
'',
417-
'| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |',
424+
'Previous commit: prev commit id',
425+
'Current commit: current commit id',
426+
'',
427+
'### :snail: The following benchmarks show regressions:',
428+
'',
429+
'| Benchmark suite | Current | Previous | Ratio |',
418430
'|-|-|-|-|',
419431
'| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |',
420432
'',
@@ -447,12 +459,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe
447459
benches: [bench('bench_fib_10', 210)], // Exceeds 2.0 threshold
448460
},
449461
error: [
450-
'# :warning: **Performance Alert** :warning:',
462+
"# Performance Report for **'Test benchmark'**",
463+
'',
464+
'Benchmark result(s) exceed ratio of `2`.',
465+
'',
466+
'Previous commit: prev commit id',
467+
'Current commit: current commit id',
451468
'',
452-
"Possible performance regression was detected for benchmark **'Test benchmark'**.",
453-
'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.',
469+
'### :snail: The following benchmarks show regressions:',
454470
'',
455-
'| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |',
471+
'| Benchmark suite | Current | Previous | Ratio |',
456472
'|-|-|-|-|',
457473
'| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |',
458474
'',
@@ -588,12 +604,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe
588604
// Though first item is truncated due to maxItemsInChart, alert still can be raised since previous data
589605
// is obtained before truncating an array of data items.
590606
error: [
591-
'# :warning: **Performance Alert** :warning:',
607+
"# Performance Report for **'Test benchmark'**",
592608
'',
593-
"Possible performance regression was detected for benchmark **'Test benchmark'**.",
594-
'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.',
609+
'Benchmark result(s) exceed ratio of `2`.',
595610
'',
596-
'| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |',
611+
'Previous commit: prev commit id',
612+
'Current commit: current commit id',
613+
'',
614+
'### :snail: The following benchmarks show regressions:',
615+
'',
616+
'| Benchmark suite | Current | Previous | Ratio |',
597617
'|-|-|-|-|',
598618
'| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |',
599619
'| `bench_fib_20` | `25000` ns/iter (`± 20`) | `10000` ns/iter (`± 20`) | `2.50` |',
@@ -629,10 +649,12 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe
629649
error: [
630650
'# Performance Report',
631651
'',
632-
"Possible performance regression was detected for benchmark **'Test benchmark'**.",
633-
'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `0`.',
652+
"For benchmark **'Test benchmark'**.",
634653
'',
635-
'| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |',
654+
'Previous commit: prev commit id',
655+
'Current commit: current commit id',
656+
'',
657+
'| Benchmark suite | Current | Previous | Ratio |',
636658
'|-|-|-|-|',
637659
'| `benchFib10` | `100` ops/sec (`+-20`) | `100` ops/sec (`+-20`) | `1` |',
638660
'',
@@ -667,12 +689,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe
667689
error: [
668690
'1 of 1 alerts exceeded the failure threshold `3` specified by fail-threshold input:',
669691
'',
670-
'# :warning: **Performance Alert** :warning:',
692+
"# Performance Report for **'Test benchmark'**",
693+
'',
694+
'Benchmark result(s) exceed ratio of `2`.',
671695
'',
672-
"Possible performance regression was detected for benchmark **'Test benchmark'**.",
673-
'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.',
696+
'Previous commit: prev commit id',
697+
'Current commit: current commit id',
674698
'',
675-
'| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |',
699+
'### :snail: The following benchmarks show regressions:',
700+
'',
701+
'| Benchmark suite | Current | Previous | Ratio |',
676702
'|-|-|-|-|',
677703
'| `bench_fib_10` | `350` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `3.50` |',
678704
'',
@@ -792,7 +818,7 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe
792818

793819
const h1 = query('h1');
794820
expect(1).toEqual(h1.length);
795-
expect(':warning: Performance Alert :warning:').toEqual(h1.text());
821+
expect("Performance Report for 'Test benchmark'").toEqual(h1.text());
796822

797823
const tr = query('tbody tr');
798824
expect(t.added.benches.length).toEqual(tr.length);
@@ -1155,12 +1181,16 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe
11551181
gitServerUrl: serverUrl,
11561182
gitHistory: gitHistory(),
11571183
error: [
1158-
'# :warning: **Performance Alert** :warning:',
1184+
"# Performance Report for **'Test benchmark'**",
1185+
'',
1186+
'Benchmark result(s) exceed ratio of `2`.',
1187+
'',
1188+
'Previous commit: prev commit id',
1189+
'Current commit: current commit id',
11591190
'',
1160-
"Possible performance regression was detected for benchmark **'Test benchmark'**.",
1161-
'Benchmark result of this commit is worse than the previous benchmark result exceeding threshold `2`.',
1191+
'### :snail: The following benchmarks show regressions:',
11621192
'',
1163-
'| Benchmark suite | Current: current commit id | Previous: prev commit id | Ratio |',
1193+
'| Benchmark suite | Current | Previous | Ratio |',
11641194
'|-|-|-|-|',
11651195
'| `bench_fib_10` | `210` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `2.10` |',
11661196
'',

0 commit comments

Comments
 (0)