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.

Sign up for GitHub

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

Support negative threshold for bigger-is-better in other tools #149

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

Support negative threshold for bigger-is-better in other tools #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,9 @@ At bottom of the page, the download button is available for downloading benchmar
### Alert comment on commit page

This action can raise [an alert comment][alert-comment-example]. to the commit when its benchmark
results are worse than previous exceeding a specified threshold.
results are worse than previous exceeding a specified threshold. If negative the threshold will alert
if the benchmark result decreases over the specified threshold, and if positive it will alert if the
benchmark result increase over the specified threshold.

![alert comment](https://raw.githubusercontent.com/rhysd/ss/master/github-action-benchmark/alert-comment.png)

Expand Down Expand Up @@ -184,8 +186,10 @@ The step which runs `github-action-benchmark` does followings:
the workflow fails and the failure is notified to you

By default, this action marks the result as performance regression when it is worse than the previous
exceeding 200% threshold. For example, if the previous benchmark result was 100 iter/ns and this time
it is 230 iter/ns, it means 230% worse than the previous and an alert will happen. The threshold can
exceeding 200% threshold. For example, if the previous benchmark result was 100 ns and this time
it is 230 ns, it means 230% worse than the previous and an alert will happen. If the threshold specified is negative it
will alert when the benchmark result decreases, so if the previous benchmark result was 230 ops/ns and this time
it is 100 ops/ns, if the threshold is set to -200% then an alert will happen. The threshold can
be changed by `alert-threshold` input.

A live workflow example is [here](.github/workflows/minimal.yml). And the results of the workflow can
Expand Down
2 changes: 1 addition & 1 deletion src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ function validateAlertThreshold(alertThreshold: number | null, failThreshold: nu
if (alertThreshold === null) {
throw new Error("'alert-threshold' input must not be empty");
}
if (failThreshold && alertThreshold > failThreshold) {
if (failThreshold && Math.abs(alertThreshold) > Math.abs(failThreshold)) {
Copy link
Member

@ningziwen ningziwen May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change as it adds another scenario that can alert?

The breaking change may be prevented if we add an additional configrable field to toggle this. (as you stated in PR description)

WDYT? How strict are we treating breaking change? @ktrz

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super fond of the current solution. I would much rather modify the biggerIsBetter function to determine the value based on the units as well.

throw new Error(
`'alert-threshold' value must be smaller than 'fail-threshold' value but got ${alertThreshold} > ${failThreshold}`,
);
Expand Down
16 changes: 9 additions & 7 deletions src/write.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,14 @@ function findAlerts(curSuite: Benchmark, prevSuite: Benchmark, threshold: number
continue;
}

const ratio = biggerIsBetter(curSuite.tool)
const ratio = biggerIsBetter(curSuite.tool) || threshold < 0
? prev.value / current.value // e.g. current=100, prev=200
: current.value / prev.value; // e.g. current=200, prev=100

if (ratio > threshold) {
if (ratio > Math.abs(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 a ratio threshold ${Math.abs(threshold)}`,
);
alerts.push({ current, prev, ratio });
}
Expand Down Expand Up @@ -272,6 +272,8 @@ async function handleComment(benchName: string, curSuite: Benchmark, prevSuite:

async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Benchmark, config: Config) {
const { alertThreshold, githubToken, commentOnAlert, failOnAlert, alertCommentCcUsers, failThreshold } = config;
const absAlertThreshold = Math.abs(alertThreshold);
const absFailThreshold = Math.abs(failThreshold);

if (!commentOnAlert && !failOnAlert) {
core.debug('Alert check was skipped because both comment-on-alert and fail-on-alert were disabled');
Expand All @@ -285,7 +287,7 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be
}

core.debug(`Found ${alerts.length} alerts`);
const body = buildAlertComment(alerts, benchName, curSuite, prevSuite, alertThreshold, alertCommentCcUsers);
const body = buildAlertComment(alerts, benchName, curSuite, prevSuite, absAlertThreshold, alertCommentCcUsers);
let message = body;
let url = null;

Expand All @@ -301,8 +303,8 @@ 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 threshold = floatStr(failThreshold);
const failures = alerts.filter((a) => a.ratio > failThreshold);
const threshold = floatStr(absFailThreshold);
const failures = alerts.filter((a) => a.ratio > absFailThreshold);
if (failures.length > 0) {
core.debug('Mark this workflow as fail since one or more fatal alerts found');
if (failThreshold !== alertThreshold) {
Expand All @@ -312,7 +314,7 @@ async function handleAlert(benchName: string, curSuite: Benchmark, prevSuite: Be
throw new Error(message);
} else {
core.debug(
`${len} alerts exceeding the alert threshold ${alertThreshold} were found but` +
`${len} alerts exceeding the alert threshold ${absAlertThreshold} were found but` +
` all of them did not exceed the failure threshold ${threshold}`,
);
}
Expand Down
38 changes: 38 additions & 0 deletions test/write.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,44 @@ describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBe
'CC: @user',
],
},
{
it: 'raises an alert when exceeding threshold negative 2.0',
config: { ...defaultCfg, alertThreshold: -2.0, failThreshold: -2.0 },
data: {
lastUpdate,
repoUrl,
entries: {
'Test benchmark': [
{
commit: commit('prev commit id'),
date: lastUpdate - 1000,
tool: 'jmh',
benches: [bench('bench_fib_10', 100), bench('bench_fib_20', 10000)],
},
],
},
},
added: {
commit: commit('current commit id'),
date: lastUpdate,
tool: 'go',
benches: [bench('bench_fib_10', 20), bench('bench_fib_20', 25000)], // Exceeds -2.0 threshold
},
error: [
'# :warning: **Performance Alert** :warning:',
'',
"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 suite | Current: current commit id | Previous: prev commit id | Ratio |',
'|-|-|-|-|',
'| `bench_fib_10` | `20` ns/iter (`± 20`) | `100` ns/iter (`± 20`) | `5` |',
'',
`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: 'raises an alert with tool whose result value is bigger-is-better',
config: defaultCfg,
Expand Down