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

ci: add loadtest workflow & script #6129

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tchapacan
Copy link

@tchapacan tchapacan commented Nov 3, 2024

Purpose of this PR is to add a new github workflow and script which will run the benchmarks for express.

  • Automatically at each PR opened & synced
    • Will compare master branch and the branch from the PR

    The result will be added as comment in the PR.

  • Manually, you can dispatch the workflow
    • You can compare 2 versions of express being published
    • You can compare 2 branches of the express repo
    • You can compare one branch of the express repo and one version of express being published

    The result will be added as summary of the workflow run

The new script is based from your initial work here https://github.com/UlisesGascon/express-performance-comparator

I reused the benchmarks/middleware.js with minimal changes to not alter the original work.

I have created a test repo if you would like to test the workflow here => https://github.com/tchapacan/express-test-wf/

This PR seems a little big to review at once and lack of tests and docs (if necessary), I can split it in smaller PRs if you prefer, ex:

  • One PR to add the load-test-workflow.js script
  • One PR for tests if relevant? (don't really know where to land them smoothly in the repo)
  • One PR to modify the benchmarks/middleware.js script
  • One PR to add the load-test.yml workflow
  • One PR to document the workflow usage somewhere relevant (don't really know where to land it smoothly in the repo)
  • One PR to modify the number of middleware/connection to fine tune benchmark scenario afterward?

LMK if this proposition suits you. I understand that CI might need further discussion between you, specifically if you have already planned/discussed something. Maybe this deserve it's own repo (if more feature, docs, tests are needed then) ?
Furthermore we have to keep in mind that benchmark will run on the runner and the result will strictly depend on it.

Anyway, I'm 100% open to discussion/changes about that, hope it's helping you a little, cheers!

@tchapacan tchapacan changed the title feat: add loadtest workflow & script ci: add loadtest workflow & script Nov 4, 2024
@UlisesGascon
Copy link
Member

Thanks a lot @tchapacan! This is a huge step for the project ❤️

I will love to have a deep discussion in this PR with the community too and later on we can split this PR on smaller pieces once we are all in the same page.

cc: @expressjs/express-tc @expressjs/triagers @andrehrferreira @cesco69 @nigrosimone @faulpeltz

Note: Just to avoid accidental merges I will change this PR to a draft

@UlisesGascon UlisesGascon self-assigned this Nov 11, 2024
@UlisesGascon UlisesGascon marked this pull request as draft November 11, 2024 16:14
@tchapacan
Copy link
Author

No problem my pleasure, happy it help and could give a starting point even if it's basic 😄. Then I'll let you discuss with the community about how you wanna align and plan that (I'm sure there might be other use case that could be turn into reusable workflow, even in a dedicated repo) , I'll stick around this PR 👀

@wesleytodd
Copy link
Member

Yeah, one thing is I dont want to rely on GH runner perf for our benchmarks. They are VERY unreliable and we don't control the execution environment enough for them to be reliable. Unfortunately I am just catching up on backed up notification queue, and so I will not be able to help with more direction on this for a bit. Could we keep this in draft until we can schedule a deep dive conversation on one of our working sessions and invite you @tchapacan?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants