Skip to content

action: support github commands for the tav modules #3246

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
Merged
merged 11 commits into from
May 10, 2023
Merged

action: support github commands for the tav modules #3246

merged 11 commits into from
May 10, 2023

Conversation

v1v
Copy link
Member

@v1v v1v commented Apr 5, 2023 •
edited
Loading

What

Support /test tav, /test tav module1,module2, /test tav module1,module2 18,16, /test tav all and /test tav all 16

Why

Help with running specific modules for specific node versions, in addition, the default behaviour will read the versions and modules from .ci/tav.json

Test

/test tav triggered https://github.com/elastic-robots/gh-actions-interactions/actions/runs/4622685839

/test tav all triggered https://github.com/elastic-robots/gh-actions-interactions/actions/runs/4622723259 (similarly to /test tav)

/test tav acme,foo triggered https://github.com/elastic-robots/gh-actions-interactions/actions/runs/4622688577

/test tav all 1,2 triggered https://github.com/elastic-robots/gh-actions-interactions/actions/runs/4622689567

/test tav acme,foo 2 triggered https://github.com/elastic-robots/gh-actions-interactions/actions/runs/4622709699

Update

Changed to use the pull_request_review event so we can warranty every run will be attached to a particular sha commit. elastic-robots/gh-actions-interactions#4 can be used for testing a similar workflow.

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Apr 5, 2023
@v1v v1v marked this pull request as draft April 5, 2023 17:55
@v1v v1v marked this pull request as ready for review April 5, 2023 20:50
@v1v v1v requested review from a team April 5, 2023 20:50
@v1v v1v mentioned this pull request Apr 5, 2023
Closed
6 tasks
v1v
v1v commented Apr 5, 2023
View reviewed changes
trentm
trentm reviewed Apr 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.

Thanks! This is looking good.

v1v
v1v commented Apr 6, 2023
View reviewed changes
.github/workflows/tav.yml

