-
-
Notifications
You must be signed in to change notification settings - Fork 5
Convert local setup to a runner #63
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
base: main
Are you sure you want to change the base?
Conversation
3c7d75f to
3c0dfb0
Compare
596a16a to
8de4e78
Compare
3c0dfb0 to
bf79959
Compare
|
There are still quite a few gaps:
But @RafaelGSS are you alright with this direction so that we align with the other runner styles and features? |
RafaelGSS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry delay, @wesleytodd, a lot of things are going on recently, as you might know.
To be honest, I was trying to say "ditch that abstraction entirely" - It's a personal preference, so I'm fine leaving it as internal packages. My feeling that this increases the contribution barrier and makes things overly complex.
Also, I do believe Wrk2 (at least) should be a global binary - making it available through this CLI will make things very complex as the binary needs different dependencies according to each supported environment. Leaving that open for the "sysadmin" seems a better choice to not make things more complex than it is nowadays.
(That's what we do for Node.js too)
b5706bc to
7e872b6
Compare
7e872b6 to
5a5c185
Compare
|
Ping me when this is ready to review @wesleytodd |
|
Will do, just cleaning things up and will move it from draft when it is ready. |
5a5c185 to
3499db7
Compare
cli: add local-bench to exbf Output example > @expressjs/perf-wg@1.0.0 local-load > expf local-load Running autocannon load... Result Autocannon: 121610.67 Running wrk2 load... Result Wrk2: [ { percentile: 50, ms: 0.829 }, { percentile: 75, ms: 1.1 }, { percentile: 90, ms: 1.41 }, { percentile: 99, ms: 1.75 }, { percentile: 99.9, ms: 1.88 }, { percentile: 99.99, ms: 2 }, { percentile: 99.999, ms: 2.2 }, { percentile: 100, ms: 2.2 } ]
3499db7 to
e764ef6
Compare
2870e11 to
ce4622a
Compare
ce4622a to
a61605e
Compare
|
|
||
| // Start here, await in .results() | ||
| const toAwait = requesters.flatMap((requester) => { | ||
| return requests.map((request) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By iterating the requests array we start one cli process per request since the cli's don't support making multiple request shapes well, but also so that we can compare to see if one specific request was the cause of problems. We may want to have a setup to separate each out and compose them back together in the future, but for now this means we can support patterns like #77 for more "real world" tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'd make it opt-in.
if (parallelRequests)
return Promise.all(requests.map(req => requester.start(req))
else {
const results = []
for (const req of requests) results.push(await requester.start(req))
}We definitely don't want this to be default for the simple process.
| "name": "@expressjs/perf-requests", | ||
| "version": "1.0.0", | ||
| "exports": { | ||
| ".": "./index.mjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think after this we may be able to get rid of the exports with a small refactor to make adding request sets easier in the future. I am not going to do that now since this needs to land so we can get the CI integration over the finish line, but just wanted to call out why I added this file here.
|
|
||
| // TODO: I don't see docs on how to do this with wrk2 | ||
| if (opts.method || this.method) { | ||
| throw new Error('not yet supported'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method and body did not show up in the readme and since I didn't get wrk2 building on my mac I just left these here for now. We should fix this before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can test it.
| }); | ||
| } | ||
|
|
||
| processResults (output) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I didn't get wkr2 built locally I also didn't test this after the refactor. I think I faithfully copied it but this needs more testing before we land this.
| app.post(expressVersion.startsWith('4.') ? '*' : '*path', (req, res) => { | ||
| res.status(200).json({ | ||
| hello: 'body!', | ||
| method: req.method, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just to test that the new request were passing method through correctly. Not necessary for this PR.
| } | ||
|
|
||
| // -H/--headers K=V | ||
| for (const [header, value] of Object.entries(opts.headers || this.headers)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opts.headers and this.headers should probably merge?
RafaelGSS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's also missing a doc update.
| // -c/--connections NUM | ||
| '-c', this.connections, | ||
| // -w/--workers NUM | ||
| '-w', Math.min(this.connections, availableParallelism() || 8), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| '-w', Math.min(this.connections, availableParallelism() || 8), | |
| '-w', Math.min(this.connections, availableParallelism() || 4), |
8 sounds too much for the dedicated machine (availableParallelism is not available on v20<)
|
|
||
| // Start here, await in .results() | ||
| const toAwait = requesters.flatMap((requester) => { | ||
| return requests.map((request) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'd make it opt-in.
if (parallelRequests)
return Promise.all(requests.map(req => requester.start(req))
else {
const results = []
for (const req of requests) results.push(await requester.start(req))
}We definitely don't want this to be default for the simple process.
|
|
||
| // TODO: I don't see docs on how to do this with wrk2 | ||
| if (opts.method || this.method) { | ||
| throw new Error('not yet supported'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can test it.
Took a quick look at converting @RafaelGSS's local server/client into a runner. I didn't have time this morning to look at the wrk2/autocannon stuff, but can assuming that #62 was not trying to say "ditch that abstraction entirely", which maybe it is and then I would like to discuss why we should do that.