stats/cloud: stop sending cloud metrics when limit reached #1130
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1130 +/- ##
==========================================
+ Coverage 73.61% 73.62% +0.01%
==========================================
Files 144 144
Lines 10527 10547 +20
==========================================
+ Hits 7749 7765 +16
- Misses 2321 2325 +4
Partials 457 457
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
As I've written I don't think the way you are doing is ... good. I think you should stop collecting and pushing metric in general not just checking that you should no longer push metrics when it gets to it.
Also I don't see how this PR implements the other part of the #1074 mainly there being a way for this to stop the execution of the test. This probably should be with an option but still ...
cuonglm
force-pushed
the
fix/1074
branch
4 times, most recently
from
b57925e
to
8dc8de7
Compare
There was a problem hiding this comment.
Looks good apart from the unlikely panic ;)
Also are we going to leave the way to tell k6 that we won't write for a later commit ? I think that will definitely require an interface change and probably will be required for a release:
In most cases where people will be writing to cloud if they stop writing for whatever reason they will probably want to end the tests if there is no other output.
cuonglm
force-pushed
the
fix/1074
branch
2 times, most recently
from
76a1c63
to
d479867
Compare
There was a problem hiding this comment.
On top of the comments that I left:
Looking at the tests I don't see you testing, that we actually stop after the max writes ... if anything this require seems to indicate that the collector didn't get stopped ... which is expected because you are collecting metrics and than canceling the context, but you are nowhere sleeping in order for there to be at least 5 pushes.
The test is done with |
|
Okay .. I guess this is fine for the test ... Also .. I suppose the reason for this test to work at all without sleeps is that the generation of metrics takes long enough? So I would prefer if there is a sleep in one of two cycles that makes it impossible for the loop to end before the needed amount of pushes is done ... |
Good idea, will fix.
Let me check. |
There was a problem hiding this comment.
The current code is broken when aggregation is not enabled, i.e. when AggregationPeriod is 0. Try to run a simple script with k6 run -o cloud and see that k6 won't ever exit, you'd have to kill the process. The reason for that behavior is that the quit channel won't be closed...
I didn't follow the previous discussion closely and I am not certain why that quit channel is necessary, but the current implementation is broken.
FIxed it. |
Once the cloud test gets aborted by limit, we should not send metrics anymore. Close #1074
Filed in #1160 |
In #1130, we stop sending metrics to cloud when limit reached. However, after stop sending metrics, collector still collects data, causing the memory usage quickly increase. To fix it, do not collect data if stop sending metrics triggered. While at it, also remove some useless condition checking that slice length is greater than 0 before looping. Fixes #1187
* fix memory leak after stop sending metrics to cloud In #1130, we stop sending metrics to cloud when limit reached. However, after stop sending metrics, collector still collects data, causing the memory usage quickly increase. To fix it, do not collect data if stop sending metrics triggered. While at it, also remove some useless condition checking that slice length is greater than 0 before looping. Fixes #1187
* fix memory leak after stop sending metrics to cloud In #1130, we stop sending metrics to cloud when limit reached. However, after stop sending metrics, collector still collects data, causing the memory usage quickly increase. To fix it, do not collect data if stop sending metrics triggered. While at it, also remove some useless condition checking that slice length is greater than 0 before looping. Fixes #1187
Once the cloud test gets aborted by limit, we should not send metrics
anymore. The test and collector will still running, though.
Close #1074