allow specifying multiple collectors #624

jmccann · 2018-05-14T18:20:42Z

Taking a stab at #620

Example:

~ # k6 run --vus 10 --duration 5s --out json=test2.json --out json=test.json github.com/loadimpact/k6/samples/http_get.js

          /\      |‾‾|  /‾‾/  /‾/   
     /\  /  \     |  |_/  /  / /   
    /  \/    \    |      |  /  ‾‾\  
   /          \   |  |‾\  \ | (_) | 
  / __________ \  |__|  \__\ \___/ .io

  execution: local
     output: json=test2.json; json=test.json
     script: github.com/loadimpact/k6/samples/http_get.js
...

~ # ls
k6          test.json   test2.json
~ # diff test.json test2.json 
~ # head -n5 test.json && tail -n5 test.json 
{"type":"Metric","data":{"name":"http_reqs","type":"counter","contains":"default","tainted":null,"thresholds":[],"submetrics":null,"sub":{"name":"","parent":"","suffix":"","tags":null}},"metric":"http_reqs"}
{"type":"Point","data":{"time":"2018-05-14T18:18:54.431179649Z","value":1,"tags":{"group":"","method":"GET","name":"http://test.loadimpact.com","proto":"HTTP/1.1","status":"200","url":"http://test.loadimpact.com"}},"metric":"http_reqs"}
{"type":"Metric","data":{"name":"http_req_duration","type":"trend","contains":"time","tainted":null,"thresholds":[],"submetrics":null,"sub":{"name":"","parent":"","suffix":"","tags":null}},"metric":"http_req_duration"}
{"type":"Point","data":{"time":"2018-05-14T18:18:54.431179649Z","value":132.390419,"tags":{"group":"","method":"GET","name":"http://test.loadimpact.com","proto":"HTTP/1.1","status":"200","url":"http://test.loadimpact.com"}},"metric":"http_req_duration"}
{"type":"Metric","data":{"name":"http_req_blocked","type":"trend","contains":"time","tainted":null,"thresholds":[],"submetrics":null,"sub":{"name":"","parent":"","suffix":"","tags":null}},"metric":"http_req_blocked"}
{"type":"Point","data":{"time":"2018-05-14T18:18:59.060387819Z","value":1,"tags":null},"metric":"iterations"}
{"type":"Point","data":{"time":"2018-05-14T18:18:59.117344636Z","value":10,"tags":null},"metric":"vus"}
{"type":"Point","data":{"time":"2018-05-14T18:18:59.117344636Z","value":10,"tags":null},"metric":"vus_max"}
{"type":"Point","data":{"time":"2018-05-14T18:18:59.244079246Z","value":10,"tags":null},"metric":"vus"}
{"type":"Point","data":{"time":"2018-05-14T18:18:59.244079246Z","value":10,"tags":null},"metric":"vus_max"}

codecov-io · 2018-05-14T18:43:04Z

Codecov Report

Merging #624 into master will increase coverage by 0.52%.
The diff coverage is 51.85%.

@@            Coverage Diff             @@
##           master     #624      +/-   ##
==========================================
+ Coverage   64.51%   65.04%   +0.52%     
==========================================
  Files          98       98              
  Lines        7829     7838       +9     
==========================================
+ Hits         5051     5098      +47     
+ Misses       2488     2436      -52     
- Partials      290      304      +14

Impacted Files	Coverage Δ
cmd/run.go	`6.76% <0%> (-0.11%)`	⬇️
core/engine.go	`89.23% <100%> (+1.14%)`	⬆️
cmd/config.go	`50.74% <66.66%> (+13.24%)`	⬆️
cmd/common.go	`40% <0%> (+25.71%)`	⬆️
cmd/options.go	`63.44% <0%> (+25.8%)`	⬆️

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 ee5a9c3...f1dc9cf. Read the comment docs.

na-- · 2018-05-15T11:07:56Z

cmd/common.go

@@ -100,6 +100,25 @@ func getNullString(flags *pflag.FlagSet, key string) null.String {
 	return null.NewString(v, flags.Changed(key))
 }

