Ensure to wait a second before next tick()
#2465
Conversation
locust/runners.py
Outdated
| @@ -367,6 +366,9 @@ def shape_worker(self) -> None: | |||
| # of each load test shape stage. | |||
| self.start(user_count=user_count, spawn_rate=spawn_rate, user_classes=user_classes) | |||
| self.shape_last_tick = current_tick | |||
| shape_adjustment_time_ms = time.time() - shape_adjustment_start | |||
| if shape_adjustment_time_ms < 1: | |||
There was a problem hiding this comment.
<1 feels strange :)
Can you maybe change it to not use an if statement at all and instead do:
gevent.sleep(max(0, 1 - shape_adjustment_time_ms))
Also, a test case seems to be breaking...
There was a problem hiding this comment.
Yep, just noticed the test, sorry! I'll fix that. the max(..., 0) seems good, will adjust the commit then :)
There was a problem hiding this comment.
I haven't noticed that overriding current_tick leads to unwanted behavior. Fix was commited
locust/runners.py
Outdated
| else: | ||
| user_count, spawn_rate, user_classes = current_tick # type: ignore | ||
| shape_adjustment_start = time.time() | ||
| user_count, spawn_rate, user_classes = current_tick if len(current_tick) == 3 else (*current_tick, None) # type: ignore |
There was a problem hiding this comment.
I think this is less readable than the old way. Does it really need to change or is it unrelated to the issue you are trying to fix?
There was a problem hiding this comment.
You're right, it's not related. I wanted to make it more "pythonic" and readable with my previous change (where I modified current_tick when its length was 2), but eventually it indeed got not very clear :) I restored the code from master for this part. Also, I changed a bit the gevent.sleep usage, to call it once for entire tick() handling - seems more understable for me now
|
@cyberw I've also added a dedicated test case that doesn't base on |
|
nice!! |
|
@wiatrak2 is it possible that one of the test cases got a little flaky from this change? Perhaps it should be adjusted? https://github.com/locustio/locust/actions/runs/6919408511/job/18822743361?pr=2472 |
|
@wiatrak2 Can you have a look at the occasionally failing test case? While I do believe your change is good, it did break the test case and I dont have time to analyze why it is breaking, so I might have to revert it... |
|
@cyberw it happened in the worst possible moment - I'm on vacation and away from my computer until December 16th :( I won't be able to have a look before |
|
No worries! I can disable the tests for now (I dont think anything is actually broken, and if it is its a small issue) |
|
Yep, the change was not a sufficient one. If you can disable the test - let's do it. I will have a (very fresh) look once I'm back to the business ;) |
When developing a custom
TestShape, a developer needs to implementtick()method, that managesUsersspawning. As mentioned in the official documentation:Therefore, a developer may design a
TestShapewith a strong assumption that the number ofUserswill be adjusted with ~1 second interval. Unfortunately, the interval is indeed kept only when no adjustment inUsersnumber is made - hence it seems useless, as any interval is meaningful only during some changes. Here's an example of logs from Locust master when a customTestShapewas implemented and thetick()method is clearly triggered too often:the timestamps of log messages show, that 20
Userswere spawned within ~6 seconds, while the spawn rate is set to1.0. In fact I was able to achieve much bigger discrepancies between expected/declared and actual number of users.In this PR I provide a small adjustment in
Runner's code to ensure the ~1 second interval is maintained. Also a small code optimization in handling thecurrent_tickTuple was added to keep the file length almost unchanged :)