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

Replace fail-on-alert with comment-on-alert on benchmark test #5671

Closed

Conversation

XSAM
Copy link
Member

@XSAM XSAM commented Aug 5, 2024

Part of #4537

The fail-on-alert would fail the entire benchmark CI like this https://github.com/open-telemetry/opentelemetry-go/actions/runs/10235553562/job/28367685489. With this behavior, we are not able to cache the benchmark result, thus the next benchmark cannot find the previous result to compare. https://github.com/open-telemetry/opentelemetry-go/actions/runs/10250450573

This PR replaces fail-on-alert with comment-on-alert, so the benchmark of the next commit can always find the result from the previous commit even if the previous CI triggers an alert. And, we can still receive the alert by comment, like this benchmark-action/github-action-benchmark@077dde1#commitcomment-36047186.

@XSAM XSAM added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 5, 2024
@XSAM XSAM mentioned this pull request Aug 5, 2024
4 tasks
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.4%. Comparing base (127d068) to head (5248400).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5671   +/-   ##
=====================================
  Coverage   84.4%   84.4%           
=====================================
  Files        272     272           
  Lines      22516   22516           
=====================================
+ Hits       19024   19025    +1     
+ Misses      3156    3155    -1     
  Partials     336     336           

see 1 file with indirect coverage changes

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I think that we should use actions/cache/save and actions/cache/restore actions instead of a single actions/cache action.

This way we can still have the benchmark action failed and the cache being updated (we can run actions/cache/save even if the workflow is failed). Otherwise, if we do not fail the workflow, how we would know that something is wrong?

References:

@XSAM
Copy link
Member Author

XSAM commented Aug 7, 2024

Otherwise, if we do not fail the workflow, how we would know that something is wrong?

The comment-on-alert should also send a notification to the author of the commit. But, yeah, a failed CI is more clear than a notification. I created #5694 to replace this PR.

@XSAM XSAM closed this Aug 7, 2024
XSAM added a commit that referenced this pull request Aug 9, 2024
Part of #4537,
replace #5671

I tried to use `save-always` to simplify it, but this flag never worked.
actions/cache#1315.

So, I split the cache action into two, one for restore and one for save.
The save step would always run even if a step failed.

Some actions I run to prove it could work:
- On success:
https://github.com/XSAM/opentelemetry-go/actions/runs/10292883964/job/28488154161
- On failure:
https://github.com/XSAM/opentelemetry-go/actions/runs/10292907887/job/28488227777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants