Repository navigation
Conversation
Add a /delay?s=<seconds> nginx endpoint, where <seconds> can be
fractional (e.g. /delay?s=0.087), and let wait_for_many_gets() send
each request there with a delay drawn from a normal distribution, so
the loadtest can simulate the latency of "normal" servers instead of an
instant local reply:
net-loadtest 10000 200 100 30 # mean 100ms, stddev 30ms
The delay is generated by random_delay_ms() using the Box-Muller
transform (written with AI assistance).
Also remove the unused /slow-reply and /really-slow-reply locations,
superseded by /delay.
|
This looks interesting. @mkaruza Could you clarify what's the motivation here? Is it just for removing |
I was looking around why we have / how extension will behave if we remove 1 sec sleep after batch is processed. Wanted to run test that simulate "real" world scenario where i can simulate network delay. Other endpoints are removed because they are not used and they can be used with this one. |
| last_failure_error text | ||
| ); | ||
|
|
||
| -- random delay in ms from a normal distribution with the given mean and standard deviation. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
idk about everyone else but I'm not a huge fan of putting performance testing scripts into CI before merge
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 time_taken and CPU usage surpasses a threshold.
There was a problem hiding this comment.
Just checked the loadtest and there's a regression #321, this proves it's useful and should keep running on PRs IMO.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The noise is worse than 2%. Another example of two runs on the same commit:
- This run had the max CPU at around 7% with one outlier at 9%.
- This run had the max CPU at around 12% with a couple of them on 13%.
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.
There was a problem hiding this comment.
I would suggest that we move CI discussion and tradeoffs to some other channel so we have clear picture what would be acceptable behavior.
This branch has not been deployed
4 participants
Add a /delay?s= nginx endpoint, where can be fractional (e.g. /delay?s=0.087),
and let wait_for_many_gets() send each request there with a delay drawn from a normal distribution,
so the loadtest can simulate the latency of "normal" servers instead of an instant local reply:
The delay is generated by random_delay_ms() using the Box-Muller transform (written with AI assistance).
Also remove the unused /slow-reply and /really-slow-reply locations, superseded by /delay.