test-tav:
needs: prepare-matrix
runs-on: ubuntu-latest
timeout-minutes: 40
strategy:
max-parallel: 30
max-parallel: 15
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no need to run in parallel more since it's a merge build, so it's not happening on PRs.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what you mean here. Our test matrix is approximately 250 jobs now. If we run only 15 in parallel it may take twice as long to complete them all. Currently (see some examples at https://github.com/elastic/apm-agent-nodejs/actions/workflows/tav.yml) a TAV run is taking 1.5h to complete. It would be unfortunate if it takes 3h to complete.

Is there a reason to want lower max-parallel? Is there added cost?

trentm
trentm approved these changes Apr 6, 2023
View reviewed changes
v1v added 2 commits May 8, 2023 08:52
@v1v
Merge branch 'main' of github.com:elastic/apm-agent-nodejs into featu…
a9130f7 
…re/support-specific-modules

* 'main' of github.com:elastic/apm-agent-nodejs: (54 commits)
  chore: fix dev-utils/ci-tav-slow-jobs.sh (elastic#3319)
  test: reduce TAV test matrix for slowest jobs (elastic#3321)
  chore: sync package-lock so 'npm ci' can work (elastic#3318)
  docs: document `useElasticTraceparentHeader` config var (elastic#3316)
  chore, test: test driver improvements (elastic#3293)
  test: drop node 14 from RC tests now that it is EOL (elastic#3315)
  test: fix running fastify.test.js with node v8 (elastic#3317)
  feat: add @apollo/server@4 support (elastic#3203)
  chore: update nvm (elastic#3309)
  tests: stop testing 'express-graphql' instrumentation (elastic#3304)
  chore: fix bitrot.js dev util for recent changes (elastic#3308)
  test: restore testing of Azure Functions on node >=18.x (elastic#3307)
  fix: support Lambda instrumentation for `contextManager: 'patch'`; refactor Lambda tests (elastic#3305)
  test: fix fastify TAV test failures (elastic#3314)
  test: fix @aws-sdk/client-s3 TAV test failures (elastic#3312)
  feat: add instrumentation for aws-sdk S3 client (elastic#3287)
  feat(fastify): add captureBody support (elastic#2681)
  feat: mysql2@3 support (elastic#3301)
  chore(deps): bump @opentelemetry/exporter-prometheus from 0.37.0 to 0.38.0 in /test/opentelemetry-metrics/fixtures (elastic#3295)
  chore(deps-dev): bump fastify from 4.16.3 to 4.17.0 (elastic#3296)
  ...
@v1v
ci: use pull_request_review
a3f17ea 
since it's linked to a specific git sha commit

hence it's more secured and there is no need to create a reaction with the status
v1v
v1v commented May 8, 2023
View reviewed changes
.github/workflows/tav-command.yml
name: tav-command

on:
pull_request_review:
Copy link
Member Author

Choose a reason for hiding this comment

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

pull_request_review worked like a charm but pull_request_review_comment didn't get triggered. In any case pull_request_review is the one linked to a particular sha commit, hence github.event.pull_request.head.sha is the correct data to be used when checking out the source code.

@v1v v1v requested a review from trentm May 8, 2023 08:12
@v1v v1v self-assigned this May 8, 2023
@david-luna
Copy link
Member

david-luna commented May 8, 2023 •
edited
Loading

Thanks @v1v ! this will help a lot to improve testing of the agent.

I wonder if you considered to have the scripts in separate file, se it in the docs

@v1v
Copy link
Member Author

v1v commented May 8, 2023

I wonder if you considered to have the scripts in separate file, se it in the docs

No strong opinion. If that's better I'm happy to change it.

@david-luna
Copy link
Member

I wonder if you considered to have the scripts in separate file, se it in the docs

No strong opinion. If that's better I'm happy to change it.

No need to. We can move it later if the script grows enough.

Looks more than good to me :)

trentm
trentm approved these changes May 8, 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.

@v1v Thanks! Just the one question. I'm fine with merging before that question is answered, if that is easier. We can always adjust later.

.github/workflows/tav.yml

test-tav:
needs: prepare-matrix
runs-on: ubuntu-latest
timeout-minutes: 40
strategy:
max-parallel: 30
max-parallel: 15
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what you mean here. Our test matrix is approximately 250 jobs now. If we run only 15 in parallel it may take twice as long to complete them all. Currently (see some examples at https://github.com/elastic/apm-agent-nodejs/actions/workflows/tav.yml) a TAV run is taking 1.5h to complete. It would be unfortunate if it takes 3h to complete.

Is there a reason to want lower max-parallel? Is there added cost?

@v1v
Copy link
Member Author

v1v commented May 9, 2023 •
edited
Loading

Is there a reason to want lower max-parallel? Is there added cost?

It's not about cost, since we use the existing GitHub runners and no hosted runners but the GitHub quota for the whole Elastic org. Back in the days was about 200 concurrent jobs though it has been increased to 500. See https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration

Using a lower max-parallel can help with an even distributed load between all the GitHub repositories using GitHub actions at elastic.

In this particular case, since the whole tav does not run per PR but per merge, I thought that limiting the max-parallel can help to distribute the workload since it's not part of the critical path.

@trentm
Copy link
Member

trentm commented May 9, 2023

In this particular case, since the whole tav does not run per PR but per merge, I thought that limiting the max-parallel can help to distribute the workload since it's not part of the critical path.

Okay, fair. Thanks.

@v1v v1v merged commit dc5762f into elastic:main May 10, 2023
@v1v v1v deleted the feature/support-specific-modules branch May 10, 2023 06:08
@trentm
Copy link
Member

trentm commented May 11, 2023

Thanks!

trentm pushed a commit that referenced this pull request May 16, 2023
@v1v @trentm
action: support github commands for the tav modules (#3246)
37838c8
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
@v1v @PeterEinberger
action: support github commands for the tav modules (elastic#3246)
3d0c4bd
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

@david-luna david-luna david-luna approved these changes

@reakaleek reakaleek reakaleek approved these changes

Assignees

@v1v v1v

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 @david-luna @trentm @reakaleek