perf(cmd-api-server): add demonstration of continuous benchmarking #3007
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
petermetz
requested review from
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
petermetz
force-pushed
the
petermetz/issue2672
branch
6 times, most recently
from
f696762 to
574cc9b
Compare
outSH
requested changes
Jan 31, 2024
petermetz
force-pushed
the
petermetz/issue2672
branch
from
574cc9b to
8a0ac09
Compare
outSH
approved these changes
Feb 1, 2024
jagpreetsinghsasan
approved these changes
Feb 2, 2024
There was a problem hiding this comment.
LGTM
@ruzell22 please refer to this implementation if this is what you are trying to work on, for Besu plugin.
Refactored the test cases so that they use the build-in file system libraries
of NodeJS to read the file contents of the dynamically pulled in package.json
The dynamic import with import type assertion cannot work because we do not
yet have the migration to ESM completed and therefore it won't compile
if you try to do it like it's supposed to be:
```typescript
const { default: data } = await import("./foo.json", { assert: { type: "json" } });
```
When running the test cases in question it would produce the failure pasted below:
```sh
not ok 3 TypeError [ERR_IMPORT_ASSERTION_TYPE_MISSING]: Module
"file:///home/runner/work/cacti/cacti/.tmp/test/test-cmd-api-server/
plugin-import-from-github_test/5c711023-7573-4272-aee9-c743f5346ce7/
fad4ca80-c93a-4611-9a68-207c9ad2085e/node_modules/@hyperledger/
cactus-dummy-package/package.json" needs an import assertion of type "json"
---
operator: error
at: bound call (/home/runner/work/cacti/cacti/node_modules/
tape-promise/node_modules/onetime/index.js:30:12)
stack: |-
TypeError [ERR_IMPORT_ASSERTION_TYPE_MISSING]: Module
"file:///home/runner/work/cacti/cacti/.tmp/test/test-cmd-api-server/
plugin-import-from-github_test/5c711023-7573-4272-aee9-c743f5346ce7/
fad4ca80-c93a-4611-9a68-207c9ad2085e/node_modules/@hyperledger/
cactus-dummy-package/package.json" needs an import assertion of type "json"
at new NodeError (node:internal/errors:405:5)
at validateAssertions (node:internal/modules/esm/assert:95:15)
at defaultLoad (node:internal/modules/esm/load:91:3)
at nextLoad (node:internal/modules/esm/loader:163:28)
at ESMLoader.load (node:internal/modules/esm/loader:603:26)
at ESMLoader.moduleProvider (node:internal/modules/esm/loader:457:22)
at new ModuleJob (node:internal/modules/esm/module_job:64:26)
at ESMLoader.#createModuleJob (node:internal/modules/esm/loader:480:17)
at ESMLoader.getModuleJob (node:internal/modules/esm/loader:434:34)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
...
```
Signed-off-by: Peter Somogyvari <[email protected]>
Primary change: --------------- This is the ice-breaker for some work that got stuck related to this issue: https://github.com/hyperledger/cacti/issues/2672 The used benchamking library (benchmark.js) is old but solid and has almost no dependencies which means that we'll be in the clear longer term when it comes to CVEs popping up. The benchmarks added here are very simple and measure the throughput of the API server's Open API spec providing endpoints. The GitHub action that we use is designed to do regression detection and reporting but this is hard to verify before actually putting it in place as we cannot simulate the CI environment's clone on a local environment. The hope is that this change will make it so that if someone tries to make a code change that will lower performance significantly, then we can catch that at the review stage instead of having to find out later. Secondary change: ----------------- 1. Started using tsx in favor of ts-node because it appers to be about 5% faster when looking at the benchmark execution. It also claims to have less problems with ESM compared to ts-node so if this initial trial goes well we could later on decide to swap out ts-node with it project-wide. Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
force-pushed
the
petermetz/issue2672
branch
from
8a0ac09 to
623511d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Primary change:
This is the ice-breaker for some work that got stuck related to this issue:
https://github.com/hyperledger/cacti/issues/2672
The used benchamking library (benchmark.js) is old but solid and has
almost no dependencies which means that we'll be in the clear longer term
when it comes to CVEs popping up.
The benchmarks added here are very simple and measure the throughput of
the API server's Open API spec providing endpoints.
The GitHub action that we use is designed to do regression detection and
reporting but this is hard to verify before actually putting it in place
as we cannot simulate the CI environment's clone on a local environment.
The hope is that this change will make it so that if someone tries to
make a code change that will lower performance significantly, then we
can catch that at the review stage instead of having to find out later.
Secondary change:
5% faster when looking at the benchmark execution. It also claims to have
less problems with ESM compared to ts-node so if this initial trial
goes well we could later on decide to swap out ts-node with it project-wide.
Signed-off-by: Peter Somogyvari [email protected]
Pull Request Requirements
upstream/mainbranch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-sflag when usinggit commitcommand. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.