Skip to content

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

Merged
amannocci merged 33 commits intomainfrom
feature/microbenchmark
Jul 6, 2023
Merged

action: support for benchmark using GitHub actions in conjunction with Buildkite#3374
amannocci merged 33 commits intomainfrom
feature/microbenchmark

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

@v1v v1v May 25, 2023

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

@trentm trentm May 26, 2023

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

@amannocci amannocci Jul 3, 2023

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
v1v
v1v commented May 26, 2023
View reviewed changes
v1v
v1v commented May 26, 2023
View reviewed changes
v1v
v1v commented May 26, 2023
View reviewed changes
v1v
v1v commented May 26, 2023
View reviewed changes
v1v
v1v commented May 26, 2023
View reviewed changes
.github/workflows/microbenchmark.yml Outdated
push:
branches:
- main
- feature/microbenchmark
Copy link
Member Author

@v1v v1v May 26, 2023

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

@trentm trentm Jul 5, 2023

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

@amannocci amannocci Jul 6, 2023

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.

.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

@trentm trentm May 26, 2023

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.

@trentm trentm mentioned this pull request May 26, 2023
Open
v1v
v1v commented Jun 2, 2023
View reviewed changes
v1v
v1v commented Jun 30, 2023
View reviewed changes
@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

@trentm trentm Jul 5, 2023

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 25 commits July 6, 2023 11:29
@v1v @amannocci
move the traces earlier
0844dd3
@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 <trent.mick@elastic.co>
@amannocci
ci: small format cleanup
581fbbb 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci
ci: let's debug nvm install
96e60cf 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci
ci: correct benchmark prepare step
419e217 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci
ci: flush positional arguments and use major version
b0804c1 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci
ci: properly set NODE_VERSION env var
a4535eb 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci
ci: use nvmrc instead of explicit value
f0c511a 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci
ci: small cleanup
76c86e3 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci
ci: last cleanup before merge
50f952e 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci amannocci force-pushed the feature/microbenchmark branch from 94fff4f to 50f952e Compare July 6, 2023 09:30
@amannocci
ci: remove leftover jenkins job scheduling
edcee09 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
@amannocci amannocci merged commit 230740e into main Jul 6, 2023
@amannocci amannocci deleted the feature/microbenchmark branch July 6, 2023 11:13
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
@v1v @trentm
@amannocci @PeterEinberger
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 <trent.mick@elastic.co>

* ci: small format cleanup

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>

* ci: let's debug nvm install

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>

* ci: correct benchmark prepare step

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>

* ci: flush positional arguments and use major version

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>

* ci: properly set NODE_VERSION env var

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>

* ci: use nvmrc instead of explicit value

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>

* ci: small cleanup

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>

* ci: last cleanup before merge

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>

* ci: remove leftover jenkins job scheduling

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>

---------

Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Co-authored-by: Trent Mick <trent.mick@elastic.co>
Co-authored-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trentm trentm trentm approved these changes

+2 more reviewers

@amannocci amannocci amannocci left review comments

@reakaleek reakaleek reakaleek approved these changes

Reviewers whose approvals may not affect merge requirements

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