Support multiple and variable speed Decrease attack phases #450
Conversation
jeremyandrews
commented
Apr 25, 2022
•
jeremyandrews
commented
- Support multiple and variable speed decrease attack phases
jeremyandrews
changed the title
There was a problem hiding this comment.
Tested and I can confirm that it works as expected. I noticed two things:
- If I interrupted the load test with Ctrl+C the decrease after that moment isn't presented as a single step but as a lot of really short steps:
The overview in the CLI looks similar.
- Initially I made a mistake with the plan string formatting (swapped user and time in pairs). Test took a very long time and when I interrupted it I got a panic:
thread 'main' panicked at 'index out of bounds: the len is 3 but the index is 3',
.../goose/src/lib.rs:1545:52
Ideally we would want to catch such a mistake and warn the user. With the current format it would be hard to do that I suppose, but what if we tweak the formatting a bit? JSON maybe (e.g. [{users: 5, time: 30}, {users: 10, time: 30}, {users: 0, time: 10}])?
If that is not possible we should at least try to exit without panic when the test is interrupted so that user can understand the mistake from the overview that is printed at the end.
| } | ||
|
|
||
| // Record current users for users per second graph in HTML report. | ||
| if let Some(started) = self.started { | ||
| self.graph_data.record_users_per_second( | ||
| self.metrics.users, | ||
| goose_attack_run_state.active_users, |
There was a problem hiding this comment.
Other graphs look OK, but this one seems odd. Is this variable always storing the number of users at the time or the target number for the step?
There was a problem hiding this comment.
In my testing it appeared that self.started is reset back to 0 in some cases when a new plan step is started. I think that this isn't expected as we have self.step_started, which should be doing that?
There was a problem hiding this comment.
In my case it happened after the first Increase step when the Maintain step started. After that is seems OK.
The load test was started with --test-plan "5,10;5,20;20,10;20,30;100,10;20,10;20,30;0,20"
There was a problem hiding this comment.
Thank you! This bug has slipped past me a long time and confused me repeatedly: we were resetting start when we were checking if we needed to reset the metrics: but we shouldn't have been doing that when using a test plan. Fixed!
|
I've addressed the bugs you ran into, introducing a new I would not want to require someone to manually type out JSON as I find this complex (something I've done when testing APIs with Curl), but we could consider optionally supporting this if necessary. I do agree it's confusing however and easy to get backwards (I've done the same) -- I've fixed it so it doesn't panic anymore. I'd prefer to address this in a followup (we could support both the current style, and the json style. Or we could make it something like "u#,t#" where u is the prefix on users and t is the prefix on time, for example?) I also created #453 another followup to add testing -- it's rather non-trivial so I'd prefer to not hold up this landing. |
There was a problem hiding this comment.
I agree re JSON. And the issue is mitigated by the fact that user now sees the overview and can understand the mistake. When we will create the UI and we'll potentially allow to configure the test plan there the issues will be even less relevant.
I pushed one small change that marks the Cancelling phase as red on graphs.
