Repository navigation
test: add loadtest with normally distributed server delay #319
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: master
Are you sure you want to change the base?
Changes from all commits
1d0f7a7
3c68112
b6ed943
027d34b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,29 @@ create table run ( | |
| last_failure_error text | ||
| ); | ||
|
|
||
| -- random delay in ms from a normal distribution with the given mean and standard deviation. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. idk about everyone else but I'm not a huge fan of putting performance testing scripts into CI before merge. I feel like CI should be only for hard failures: testing internal logic, thread safety issues, memory safety stuff, etc. Maybe I'm totally off base here but just wondering what everyone else thinks.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR doesn't enable or add load test that will run in CI with this feature. But I share same view that for CI performance/benchmark should not be running.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I was not criticising this PR in particular, just using it as an opportunity to open discussion. I do think performance testing is important and should be done, just not sure when/how. Maybe on every commit on merge so u can look at the exact commit a regression occurred? Idk
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this PR, but continuous benchmarking has been very helpful to ensure correctness. At some point we got random timeouts during big batches and CI detects those (request failures at 0) plus it shows that CPU usage should be low. The runs are pretty stable too (see CI results, all the batches are completed in < 1 min and that has been maintained) It's yet incomplete though, it should fail when
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just checked the loadtest and there's a regression #321, this proves it's useful and should keep running on PRs IMO.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A per-commit performance regression suite is indeed valuable. What we also need are graphs that show performance over time which is easier to spot visually than visit each commit's results as Steve did above. Also a little more sophisticated statistical analysis will help gauge the trends better as currently we only have raw numbers. Ideally we run the benchmarks on non-GH runners to avoid noisy-neighbour problems. I ran the CI multiple times on the master branch and there is a noise of at least 2%, this run's max CPU was 13%, while this one was at 11%. I also suspect there's a regression but it becomes harder to compare these small changes on GH runners. Maybe have dedicated machines for benchmarking?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The noise is worse than 2%. Another example of two runs on the same commit:
This has a variation of 5% points in max CPU, which is not great to compare performance. Also we should be comparing the mean and standard deviation to meaningfully compare two runs, but with the current runs this is what we have.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest that we move CI discussion and tradeoffs to some other channel so we have clear picture what would be acceptable behavior. |
||
| -- Uses random_normal(), which is only available on PostgreSQL 16+. | ||
| -- Negative results are clamped to 0 since a delay can't be negative. | ||
| create or replace function random_delay_ms(mean_ms int, sd_ms int) returns int as $$ | ||
| begin | ||
| if current_setting('server_version_num')::int < 160000 then | ||
| raise exception 'function `random_normal` requires PostgreSQL 16 or newer, running %', current_setting('server_version'); | ||
| end if; | ||
| return greatest(0, round(random_normal(mean_ms, sd_ms)))::int; | ||
| end; | ||
| $$ language plpgsql volatile; | ||
|
|
||
| -- loadtest using many gets, used to be called `repro_timeouts` | ||
| create or replace procedure wait_for_many_gets(number_of_requests int default 10000, url text default 'http://localhost:8080') as $$ | ||
| -- | ||
| -- when delay_ms is given, each request goes to the nginx /delay endpoint with a server delay drawn | ||
| -- from a normal distribution with mean delay_ms and standard deviation delay_sd_ms (clamped | ||
| -- at 0), to simulate the latency of real servers | ||
| create or replace procedure wait_for_many_gets( | ||
| number_of_requests int default 10000, | ||
| url text default 'http://localhost:8080', | ||
| delay_ms int default null, | ||
| delay_sd_ms int default 0 | ||
| ) as $$ | ||
| declare | ||
| last_id bigint; | ||
| first_time timestamptz; | ||
|
|
@@ -22,7 +43,11 @@ begin | |
|
|
||
| with do_requests as ( | ||
| select | ||
| net.http_get(url) as id | ||
| net.http_get( | ||
| case when delay_ms is null then url | ||
| else url || '/delay?s=' || round(random_delay_ms(delay_ms, delay_sd_ms) / 1000.0, 3) | ||
|
imor marked this conversation as resolved.
|
||
| end | ||
| ) as id | ||
| from generate_series (1, number_of_requests) x | ||
| ) | ||
| select id, clock_timestamp() into last_id, first_time from do_requests offset number_of_requests - 1; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.