Fix active VU reporting by arrival-rate executors #2953
Conversation
This is a somewhat quick and dirty fix of #1977, where instead of changing the global active VUs when the VU is activated, we change it when the VU is actually running. This synchronizes it with the running count of the internal activeVUPool, and reports the correct value everywhere the ExecutionState is used (the real-time total active VU count while the test is running, in all outputs, etc.).
imiric
force-pushed
the
fix/arrival-rate-active-vus
branch
from
c148f61 to
6f6a4ef
Compare
Codecov Report
@@ Coverage Diff @@
## master #2953 +/- ##
==========================================
- Coverage 76.88% 76.74% -0.14%
==========================================
Files 226 227 +1
Lines 16908 16919 +11
==========================================
- Hits 13000 12985 -15
- Misses 3074 3086 +12
- Partials 834 848 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This behavior is unique to the arrival rate executors, which are the only ones that have an internal VU pool. They introduce an additional meaning to "active" VUs, which can actually remain dormant until they're "running" an iteration. This change simply synchronizes the "running" counter to the global "active" one, which makes the value reported globally more accurate. |
There was a problem hiding this comment.
LGTM! That seems way too easy 🤣 but I expect it was hard to make it this easy 👏
I did some benchmarking and it seems to have some performance impact in one of my scripts lying around, but it seemed really small. After some more tests - specifically with empty default functions, I am pretty sure that was due to some randomness in my original script, so it seems to work really well.
|
|
||
| runner := simpleRunner(func(ctx context.Context, _ *lib.State) error { | ||
| runMx.Lock() | ||
| assert.Equal(t, atomic.AddInt64(&running, 1), getCurrActiveVUs()) |
There was a problem hiding this comment.
nit: either the atomic here is pointless (we already have mutex) or you should also use it t o read the value in the checks at the end of the test
There was a problem hiding this comment.
Good point. I removed the atomic in 7162f77.
The reason I didn't use it at the end is because the test is already done at that point, so it should be safe to read it directly.
Not really, I was surprised as well, and thought there might be a catch I'm missing. 😅 It does have the side effect that the global VU counter is now "bursty" as the arrival rate VU counter is, so we might want to cache it slightly at least in the UI, since currently it's not very legible on terminals with high refresh rate. Thanks for confirming it doesn't impact performance. I was slightly concerned about that as well. 👍 |
|
Awesome, thats for constant arrival rate |
This is a somewhat quick and dirty fix of #1977, where instead of changing the global active VUs when the VU is activated, we change it when the VU is actually running. This synchronizes it with the running count of the internal
activeVUPool, and reports the correct value everywhere theExecutionStateis used (the real-time total active VU count while the test is running, in all outputs, etc.).