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

action: tav #3127

Merged
merged 10 commits into from
Mar 29, 2023
Merged

action: tav #3127

merged 10 commits into from
Mar 29, 2023

Conversation

v1v
Copy link
Member

@v1v v1v commented Jan 25, 2023

What

Migrate TAV to run as GitHub actions when

  • a merge commit in the main branch
  • manually triggered

TAV matrix is declared in the GH workflow hence no more .tav files to be maintained.

Checklist

  • Implement code

Caveats

We agreed recently that It will not run on PRs for now, if needed we can add that feature in a follow up, maybe using the smarter jenkins implementation if it still works.

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jan 25, 2023
@apmmachine
Copy link
Contributor

apmmachine commented Jan 25, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-03-27T09:36:46.445+0000

  • Duration: 24 min 39 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

  • run benchmark tests : Run the benchmark test only.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@v1v v1v mentioned this pull request Jan 25, 2023
8 tasks
v1v added 2 commits March 21, 2023 09:36
* upstream/main: (44 commits)
  action: abort builds when new commit (elastic#3196)
  docs: note that 3.40.0 was bad release (elastic#3194)
  chore(deps-dev): bump @babel/cli from 7.20.7 to 7.21.0 (elastic#3170)
  chore(deps-dev): bump @babel/core from 7.20.2 to 7.21.0 (elastic#3169)
  ci: drop max-parallel for GH actions (elastic#3191)
  test: correct sense of test message (elastic#3186)
  3.43.0 (elastic#3184)
  test, ci: some small changes (elastic#3183)
  ci: limit parallel GH Action test runs to try to avoid 429 errors on checkout (elastic#3180)
  fix: transaction name for Next.js API routes in [email protected] was broken (elastic#3178)
  mongodb@5 support (elastic#3177)
  chore(deps-dev): bump body-parser from 1.20.1 to 1.20.2 (elastic#3171)
  chore(deps-dev): bump restify from 11.0.0 to 11.1.0 (elastic#3172)
  docs: minor fix in README for Azure Functions example (elastic#3175)
  feat: Make Agent.flush() return a Promise if no callback is passed as param (elastic#3167)
  chore(deps-dev): bump @types/node from 18.11.9 to 18.14.0 (elastic#3165)
  chore(deps-dev): bump @hapi/hapi from 21.2.1 to 21.3.0 (elastic#3166)
  chore(deps-dev): bump undici from 5.19.1 to 5.20.0 (elastic#3164)
  chore(deps-dev): bump fastify from 4.12.0 to 4.13.0 (elastic#3154)
  Use composite action for updatecli workflow (elastic#3162)
  ...
@v1v v1v self-assigned this Mar 21, 2023
@v1v v1v requested review from a team March 21, 2023 08:47
@v1v v1v marked this pull request as ready for review March 21, 2023 08:48
@v1v v1v requested a review from trentm March 21, 2023 17:54
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.

How can one trigger a TAV run on a PR?

  • a full TAV run
  • just for a particular module
  • just for a particular node version

- "12"
- "10"
- "8"
framework:
Copy link
Member

Choose a reason for hiding this comment

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

nit: Maybe call this "module" instead of "framework". This is minor, however.

@v1v
Copy link
Member Author

v1v commented Mar 27, 2023

How can one trigger a TAV run on a PR?
a full TAV run
just for a particular module
just for a particular node version

I probably misunderstood what we discussed online the other day, somehow I thought the smart jenkins stuff was never used hence I kept it simple to run only on the main branch or manually when needed.

If the above is needed, then let's discuss about each individual scenario:

  • a full TAV run on a PR basis

  • just for a particular module and just for a particular node version

    • Same as above, either specific labels or using a github command, unless we wanna use the git changeset and compare what modules have been changed.

A few other questions:

  1. What's your preference in terms of usability?
  2. What's the one of the above options that we can discard?
  3. Any other ideas?

Thanks

@trentm
Copy link
Member

trentm commented Mar 27, 2023

How can one trigger a TAV run on a PR?
...

I probably misunderstood what we discussed online the other day, somehow I thought the smart jenkins stuff was never used hence I kept it simple to run only on the main branch or manually when needed.

@v1v I think we only need the ability to manually start a TAV test run. I don't think we need any smart automatic triggering. I also don't have any requirements for how we do that -- any of the following would be fine: a GitHub comment, clicking some GitHub checks UI, running some gh workflow ... command. Whatever is cleanest/easiest.


Should we be able to see some "TAV / ..." GitHub checks on this PR?

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.

Approving, though I'm still not sure what the suggested way to manually trigger a TAV run is.

@v1v
Copy link
Member Author

v1v commented Mar 28, 2023

Should we be able to see some "TAV / ..." GitHub checks on this PR?

I'm afraid new GH actions coming from forked repositories are not executed. Let me create a branch in the upstream and cherry-pick the changes so you can see how it looks like.

See #3221

@v1v v1v mentioned this pull request Mar 28, 2023
@trentm
Copy link
Member

trentm commented Mar 28, 2023

I'm afraid new GH actions coming from forked repositories are not executed. Let me create a branch in the upstream and cherry-pick the changes so you can see how it looks like.

Oh, right. Thanks for doing that. This looks very good.


Some TAV comparisons between GH Actions and Jenkins:

Screenshot 2023-03-28 at 8 41 18 AM
Screenshot 2023-03-28 at 8 41 11 AM

@trentm
Copy link
Member

trentm commented Mar 28, 2023

@v1v Do you know if there will be a way to manually run just a part of the TAV tests on a PR? For example, say I have a PR that is working on the "fastify" module. Can I run the TAV tests for module=fastify for either all node versions, or just a subset of the node versions?

This doesn't work:

% gh workflow run tav.yml --ref test/action-tav -f node=16 -f module=fastify
could not create workflow dispatch event: HTTP 422: Unexpected inputs provided: ["module", "node"] (https://api.github.com/repos/elastic/apm-agent-nodejs/actions/workflows/46437793/dispatches)

because the matrix values are not the same as workflow dispatch inputs. I haven't played with workflow dispatch inputs in GH Actions, so I don't know if what I'm asking is possible.

This isn't a requirement for this PR. We could look into this separately.

@trentm
Copy link
Member

trentm commented Mar 28, 2023

I notice this from https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategymatrix

A matrix will generate a maximum of 256 jobs per workflow run. This limit applies to both GitHub-hosted and self-hosted runners.

Currently we have 35 modules * 7 node versions = 245 jobs. So we are very close to the maximum currently. :)
When node v20 is released (expected 2023-04-18) and there are initial node v21 releases (I'm not sure when), then we'll be over the limit.
Or, if/when we add two new instrumented modules, then we'll be over the limit.
This is just something to be aware of. We may have a new major version of the APM agent by then that drops support for some of the older ancient node versions. Or we could stop TAV testing for the deprecated node v8. Or stop testing some older, less useful modules like got or bluebird.

@v1v
Copy link
Member Author

v1v commented Mar 28, 2023

@v1v Do you know if there will be a way to manually run just a part of the TAV tests on a PR? For example, say I have a PR that is working on the "fastify" module. Can I run the TAV tests for module=fastify for either all node versions, or just a subset of the node versions?

I'd like to work on this in a follow up, so we can use one of the above proposals (#3127 (comment))

Currently we have 35 modules * 7 node versions = 245 jobs. So we are very close to the maximum currently. :)

We managed to use buckets in the apm-agent-python:

It's not ideal, otherwise if possible we could create combinations of two modules per runner, though I don't know whether that could cause any other issues (tearing down the environment might be required)

To find TAV tests that were taking very long to run (to know if we should look into reducing the number of versions tested), I had written https://github.com/elastic/apm-agent-nodejs/blob/main/dev-utils/jenkins-build-slow-steps.sh At some point I'll see about writing a replacement for these GH Actions. That definitely is not a requirement for this work.

You can use the existing opentelemetry data :) https://ela.st/oblt-ci-cd-stats. (though it's only accessible for Elasticians and the new TAV GH action won't be available until this PR gets merged)

Our plan is to leverage the existing GitHub metadata and provide insights that are meaningful. We are at the moment with the migration stage, but definitely it's something we will invest effort so we can help to answer those kind of questions.

@trentm
Copy link
Member

trentm commented Mar 28, 2023

You can use the existing opentelemetry data :)

That'll be cool to browse around, and yes perhaps we could make a dashboard from that data.

Here is the beginnings of a replacement using the GH API and gh:

#!/bin/bash
branch=main
branch=test/action-tav
latestTavRun=$(gh run list -R elastic/apm-agent-nodejs -b "$branch" -w TAV -L 1 --json databaseId --jq '.[].databaseId')
gh api --paginate repos/elastic/apm-agent-nodejs/actions/runs/$latestTavRun/jobs \
    | json -ga jobs \
    | json -ga -e '
        this.s = (new Date(this.completed_at) - new Date(this.started_at)) / 1000;
        this.minSec = Math.floor(this.s/60) + "m" + (this.s%60).toString().padStart(2,"0") + "s"
        ' s minSec name \
    | sort -n

Running that yields:

...
1118 18m38s test-tav (10, aws-sdk)
1125 18m45s test-tav (12, tedious)
1129 18m49s test-tav (10, fastify)
1199 19m59s test-tav (8, aws-sdk)
1203 20m03s test-tav (12, pg)
1221 20m21s test-tav (8, pg)
1248 20m48s test-tav (10, mongodb-core)
1256 20m56s test-tav (14, next)
1307 21m47s test-tav (12, knex)
1307 21m47s test-tav (14, knex)
1323 22m03s test-tav (10, pg)
1386 23m06s test-tav (12, graphql)
1431 23m51s test-tav (14, tedious)
1496 24m56s test-tav (10, graphql)
1508 25m08s test-tav (8, graphql)
1757 29m17s test-tav (14, graphql)
1794 29m54s test-tav (10, knex)

@v1v v1v merged commit 6660cc3 into elastic:main Mar 29, 2023
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
Migrate TAV to run as GitHub actions when
- a merge commit in the `main` branch
- manually triggered

TAV matrix is declared in the GH workflow hence no more .tav files to be maintained.

We agreed recently that It will not run on PRs for now, if needed we can add that feature in a follow up, maybe using the smarter jenkins implementation if it still works.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants