New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow specifying multiple collectors #624

Merged
merged 3 commits into from May 16, 2018

Conversation

Projects
None yet
4 participants
@jmccann
Contributor

jmccann commented May 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 14, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

codecov-io commented May 14, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

Show outdated Hide outdated cmd/common.go
Show outdated Hide outdated cmd/config.go
Show outdated Hide outdated cmd/config.go
Show outdated Hide outdated cmd/run.go
if len(e.Collectors) > 0 {
for _, collector := range e.Collectors {
collectorwg.Add(1)
go func(collector lib.Collector) {

This comment has been minimized.

@na--

na-- May 15, 2018

Member

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

@na--

na-- May 15, 2018

Member

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

This comment has been minimized.

@jmccann

jmccann May 15, 2018

Contributor

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.

@jmccann

jmccann May 15, 2018

Contributor

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.

This comment has been minimized.

@na--

na-- May 15, 2018

Member

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.

@na--

na-- May 15, 2018

Member

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

This comment has been minimized.

Show comment
Hide comment
@jmccann

jmccann May 15, 2018

Contributor

I think I got everything addressed. Thanks!

Contributor

jmccann commented May 15, 2018

I think I got everything addressed. Thanks!

@na--

na-- approved these changes May 16, 2018

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

@na-- na-- requested a review from luizbafilho May 16, 2018

@luizbafilho

LGTM

@luizbafilho luizbafilho merged commit c0c680d into loadimpact:master May 16, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment