feat: advanced benchmarking #25
Conversation
|
The scripts are a bit "rough around the edges" and would definitely benefit from a good refactoring. However, I wanted to publish the results as soon as possible. |
benchmark/scripts/consolidate.ts
Outdated
| const gitBranch = execSync("git rev-parse --abbrev-ref HEAD 2>/dev/null", { | ||
| encoding: "utf-8", | ||
| }).trim(); |
benchmark/scripts/consolidate.ts
Outdated
| const raw: IResult = JSON.parse( | ||
| await fsPromises.readFile(rawFilePath, "utf8") | ||
| ); | ||
| let result: IResult = { |
benchmark/scripts/consolidate.ts
Outdated
| let count = { functionCalls: 0, logEntries: 0 }; | ||
| let consolidatedEntry: Record<string, any> = {}; | ||
| let requestId: string | undefined; | ||
| for (const entry of raw.log) { |
There was a problem hiding this comment.
some comments might be helpful - it looks like this is aggregating the results of multiple runs into a single result?
benchmark/scripts/consolidate.ts
Outdated
| console.log(`- Build datasets`); | ||
| const datasetPointers: string[] = []; | ||
| for (const entry of result.log) { | ||
| const label = entry.name.substring(0, entry.name.lastIndexOf("-")); |
There was a problem hiding this comment.
i'm curious, do we control the format of this log? Is there a reason it's not in an easy to parse JSON format?
There was a problem hiding this comment.
The stack deploys n instances of the same lambda (10 by default). the instance name is suffixed with its iteration number. Thus, we need to strip the suffix to consolidate the data of the same kind of a function.
And of course CloudWatch log is text only. You need to find your json in the message, extract the logs of the API call and then you can parse it.
Only then comes the aggregation part by type and metric.
benchmark/scripts/consolidate.ts
Outdated
| if (!result.datasets.coldStarts.totalDuration[label]) { | ||
| datasetPointers.push( | ||
| `datasets.${targetDataset}.totalDuration.${label}` | ||
| ); | ||
| result.datasets.coldStarts.totalDuration["title"] = | ||
| "Cold start: total duration"; | ||
| result.datasets.coldStarts.totalDuration[label] = { | ||
| label: label, | ||
| order: CONFIG.functions.reduce( | ||
| (prev, curr) => | ||
| curr.functionName === label ? curr.chart.order : prev, | ||
| 0 | ||
| ), | ||
| data: [entry.function.initDuration + entry.function.duration], | ||
| }; | ||
| } else { | ||
| result.datasets.coldStarts.totalDuration[label].data.push( | ||
| entry.function.initDuration + entry.function.duration | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
In general, I'd recommend extracting these into dedicated functions and leaning towards pure functions instead of mutating state. I'm not too pure/opinionated, but procedures with a ton of mutation operations can be hard to refactor and read. This could be computeTotalDuration and return the properties you need. Extract each computation into pure functions and then compose them together with map/reduce logic.
Feel free to ignore this feedback as it would require substantial rework which is not necessary at this time.
There was a problem hiding this comment.
True. This is part of the refactoring I mentioned previously. I posted the results quickly however I will do some refactoring now to avoid technical debt accumulation.
benchmark/scripts/run.ts
Outdated
| const command = new DeleteLogGroupCommand({ logGroupName }); | ||
| console.log(` - log group '${logGroupName}'`); |
There was a problem hiding this comment.
why are you deleting cloudwatch logs?
There was a problem hiding this comment.
I flush any logs from potential previous runs to avoid data corruption.
It will also be useful to prevent redeploying the benchmark stack across features/branche if it has not changed
benchmark/scripts/run.ts
Outdated
| const cloudwatchClient = new CloudWatchLogsClient({}); | ||
| await cloudwatchClient.send(command); | ||
| cloudwatchClient.destroy(); |
There was a problem hiding this comment.
why create and destroy? Usually better to create once per node runtime
There was a problem hiding this comment.
This is mandatory. I issued random timeouts from the SDK when reusing the same instance. I had no other choice but creating a fresh instance every time ... 😞
There was a problem hiding this comment.
Hmm that is odd. Not sure why a separate instance would fix that. I'd be interested in seeing if that's still the behavior
benchmark/scripts/run.ts
Outdated
| } catch { | ||
| null; | ||
| } |
tsconfig.esm.json
Outdated
| @@ -1,9 +1,11 @@ | |||
| { | |||
| "extends": "./tsconfig.base.json", | |||
| "include": ["src"], | |||
| "include": ["src/**/*"], | |||
There was a problem hiding this comment.
i don't think this is necssary is it?
| { | ||
| "files": [], | ||
| "references": [ | ||
| { "path": "./tsconfig.esm.json" }, | ||
| { "path": "./tsconfig.test.json" } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Should this be the default tsconfig.json?
There was a problem hiding this comment.
I don't think so.
I didn't want the build command to build the benchmark. This is handled by the CDK. That's why I removed the benchmark project from tsconfig.build.json.
However I left every project in the default tsconfig.json for the IDE. I'm not sure about the consequences of not referencing every project in the root tsconfig.json, however I think it may cause unwanted side effects like unnecessary TSServer reloads when switching files.
There was a problem hiding this comment.
I usually have the root tsconfig.json reference every sub project because the IDE uses the root by default. The consequence is that it will build everything when you run tsc-b from the root, which is probably what we want. Not high priority - we can figure it out.
There was a problem hiding this comment.
We agree. That's what is implemented here: tsconfig.json references every sub project including @itty-aws/benchmark for the IDE.
But this is tsconfig.build.json that we are talking about, and I removed benchmark/tsconfig.json from it since the benchmark sub-project is not to be build through the main pnpm build command. In this subproject, the CDK takes care of building the lambda functions automatically with esbuild when deploying through pnpm bench:deploy.
I will do a thorough refactoring of the benchmark. Scripts do too many things at once and I'm going to break them down into simpler functions. I will also reshape the data model and the corresponding types to simplify the implementation. I will include your reviews and comments in the refactoring, as they all make sense. Thank you for your much appreciated feedback. |
|
I saw that you recently pushed some changes. Ready for another review? I also need to take some time to fix the build - something is up with that. |
|
I'm in the final stretch of my refactoring. I'll let you know as soon as I'm done. |
Hi
@itty-aws/benchmarktsconfigsetup to exclude the benchmark package from the build process/benchmark/README.md/benchmark/data/advanced-benchmarking/README.mdTo keep things seperated, I propose that we talk about the result of the benchmark in issue #21 (project status)
Closes #22, Closes #25