Redirect console to a file. #833

cheesedosa · 2018-11-01T09:54:32Z

WIP for #831

CLAassistant · 2018-11-01T09:54:39Z

All committers have signed the CLA.

codecov-io · 2018-11-01T10:57:17Z

Codecov Report

Merging #833 into master will decrease coverage by 0.08%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
- Coverage   69.82%   69.74%   -0.09%     
==========================================
  Files         112      112              
  Lines        8776     8804      +28     
==========================================
+ Hits         6128     6140      +12     
- Misses       2251     2262      +11     
- Partials      397      402       +5

Impacted Files	Coverage Δ
cmd/archive.go	`19.44% <0%> (-1.15%)`	⬇️
cmd/run.go	`6.38% <0%> (-0.03%)`	⬇️
lib/runner.go	`67.64% <0%> (-2.05%)`	⬇️
cmd/cloud.go	`5.12% <0%> (-0.09%)`	⬇️
lib/options.go	`91.35% <0%> (-1.15%)`	⬇️
cmd/options.go	`59.48% <33.33%> (-1.43%)`	⬇️
js/runner.go	`85.54% <81.81%> (-0.41%)`	⬇️
js/console.go	`94.87% <86.66%> (-5.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e93f26...b8c8aee. Read the comment docs.

na--

Thanks for tackling this and sorry for the delay in reviewing! Here's my initial impression of the changes.

cmd/options.go

na-- · 2018-11-15T08:46:12Z

lib/options.go

@@ -268,6 +268,9 @@ type Options struct {

 	// Discard Http Responses Body
 	DiscardResponseBodies null.Bool `json:"discardResponseBodies" envconfig:"discard_response_bodies"`
+
+	// Redirect console logging to a file
+	RedirectConsoleToFile null.String `json:"redirectConsoleToFile" envconfig:"redirect_console_to_file"`


I'm not sure if this option should actually be accessible from the exported in-script options. It feels like we should disable it (maybe a json:"-" struct tag here will be enough?) for the same security and UX reasons (mostly with regards to distributed and cloud execution) we don't allow scripts to write stuff in files directly.

I see this functionality as something that, for the moment at least, would be meant primarily to help with local debugging of k6 scripts. We can probably extend it in the future, when we properly implement distributed execution inside of k6 (#140), but for now I think we should only allow configuring this via environment variables or CLI flags.

mstoykov · 2018-11-15T09:26:49Z

js/console.go

+	f, err := fs.OpenFile(filepath, os.O_APPEND|os.O_WRONLY, os.ModeAppend)
+	if err != nil {
+		if os.IsNotExist(err) {
+			if f, err = fs.Create(filepath); err != nil {


I would prefer if you use os.O_CREATE above and change the file permissions to 0644

So, this may take some time to resolve. os.O_CREATE doesn't work with afero.CacheOnReadFs that we use as the base FS for the runner. I have raised issue spf13/afero#193 to track this.

mstoykov · 2019-01-11T08:01:47Z

@cheesedosa ping! Hi I would like if we could, merge this. If you can't or don't want to do the changes we required I will be happy to do it myself :)

cheesedosa · 2019-01-11T08:13:19Z

@mstoykov , I am sorry, I totally forgot about this PR 😞
I'll work on this over the weekend and make the changes.
Thanks!

mstoykov · 2019-01-15T08:56:51Z

After another look and actually trying the code I realized two things:

I don't know if you have tried it but it doesn't work for me when I try it for real, and not in running the tests. This looks to be because the parsing of the cli flags is after the initializing of the js,Runner.
We actually don't want to use afero.FS in this case. The primary idea of afero.FS in the runner is to sandbox all disk operations so that it is actually usable in a secure way in the cloud execution and easy to test. The idea of the redirect to disk is specifically the output to go to a file on the disk (at this point atleast). For example running your code with an archive instead of an test file will try to write create a file inside a memory only fs which is definitely not what we want.

Because of all this I would like if this redone so that

We open a file on the disk directly with os. Maybe before making the js.Runner ? Maybe not. Maybe the whole Console object should be given to the js.Runner. You will have to see how it will work best.
This definitely should not be changeable through the script or through the metadata.json inside the archive.tar. Actually if archive.tar is ran with k6 run it should be possible to redirect it output as well.

cheesedosa · 2019-01-15T11:01:10Z

After another look and actually trying the code I realized two things:

I don't know if you have tried it but it doesn't work for me when I try it for real, and not in running the tests. This looks to be because the parsing of the cli flags is after the initializing of the js,Runner.

We actually don't want to use afero.FS in this case. The primary idea of afero.FS in the runner is to sandbox all disk operations so that it is actually usable in a secure way in the cloud execution and easy to test. The idea of the redirect to disk is specifically the output to go to a file on the disk (at this point atleast). For example running your code with an archive instead of an test file will try to write create a file inside a memory only fs which is definitely not what we want.

Because of all this I would like if this redone so that

We open a file on the disk directly with os. Maybe before making the js.Runner ? Maybe not. Maybe the whole Console object should be given to the js.Runner. You will have to see how it will work best.

This definitely should not be changeable through the script or through the metadata.json inside the archive.tar. Actually if archive.tar is ran with k6 run it should be possible to redirect it output as well.

Yes, it was because the Options (from cli/env) are set after the js.Runner has been initialised. This requires us to re-init the console in-case we have console-output flag set to a file-based console.

Currently, in k6 each VU gets its own Console. I have changed this to have each VU spawned from the Runner to use the common Runner's console. As in-case of a file-based console, each VU opening a handle to the same file would be excessive especially when we have many VUs.

mstoykov

Thanks again for this PR!
This is better but as I have said I think we can do everything in SetOptions instead of adding new Methods to the interface. I tested it a little bit and everything seems to be working correctly so far though :).
Can you as well move the update of logrus before actually using it and having just two commits like:

Updating logrus
Implementing the console redirect
So that it looks cleaner in the history.

js/runner.go

js/console.go

`console-output` attaches a new file based console to the VUs. This allows users to redirect all their `console.log/warn/etc.` to the given file. This prevents the stdout from getting cluttered with debug statements that may be printed in the test `.js` files.

na--

LGTM, though as a future extension to this, I'm wondering if it would be worth it to support outputting the console to a unix pipe or to something like /dev/null?

cheesedosa · 2019-01-18T07:34:56Z

LGTM, though as a future extension to this, I'm wondering if it would be worth it to support outputting the console to a unix pipe or to something like /dev/null?

Since these are Unix files, passing /dev/stdout or /dev/null to console-output should just work.

na-- reviewed Nov 15, 2018

View reviewed changes

mstoykov reviewed Nov 15, 2018

View reviewed changes

mstoykov mentioned this pull request Jan 15, 2019

Unexpected output when using --quiet #782

Closed

cheesedosa force-pushed the redirect-console branch from d88179f to 65afa75 Compare January 15, 2019 10:53

mstoykov requested changes Jan 16, 2019

View reviewed changes

js/runner.go Outdated Show resolved Hide resolved

js/console.go Outdated Show resolved Hide resolved

Vendor new logrus which includes SetOutput().

76f07f3

cheesedosa force-pushed the redirect-console branch from 65afa75 to f6f29e9 Compare January 16, 2019 14:14

cheesedosa force-pushed the redirect-console branch from f6f29e9 to b8c8aee Compare January 16, 2019 14:45

mstoykov approved these changes Jan 17, 2019

View reviewed changes

na-- approved these changes Jan 18, 2019

View reviewed changes

mstoykov merged commit a018c78 into grafana:master Jan 18, 2019

na-- mentioned this pull request Jan 28, 2020

Proposal for adding http_req_errors metric #1311

Closed

mstoykov mentioned this pull request Mar 26, 2020

console.log in the init context doesn't respect --console-output #1131

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redirect console to a file. #833

Redirect console to a file. #833

cheesedosa commented Nov 1, 2018

CLAassistant commented Nov 1, 2018 •

edited

codecov-io commented Nov 1, 2018 •

edited

na-- left a comment

na-- Nov 15, 2018

mstoykov Nov 15, 2018

cheesedosa Jan 11, 2019

mstoykov commented Jan 11, 2019 •

edited

cheesedosa commented Jan 11, 2019

mstoykov commented Jan 15, 2019 •

edited

cheesedosa commented Jan 15, 2019

mstoykov left a comment

na-- left a comment

cheesedosa commented Jan 18, 2019

Redirect console to a file. #833

Redirect console to a file. #833

Conversation

cheesedosa commented Nov 1, 2018

CLAassistant commented Nov 1, 2018 • edited

codecov-io commented Nov 1, 2018 • edited

Codecov Report

na-- left a comment

Choose a reason for hiding this comment

na-- Nov 15, 2018

Choose a reason for hiding this comment

mstoykov Nov 15, 2018

Choose a reason for hiding this comment

cheesedosa Jan 11, 2019

Choose a reason for hiding this comment

mstoykov commented Jan 11, 2019 • edited

cheesedosa commented Jan 11, 2019

mstoykov commented Jan 15, 2019 • edited

cheesedosa commented Jan 15, 2019

mstoykov left a comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

cheesedosa commented Jan 18, 2019

CLAassistant commented Nov 1, 2018 •

edited

codecov-io commented Nov 1, 2018 •

edited

mstoykov commented Jan 11, 2019 •

edited

mstoykov commented Jan 15, 2019 •

edited