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.

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

Track historic best results and (optionally) compute ratio based on best results #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ksvladimir
Copy link

Thanks for providing this awesome action that our team greatly enjoys!

Our team got concerned that, since this action shows alerts based on comparison to the previous run only, it wouldn't catch gradual performance degradation over several commits. Such degradation is very real, especially when using this action for end-to-end benchmarks: the code becomes a bit slower (but below the alert threshold) with each new feature added, and eventually, it reaches the point where we want to revisit its performance — ideally as advised by an alert from this action.

This PR makes this action usable for detecting performance degradation over time in end-to-end benchmarks by:

  • showing Best value for each benchmark in commit comments (in addition to previous and current values)
  • having an option to use the best benchmark instead of the previous one for computing ratios and triggering alerts

We'd appreciate your consideration for including this PR in this action!

@ningziwen
Copy link
Member

If it reaches a super good value by transient issue for one time, will be "Best" be corrupted and it will consistently alert until you fix the data point manually?

Alternatively, what do you think of setting an absolute threshold for your use case? Or thinking about a better stats value instead of "Best", like "Median during recent period"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants