feature: add KeyLogWriter runtime option and envVar #2210
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2210 +/- ##
==========================================
+ Coverage 72.71% 72.80% +0.08%
==========================================
Files 184 184
Lines 14571 14706 +135
==========================================
+ Hits 10596 10706 +110
- Misses 3333 3354 +21
- Partials 642 646 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Hi @chtnnh, thanks for your contribution! Can you fix the linter and add a test, please? |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
This will close the file before anything has had time to write to it.
We either need to close it at some other time (there is currently no VU cleanup state, and I would argue that will be kind of hard to implement), or we can just not close it with a comment to actually figure out how to close it later on. Arguably in the fast majority of cases this file will stay open until just before k6 gets killed so 🤷
There was a problem hiding this comment.
While it will be nice to open this once and use it throughout, I think doing it in the run command will require way too many lib.Runner (the interface) changes that I am not particularly certain about and will just be one more thing this interface does (and it already is pretty huge IMO).
But this(the opening of the file) can definitely be moved to the creation of the js.Runner where we first get the RuntimeOptions and keep it as a field. But at least currently, looking at the code around all of this, there is no particular moment the js.Runner has no Stop method or some way to emulate it that will make the rest of k6 to wait for it to close the file after it is no longer necessary.
Alternatively, we have a context when we activate a VU and a known place where it(vu) no longer runs, but we haven't yet returned it, so the rest of the system will wait for us to ... for example close the file. Opening and closing a file on each activation though might not be great 🤷. Also this means that we are using 1 file descriptor per VU just for that ... again not great.
We can probably combo the two, so that each time a VU is activated it will create the file and then use it and if it's the last one to be deactivated it will close it 🤷 , but IMO this goes in the same (if not bigger) rabbit hole as the adding of more methods to the lib.Runner interface and IMO is completely out of scope.
I am also not certain that just because we are not running code, some (retry) logic in net/http (or something else as this should be used by all tls connections) will not just try to make a new connection and try to write to an already closed key log file.
So I am mostly for this to be moved in the New* methods returning the runner and having it in one place and just having a TODO+issue that we need to close this file. From my experiments, it works great even without closing it and the 1 more open file forever is probably the least problem around the runner ;)
There was a problem hiding this comment.
@mstoykov @na--
To echo my comment from my (duplicate as it turns out) solution: I think you may be overthinking this quite a bit :) just think of a use case: it is only to debug the comm with a few users at most - never to run at full load, because doing so will compromise the test infra. A few extra file handles on K6' side is nothing compared to the CAP portion (inspect streams, store/dump session) - IMO the only appropriate way to implement large scale capture, is by using an appropriately sized squid/nginx reverse proxy cluster with logging and post-processing.
| @@ -58,6 +58,7 @@ type RuntimeOptions struct { | |||
| NoThresholds null.Bool `json:"noThresholds"` | |||
| NoSummary null.Bool `json:"noSummary"` | |||
| SummaryExport null.String `json:"summaryExport"` | |||
| KeyWriter null.String `json:"keyWriter"` | |||
There was a problem hiding this comment.
| KeyWriter null.String `json:"keyWriter"` | |
| KeyWriter null.String `json:"-"` |
I don't think this is used anywhere, but still, it shouldn't be configurable from any place that is JSON ... or maybe it works with config.json, we need to test that 🤔
cc @na-- WDYT, should this even be configurable from there?
|
Hey @chtnnh 👋 we would love to send you a little k6 swag pack as per "thank you" for contributing to k6 this hacktoberfest. Please send your address details to floor[at]grafana.com and I'll take care it's shipped asap. |
|
|
|
Hi @chtnnh, sorry for the really slow response. We did have some internal discussions at it seems most other contributors want the file to be closed. This unfortunately likely will mean a lot more code :(. We are currently near the end of the development cycle for v0.37.0 and will stop merging PRs ... soon. There have been some changes (and more to come) that will make the "when should I close the keylog file?" question easier to answer. What I am trying to say is that I am going to try to work on (or at least look into) to make your required changes as easy and small as possible. Unfortunately this will have to wait for the next release and might not even land there, depending on priorities. But I haven't forgotten you :). Thanks for the contribution 🙇 |
@codebien if you could have a look that would be great!
Reference #1043