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

Jump to bottom

action: support for benchmark using GitHub actions in conjunction with Buildkite #3374

Merged
merged 33 commits into from
Jul 6, 2023
Merged

action: support for benchmark using GitHub actions in conjunction with Buildkite #3374

merged 33 commits into from
Jul 6, 2023

Conversation

v1v
Copy link
Member

@v1v v1v commented May 24, 2023 •
edited
Loading

What

Migrate the very last thing so we can delete the pipeline in Jenkins.

It uses Buildkite since the existing microbenchmark runners are attached to Buildkite. Though the orchestrator is GitHub actions, hence the buildkite run is reported.

It is only supported for main if needed for PRs, it will be done in a follow up - we don't wanna run by default on PRs so we can avoid running code from external contributors.

Checklist

  • Implement code
  • Add tests
  • Update TypeScript typings
  • Update documentation
  • Add CHANGELOG.asciidoc entry
  • Commit message follows commit guidelines

Test

See https://github.com/elastic/apm-agent-nodejs/actions/runs/5089734599/jobs/9147721605 that got triggered the microbenchmarks.

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label May 24, 2023
v1v
v1v commented May 25, 2023
View reviewed changes
.ci/scripts/prepare-benchmarks-env.sh Outdated
# nvm_resolve_local_alias default returns VERSION= while in Jenkins a similar
# runner returns VERSION=v14.21.3
# Already reported to the System owners of the Buildkite agent setup.
\. "$NVM_DIR/nvm.sh" || true
Copy link
Member Author

Choose a reason for hiding this comment

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

I've no clue what's the reason for the failure... I also asked to the community in nvm-sh/nvm#3117

It seems to work later on if using || true

Copy link
Member

Choose a reason for hiding this comment

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

# Let's require that the given node version is a full `X.Y.Z` version.
node_version=16.20.0

# Then the "install node" process is just this:
curl -sS https://nodejs.org/dist/v${node_version}/node-v${node_version}-linux-x64.tar.gz -o node.tar.gz
tar xf node.tar.gz
export PATH=`pwd`/node-v${node_version}-linux-x64/bin:$PATH

Perhaps just error out if we are not running on linux-x64.

Copy link
Contributor

Choose a reason for hiding this comment

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

The node version used is the one present in .nvmrc.

v1v
v1v commented May 26, 2023
View reviewed changes
.ci/scripts/bench.sh Outdated Show resolved Hide resolved
v1v
v1v commented May 26, 2023
View reviewed changes
.ci/scripts/bench.sh Outdated Show resolved Hide resolved
v1v
v1v commented May 26, 2023
View reviewed changes
.ci/scripts/bench.sh Outdated Show resolved Hide resolved
v1v
v1v commented May 26, 2023
View reviewed changes
.ci/scripts/bench.sh Outdated Show resolved Hide resolved
v1v
v1v commented May 26, 2023
View reviewed changes
.ci/scripts/bench.sh Outdated Show resolved Hide resolved
v1v
v1v commented May 26, 2023
View reviewed changes
.github/workflows/microbenchmark.yml Outdated
push:
branches:
- main
- feature/microbenchmark
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
- feature/microbenchmark

Copy link
Member

Choose a reason for hiding this comment

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

This will get removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed it

@v1v v1v self-assigned this May 26, 2023
@v1v v1v marked this pull request as ready for review May 26, 2023 10:41
@v1v v1v requested review from a team May 26, 2023 10:41
@trentm
Copy link
Member

trentm commented May 26, 2023

It is only supported for main if needed for PRs, it will be done in a follow up

Just for main is fine.

trentm
trentm requested changes May 26, 2023
View reviewed changes
Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

  • Can the .groovy files in ".ci/" be removed as well?

A number of comments below.

.github/workflows/microbenchmark.yml Outdated Show resolved Hide resolved
test/benchmarks/scripts/run-benchmarks-ci.sh Outdated Show resolved Hide resolved
test/benchmarks/scripts/run-benchmarks-ci.sh Show resolved Hide resolved
test/benchmarks/scripts/run-benchmarks-ci.sh Outdated Show resolved Hide resolved
.ci/scripts/bench.sh Outdated Show resolved Hide resolved
.ci/scripts/bench.sh Outdated Show resolved Hide resolved
.ci/scripts/bench.sh Outdated Show resolved Hide resolved
.ci/scripts/bench.sh Outdated Show resolved Hide resolved
.ci/scripts/prepare-benchmarks-env.sh Outdated
# nvm_resolve_local_alias default returns VERSION= while in Jenkins a similar
# runner returns VERSION=v14.21.3
# Already reported to the System owners of the Buildkite agent setup.
\. "$NVM_DIR/nvm.sh" || true
Copy link
Member

Choose a reason for hiding this comment

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

# Let's require that the given node version is a full `X.Y.Z` version.
node_version=16.20.0

# Then the "install node" process is just this:
curl -sS https://nodejs.org/dist/v${node_version}/node-v${node_version}-linux-x64.tar.gz -o node.tar.gz
tar xf node.tar.gz
export PATH=`pwd`/node-v${node_version}-linux-x64/bin:$PATH

Perhaps just error out if we are not running on linux-x64.

.github/workflows/microbenchmark.yml Show resolved Hide resolved
@trentm trentm mentioned this pull request May 26, 2023
Open
@amannocci amannocci assigned amannocci and unassigned v1v Jul 3, 2023
@amannocci amannocci requested review from amannocci and trentm and removed request for amannocci July 3, 2023 09:40
trentm
trentm approved these changes Jul 5, 2023
View reviewed changes
Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

