-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat(tests): initial performance test #2828
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
a835667
to
89e7e19
Compare
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.
Seems like this is unused... Remove?
Both pnpm --filter performance run-tests
and pnpm --filter performance run-tests-docker
use run-test.sh
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.
Good catch, thanks. Meant to remove it
headers | ||
} | ||
) | ||
const receiver = JSON.parse(createReceiverResponse.body).data.createReceiver |
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.
Im seeing some errors like this when spinning up the localenv and running pnpm --filter performance run-tests-docker
:
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createReceiver' of undefined\n\tat file:///scripts/create-outgoing-payments.js:125:64(84)\n" executor=constant-vus scenario=default source=stacktrace
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createReceiver' of undefined\n\tat file:///scripts/create-outgoing-payments.js:125:64(84)\n" executor=constant-vus scenario=default source=stacktrace
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createReceiver' of undefined\n\tat file:///scripts/create-outgoing-payments.js:125:64(84)\n" executor=constant-vus scenario=default source=stacktrace
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createReceiver' of undefined\n\tat file:///scripts/create-outgoing-payments.js:125:64(84)\n" executor=constant-vus scenario=default source=stacktrace
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createReceiver' of undefined\n\tat file:///scripts/create-outgoing-payments.js:125:64(84)\n" executor=constant-vus scenario=default source=stacktrace
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createReceiver' of undefined\n\tat file:///scripts/create-outgoing-payments.js:125:64(84)\n" executor=constant-vus scenario=default source=stacktrace
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createReceiver' of undefined\n\tat file:///scripts/create-outgoing-payments.js:125:64(84)\n" executor=constant-vus scenario=default source=stacktrace
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createReceiver' of undefined\n\tat file:///scripts/create-outgoing-payments.js:125:64(84)\n" executor=constant-vus scenario=default source=stacktrace
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createReceiver' of undefined\n\tat file:///scripts/create-outgoing-payments.js:125:64(84)\n" executor=constant-vus scenario=default source=stacktrace
time="2024-08-05T18:40:26Z" level=error msg="TypeError: Cannot read property 'createQuote' of undefined\n\tat file:///scripts/create-outgoing-payments.js:159:58(128)\n" executor=constant-vus scenario=default source=stacktrace
9 TypeErrors for createReceiver
and then 1 createQuote
. Looks like its indicating data
in JSON.parse(createReceiverResponse.body).data
is undefined
.
Im not sure what to make of it though because then the VUs seem to run fine. I see some quote times in http://localhost:4500/ but not sure there is enough info there to make much sense of the errors.
If I run pnpm --filter performance run-tests-docker
again without restarting the localenv I dont see these TypeErrors.
As a side note, I was surprised to see no outgoing payment based metrics but this is actually consistent with the bruno peer-to-peer example (guess it happens outside of the outgoing payment lifecycle where these are recorded).
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.
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 these errors are performance issues being exposed by the test. It seems to work more consistently after bringing the number of virtual users down to one, so I suspect that these are symptoms of the service being overwhelmed.
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.
makes sense... I actually dont see it anymore
// A number specifying the number of VUs to run concurrently. | ||
vus: 10, | ||
// A string specifying the total duration of the test run. | ||
duration: '30s' |
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.
Should we bump this up? Im seeing this:
time="2024-08-05T18:51:25Z" level=warning msg="No script iterations fully finished, consider making the test duration longer"
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.
Looks like I need to bump it up to match the issue description anyways:
For now, we can set the scenario to be quite small, a single virtual user (VU) across 10 minutes. We should be able to see the local playground Grafana dashboard properly populate with metrics (and traces).
fi | ||
|
||
# Verify that cloud nine wallet address is live | ||
if curl -s --head --request GET "$c9_wallet_address" | grep "HTTP/1.[01]" > /dev/null; then |
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.
So this is kinda funny....
The outgoing payment metrics should show after all. I was wrong here and the peer-to-peer bruno tests DO populate the outgoing payment based metrics (I was just impatient - underestimated the delay). So the performance tests should populate them too.
You can confirm this beyond the metrics if you do docker logs rafiki-cloud-nine-mock-ase-1
after running the tests and you should see many of these:
Error: No account found for wallet address
at handleOutgoingPaymentCreated (/home/rafiki/localenv/mock-account-servicing-entity/build/index.js:2200:11)
...
This is showing the mock ase cant find the wallet address in the handle outgoing payment create webhook handler and then it doesnt deposit liquidity and the payment doesnt get funded. I looked into the DB and saw that we had 2 Grace Franklin
wallet addresses. https://cloud-nine-wallet-backend/accounts/gfranklin
and https://localhost:3000/accounts/gfranklin
. Looks like the localhost:3000
one is getting created by the mock ase's wallet address not found handler when we ping here. In other words, you can change gfranklin
to anything in c9_wallet_address
and it works because it gets created if it doesnt exit. Then when the mock ASE looks up the wallet address, it expects this new one while the one specified in the performance test script is the original so it never gets funded.
Perhaps we can ping something else? Or nothing? http://cloud-nine-wallet-backend:3000/accounts/gfranklin
connects but creates a new record as well, and we cant reach by https://cloud-nine-wallet-backend/accounts/gfranklin
. Originally I noticed I could change the script to do const CLOUD_NINE_WALLET_ADDRESS = 'https://localhost:3000/accounts/gfranklin'
but this is just kinda hacking around the unintended wallet address creation.
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.
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 investigated this a little bit and I think it's a separate issue from there being different wallet addresses with the same name. I think when the webhook is generated it is populated with the database id of the wallet address, but the mock ASE passes that id into an instance of account-provider
from mock-account-service-lib
, which is seeded with different ids from what gets set in the database.
I get this same issue with the webhook handler not finding the wallet address coming up when I perform the Bruno peer-to-peer example, not sure how it got through when you tried it...
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.
LGtM 🚀
test/performance/README.md
Outdated
|
||
- [Running local playground for Rafiki](../../localenv/README.md) | ||
|
||
If the local environment isn't running it may be started |
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 you meant to finish this sentence. We should also mention how to start it with telemetry enabled
const c9WalletAddresses = JSON.parse(c9WalletAddressesRes.body).data | ||
.walletAddresses.edges | ||
const c9WalletAddress = c9WalletAddresses.find( | ||
(edge) => edge.node.url === CLOUD_NINE_WALLET_ADDRESS | ||
).node |
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 we could move this into the setup
function of the script, such that we don't need to fetch them every iteration. We should give a good error if the request to find the specific wallet address fails
f914afd
to
14ea6e5
Compare
// The following section contains configuration options for execution of this | ||
// test script in Grafana Cloud. | ||
// | ||
// See https://grafana.com/docs/grafana-cloud/k6/get-started/run-cloud-tests-from-the-cli/ | ||
// to learn about authoring and running k6 test scripts in Grafana k6 Cloud. | ||
// | ||
// cloud: { | ||
// // The ID of the project to which the test is assigned in the k6 Cloud UI. | ||
// // By default tests are executed in default project. | ||
// projectID: "", | ||
// // The name of the test in the k6 Cloud UI. | ||
// // Test runs with the same name will be grouped. | ||
// name: "script.js" | ||
// }, | ||
|
||
// Uncomment this section to enable the use of Browser API in your tests. | ||
// | ||
// See https://grafana.com/docs/k6/latest/using-k6-browser/running-browser-tests/ to learn more | ||
// about using Browser API in your test scripts. | ||
// | ||
// scenarios: { | ||
// // The scenario name appears in the result summary, tags, and so on. | ||
// // You can give the scenario any name, as long as each name in the script is unique. | ||
// ui: { | ||
// // Executor is a mandatory parameter for browser-based tests. | ||
// // Shared iterations in this case tells k6 to reuse VUs to execute iterations. | ||
// // | ||
// // See https://grafana.com/docs/k6/latest/using-k6/scenarios/executors/ for other executor types. | ||
// executor: 'shared-iterations', | ||
// options: { | ||
// browser: { | ||
// // This is a mandatory parameter that instructs k6 to launch and | ||
// // connect to a chromium-based browser, and use it to run UI-based | ||
// // tests. | ||
// type: 'chromium', | ||
// }, | ||
// }, | ||
// }, | ||
// } |
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 we can remove this comment, if more scenarios are needed we can access the docs then
metadata: { | ||
description: null, | ||
externalRef: null | ||
}, |
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.
metadata: { | |
description: null, | |
externalRef: null | |
}, |
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.
There doesn't seem to be a change here?
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 was just commenting we can remove metadata if it doesn't contain any info
The test can also be run inside of a Docker container on the same Docker network as the Local Playground: | ||
|
||
``` | ||
pnpm --filter performance run-tests-docker |
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 script is useful such that we don't need to set the host names?
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 took this from the description of the issue:
Since the local playground is within docker, we will need to run this script in docker as well, e.g.
docker run --rm -i grafana/k6 run - script.js
(Though not setting the host names is a benefit).
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.
🚀
Changes proposed in this pull request
Context
Fixes #2810.
The performance test script exercises the
createReceiver
,createQuote
, andcreateOutgoingPayment
GraphQL endpoints.Checklist
fixes #number