+func getNullStrings(flags *pflag.FlagSet, key string) []null.String {


As I mentioned in the kafka PR, I don't think it's necessary it to have null.String slices.

Yep, I'll get this updated

na-- · 2018-05-15T11:08:45Z

cmd/config.go

-	Linger        null.Bool   `json:"linger" envconfig:"linger"`
-	NoUsageReport null.Bool   `json:"noUsageReport" envconfig:"no_usage_report"`
-	NoThresholds  null.Bool   `json:"noThresholds" envconfig:"no_thresholds"`
+	Out           []null.String `json:"out" envconfig:"out"`


See comment above about []null.String.

na-- · 2018-05-15T11:13:08Z

cmd/config.go

-		c.Out = cfg.Out
+	for _, o := range cfg.Out {
+		if o.Valid {
+			c.Out = append(c.Out, o)


This would make it so there's no way to actually override a configured collector from a lower level. If we (after fixing #587) specify a collector in the exported script options, there wouldn't be a way to actually override it from the CLI options, like we could do with all the other options.

na-- · 2018-05-15T11:46:28Z

cmd/run.go

@@ -125,19 +125,21 @@ a commandline interface for interacting with it.`,
 		// defaults in there, override with Runner-provided ones, then merge the CLI opts in
 		// on top to give them priority.
 		fmt.Fprintf(stdout, "%s options\r", initBar.String())
-		cliConf, err := getConfig(cmd.Flags())
+
+		defaultConf := Config{Options: r.GetOptions()}


Ah, unfortunately this would break some stuff 😒... We have to use the cliConf as the base from which to build the config (and also Apply() it last) because it both contains the default values for different options AND is the highest user-supplied config set in the configuration hierarchy...
For example if you try running the code as it is without specifying VUs anywhere, it won't run because the number of VUs would be 0.
I'm sorry you have to deal with this, we'll hopefully fix the configuration mess soon. I'm still collecting pain points, issues and unexpected behaviours, but I'll soon create a github issue that comprehensively describes the current problems with option and configuration handling. For the moment, to avoid breakage, please don't touch the sacred 💩 that is cliConf.Apply(fileConf).Apply(Config{Options: r.GetOptions()}).Apply(envConf).Apply(cliConf) 😒

na-- · 2018-05-15T11:51:26Z

core/engine.go

+	if len(e.Collectors) > 0 {
+		for _, collector := range e.Collectors {
+			collectorwg.Add(1)
+			go func(collector lib.Collector) {


I'm not sure we need goroutines here, see my comment about Collect() in the kafka PR.

So I didn't add it, I'm just using what was already there ... did you want me to work on removing it? Should that be in scope for this PR?

But I can play with it ... maybe it would help address the issue of why I switched from Run() to Collect() for sending the samples to Kafka that I was talking about in the Kafka PR.

Argh, my bad, I misread the diff and the collector method called, ignore my comment entirely. It's exactly how it's supposed to be and your changes are perfectly fine.

jmccann · 2018-05-15T18:49:04Z

I think I got everything addressed. Thanks!

na--

LGTM! @luizbafilho, please take a look at this as well

luizbafilho

LGTM

na-- requested changes May 15, 2018

View reviewed changes

jmccann added 3 commits May 15, 2018 11:27

allow specifying multiple collectors from cli (and config?)

f6898a7

[]null.String -> []string

a483544

revert conf setup

f1dc9cf

jmccann force-pushed the multi-output-pt2 branch from c536645 to f1dc9cf Compare May 15, 2018 16:27

na-- approved these changes May 16, 2018

View reviewed changes

na-- requested a review from luizbafilho May 16, 2018 11:02

luizbafilho approved these changes May 16, 2018

View reviewed changes

luizbafilho merged commit c0c680d into grafana:master May 16, 2018

jmccann deleted the multi-output-pt2 branch May 16, 2018 17:49

jmccann mentioned this pull request May 17, 2018

Ability to run multiple Collectors #620

Closed

na-- mentioned this pull request Aug 27, 2018

Move Output options in lib.Options #587

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow specifying multiple collectors #624

allow specifying multiple collectors #624

jmccann commented May 14, 2018

codecov-io commented May 14, 2018 •

edited

Loading

na-- May 15, 2018

jmccann May 15, 2018

na-- May 15, 2018

jmccann May 15, 2018

na-- May 15, 2018

na-- May 15, 2018

na-- May 15, 2018

jmccann May 15, 2018

na-- May 15, 2018

jmccann commented May 15, 2018

na-- left a comment

luizbafilho left a comment

allow specifying multiple collectors #624

allow specifying multiple collectors #624

Conversation

jmccann commented May 14, 2018

codecov-io commented May 14, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmccann commented May 15, 2018

na-- left a comment

Choose a reason for hiding this comment

luizbafilho left a comment

Choose a reason for hiding this comment

codecov-io commented May 14, 2018 •

edited

Loading