A couple small things, otherwise LGTM. Thanks.

Can these be removed?

.ci/schedule-daily.groovy
.ci/schedule-weekly.groovy

.github/workflows/microbenchmark.yml Outdated
push:
branches:
- main
- feature/microbenchmark
Copy link
Member

Choose a reason for hiding this comment

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

This will get removed?

v1v and others added 24 commits July 6, 2023 11:29
@v1v @amannocci
debug what's going on
9981263
@v1v @amannocci
use the installation steps explained in https://github.com/nvm-sh/nvm
333a15d
@v1v @amannocci
use install manually as explained in https://github.com/nvm-sh/nvm\#m…
2a12181 
…anual-install
@v1v @amannocci
Revert "use install manually as explained in https://github.com/nvm-s…
004273d 
…h/nvm\#manual-install"

This reverts commit 0af0553.
@v1v @amannocci
install only if it is not installed
73f6c34
@v1v @amannocci
debug whether shell login
c1a471f
@v1v @amannocci
avoid errors when calling the nvm for the first time
f4acdc8
@v1v @amannocci
support buildkite missbehaviour
5386243
@v1v @amannocci
remove debug
daed361
@v1v @amannocci
revert changes to be similar to the previous configuration
ac1e989
@v1v @amannocci
debug
675b986
@v1v @amannocci
reduce log traces
63f3c2e
@v1v @amannocci
Update .ci/scripts/bench.sh
63b56d7
@v1v @amannocci
Apply suggestions from code review
be6421a
@v1v @amannocci
Apply suggestions from code review
6e45e21
@v1v @trentm @amannocci
Apply suggestions from code review
056e2c5 
Co-authored-by: Trent Mick <[email protected]>
@amannocci
ci: small format cleanup
581fbbb 
Signed-off-by: Adrien Mannocci <[email protected]>
@amannocci
ci: let's debug nvm install
96e60cf 
Signed-off-by: Adrien Mannocci <[email protected]>
@amannocci
ci: correct benchmark prepare step
419e217 
Signed-off-by: Adrien Mannocci <[email protected]>
@amannocci
ci: flush positional arguments and use major version
b0804c1 
Signed-off-by: Adrien Mannocci <[email protected]>
@amannocci
ci: properly set NODE_VERSION env var
a4535eb 
Signed-off-by: Adrien Mannocci <[email protected]>
@amannocci
ci: use nvmrc instead of explicit value
f0c511a 
Signed-off-by: Adrien Mannocci <[email protected]>
@amannocci
ci: small cleanup
76c86e3 
Signed-off-by: Adrien Mannocci <[email protected]>
@amannocci
ci: last cleanup before merge
50f952e 
Signed-off-by: Adrien Mannocci <[email protected]>
@amannocci amannocci merged commit 230740e into main Jul 6, 2023
@amannocci amannocci deleted the feature/microbenchmark branch July 6, 2023 11:13
fpm-peter pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
@v1v @trentm
@amannocci @fpm-peter
action: support for benchmark using GitHub actions in conjunction wit…
a6f273f 
…h Buildkite (elastic#3374)

* ci: remove Jenkins

* support for benchmark using GitHub actions in conjunction with Buildkite

* chore: for testing purposes

* remove any Jenkins references

* add buildkite log echo for helping with collapsing the output in the UI

 this will not show make the UI better in GitHub actions though

* action: group logs

* Revert "action: group logs"

This reverts commit 92bcd6c.

* move the traces earlier

* debug what's going on

* use the installation steps explained in https://github.com/nvm-sh/nvm

* use install manually as explained in https://github.com/nvm-sh/nvm\#manual-install

* Revert "use install manually as explained in https://github.com/nvm-sh/nvm\#manual-install"

This reverts commit 0af0553.

* install only if it is not installed

* debug whether shell login

* avoid errors when calling the nvm for the first time

* support buildkite missbehaviour

* remove debug

* revert changes to be similar to the previous configuration

* debug

* reduce log traces

* Update .ci/scripts/bench.sh

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Trent Mick <[email protected]>

* ci: small format cleanup

Signed-off-by: Adrien Mannocci <[email protected]>

* ci: let's debug nvm install

Signed-off-by: Adrien Mannocci <[email protected]>

* ci: correct benchmark prepare step

Signed-off-by: Adrien Mannocci <[email protected]>

* ci: flush positional arguments and use major version

Signed-off-by: Adrien Mannocci <[email protected]>

* ci: properly set NODE_VERSION env var

Signed-off-by: Adrien Mannocci <[email protected]>

* ci: use nvmrc instead of explicit value

Signed-off-by: Adrien Mannocci <[email protected]>

* ci: small cleanup

Signed-off-by: Adrien Mannocci <[email protected]>

* ci: last cleanup before merge

Signed-off-by: Adrien Mannocci <[email protected]>

* ci: remove leftover jenkins job scheduling

Signed-off-by: Adrien Mannocci <[email protected]>

---------

Signed-off-by: Adrien Mannocci <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: Adrien Mannocci <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amannocci amannocci amannocci left review comments

@reakaleek reakaleek reakaleek approved these changes

@trentm trentm trentm approved these changes

Assignees

@amannocci amannocci

Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@v1v @trentm @amannocci @reakaleek