Skip to content

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

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

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

Conversation

@v1v

@v1v v1v commented May 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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
Comment thread .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

@v1v v1v May 25, 2023

Copy link
Copy Markdown
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

@trentm trentm May 26, 2023

Copy link
Copy Markdown
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.

@amannocci amannocci Jul 3, 2023

Copy link
Copy Markdown

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
Comment thread .ci/scripts/bench.sh Outdated
v1v
v1v commented May 26, 2023
View reviewed changes
Comment thread .ci/scripts/bench.sh Outdated
v1v
v1v commented May 26, 2023
View reviewed changes
Comment thread .ci/scripts/bench.sh Outdated
v1v
v1v commented May 26, 2023
View reviewed changes
Comment thread .ci/scripts/bench.sh Outdated
v1v
v1v commented May 26, 2023
View reviewed changes
Comment thread .ci/scripts/bench.sh Outdated
v1v
v1v commented May 26, 2023
View reviewed changes
Comment thread .github/workflows/microbenchmark.yml Outdated
push:
branches:
- main
- feature/microbenchmark

@v1v v1v May 26, 2023

Copy link
Copy Markdown
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

@trentm trentm Jul 5, 2023

Copy link
Copy Markdown
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?

@amannocci amannocci Jul 6, 2023

Copy link
Copy Markdown

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

trentm commented May 26, 2023

Copy link
Copy Markdown
Member

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

@trentm trentm left a comment

Copy link
Copy Markdown
Member

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.

Comment thread .github/workflows/microbenchmark.yml Outdated
Comment thread test/benchmarks/scripts/run-benchmarks-ci.sh Outdated
Comment thread test/benchmarks/scripts/run-benchmarks-ci.sh
Comment thread test/benchmarks/scripts/run-benchmarks-ci.sh Outdated
Comment thread .ci/scripts/bench.sh Outdated
Comment thread .ci/scripts/bench.sh Outdated
Comment thread .ci/scripts/bench.sh Outdated
Comment thread .ci/scripts/bench.sh Outdated
Comment thread .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

@trentm trentm May 26, 2023

Copy link
Copy Markdown
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.

Comment thread .github/workflows/microbenchmark.yml
@trentm trentm mentioned this pull request May 26, 2023
Open
v1v
v1v commented Jun 2, 2023
View reviewed changes
Comment thread .github/workflows/microbenchmark.yml
v1v
v1v commented Jun 30, 2023
View reviewed changes
Comment thread .ci/scripts/prepare-benchmarks-env.sh
@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

@trentm trentm left a comment

Copy link
Copy Markdown
Member

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

Comment thread .github/workflows/microbenchmark.yml Outdated
push:
branches:
- main
- feature/microbenchmark

@trentm trentm Jul 5, 2023

Copy link
Copy Markdown
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 25 commits July 6, 2023 11:29
@v1v Adrien Mannocci
move the traces earlier
0844dd3
@v1v Adrien Mannocci
debug what's going on
9981263
@v1v Adrien Mannocci
use the installation steps explained in https://github.com/nvm-sh/nvm
333a15d
@v1v Adrien Mannocci
use install manually as explained in https://github.com/nvm-sh/nvm\#m…
2a12181 
…anual-install
@v1v Adrien Mannocci
Revert "use install manually as explained in https://github.com/nvm-s…
004273d 
…h/nvm\#manual-install"

This reverts commit 0af0553.
@v1v Adrien Mannocci
install only if it is not installed
73f6c34
@v1v Adrien Mannocci
debug whether shell login
c1a471f
@v1v Adrien Mannocci
avoid errors when calling the nvm for the first time
f4acdc8
@v1v Adrien Mannocci
support buildkite missbehaviour
5386243
@v1v Adrien Mannocci
remove debug
daed361
@v1v Adrien Mannocci
revert changes to be similar to the previous configuration
ac1e989
@v1v Adrien Mannocci
debug
675b986
@v1v Adrien Mannocci
reduce log traces
63f3c2e
@v1v Adrien Mannocci
Update .ci/scripts/bench.sh
63b56d7
@v1v Adrien Mannocci
Apply suggestions from code review
be6421a
@v1v Adrien Mannocci
Apply suggestions from code review
6e45e21
@v1v @trentm Adrien Mannocci
Apply suggestions from code review
056e2c5 
Co-authored-by: Trent Mick <trent.mick@elastic.co>
Adrien Mannocci
ci: small format cleanup
581fbbb 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Adrien Mannocci
ci: let's debug nvm install
96e60cf 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Adrien Mannocci
ci: correct benchmark prepare step
419e217 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Adrien Mannocci
ci: flush positional arguments and use major version
b0804c1 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Adrien Mannocci
ci: properly set NODE_VERSION env var
a4535eb 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Adrien Mannocci
ci: use nvmrc instead of explicit value
f0c511a 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Adrien Mannocci
ci: small cleanup
76c86e3 
Signed-off-by: Adrien Mannocci <adrien.mannocci@elastic.co>
Adrien Mannocci
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
Adrien Mannocci
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
Adrien Mannocci @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.

5 participants

@v1v @trentm @amannocci @reakaleek @elastic-apm-tech