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

feat: add switchboard artillery target #25

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

bchrobot
Copy link
Member

@bchrobot bchrobot commented Jun 16, 2023

Description

This adds Switchboard as a target along with one Artillery script for the sendMessage endpoint.

Motivation and Context

  • Blocked by politics-rewired/switchboard#378

@bchrobot bchrobot marked this pull request as ready for review July 12, 2023 02:37
@bchrobot bchrobot requested review from hiemanshu, lediur and ajohn25 July 12, 2023 02:38
Copy link

@ajohn25 ajohn25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun one to review 🥳

@@ -0,0 +1,5 @@
SWITCHBOARD_CLIENT_ID="00000000-0000-4000-0000-000000000000"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question (not important): Do you intentionally use 4000 in the middle 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, UUIDv4 always has a 4 at that position

environments:
staging:
target: "https://staging.switchboard.assemble.live"
phases:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Do we want to treat these like unit tests where these tests all need to pass before a production release occurs?

If failing tests are ok, considering you saw good performance up to 500 so far, you could add tests up to 1000? Our Bandwidth rate limit for the registered account is now set to 900, so that feels like a goal to shoot for even if we're not ready to get there yet.

Copy link

@lediur lediur Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of testing on a kind of log scale beyond our current external rate limits so we know our sending rate is artificially limited by policy rather than software performance.

So like Aashish mentioned I would set phases for like 100, 200, 400, 800, 1600, 3200 to start with, then doing a bit of a binary search to add new phases so we have a good sense of "max possible performance" within 100 req/s or so.

Then we can decide how to do a testing strategy once we know the software perf limits.

Copy link
Member Author

@bchrobot bchrobot Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Do we want to treat these like unit tests where these tests all need to pass before a production release occurs?

Treating stress test performance as part of release acceptance seems like a good idea to me but out of scope for this PR.

If failing tests are ok, considering you saw good performance up to 500 so far, you could add tests up to 1000? Our Bandwidth rate limit for the registered account is now set to 900, so that feels like a goal to shoot for even if we're not ready to get there yet.

This is currently blocked by stress test runner limitations. I encountered file descriptor quota exhaustion on Lambda (see 19709a5) and in attempting to fix that ended up down a rabbit hole debugging other AWS runner issues.

targets/switchboard/artillery-scripts/send-message.yaml Outdated Show resolved Hide resolved
targets/switchboard/artillery-scripts/send-message.yaml Outdated Show resolved Hide resolved
as: "messageCreatedAt"

# Give process-grey-route-message a chance to work
- think: 0.2
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧠

text: "An SMS from Artillery load test!"
tag: "v1|{{ messageId }}|{{ messageCreatedAt }}"

# Receive 'message-delivered' DLR
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Do we want some think time in btwn here before receiving the 2nd DLR or does it not really make a difference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make a difference and in practice we have actually seen the second DLR arrive first (that's why there's logic in Spoke for handling DLR looking at previous message status so we don't accidentally go from DELIVERED to SENT if the sequence gets mixed up I can't actually find this logic in Spoke codebase anymore 🤔).

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

Successfully merging this pull request may close these issues.

3 participants