Add report summary CLI flag

[cuonglm]

Add --summary-export flag for "k6 run", the parameter is output file to
write report summary in JSON format.

Empty string means output is ignored.

Close #355

[codecov-io]

Codecov Report

Merging #1168 into master will increase coverage by <.01%.
The diff coverage is 69.73%.

@@            Coverage Diff             @@
##           master    #1168      +/-   ##
==========================================
+ Coverage   75.34%   75.34%   +<.01%     
==========================================
  Files         148      148              
  Lines       10763    10832      +69     
==========================================
+ Hits         8109     8161      +52     
- Misses       2189     2204      +15     
- Partials      465      467       +2

Impacted Files	Coverage Δ
cmd/run.go	9.27% <0%> (-0.44%)	⬇️
core/engine.go	93.77% <100%> (ø)	⬆️
cmd/config.go	78.1% <75%> (-0.16%)	⬇️
ui/summary.go	90.28% <95.83%> (+1.33%)	⬆️

---

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 82d6dd4...15552be. Read the comment docs.

[imiric]

The implementation LGTM, but this needs some tests.

And would this PR also close #1120?

[mstoykov]

IMHO this should wait for #1143 and definitely have tests (extend the one in #1143 ?) to test that everything works as expected :)
I haven't looked through the printing code and whether everything in the stdout summary is in the json one but given the lack of tests I would wait for them ;)

[na--]

as I mentioned in the inline comment, since this option is for making a JSON report of the end-of-test summary, in my opinion it's better to call it SummaryReport, not the current ReportSummary (which to me implies that we're making a summary of some report)

though now that I think about it, "report" itself is the ambiguous part here, maybe we can replace it? so... --summary-export? @cuonglm, @mstoykov , @imiric - thoughts?

[cuonglm]

as I mentioned in the inline comment, since this option is for making a JSON report of the end-of-test summary, in my opinion it's better to call it SummaryReport, not the current ReportSummary (which to me implies that we're making a summary of some report)

though now that I think about it, "report" itself is the ambiguous part here, maybe we can replace it? so... --summary-export? @cuonglm, @mstoykov , @imiric - thoughts?

/me +1 for --summary-export

[mstoykov]

I don't like "summary" to be honest :). --result-export though might be confused to be all the metrics ...

[na--]

"[End of test] summary" might not be the best name, but we're stuck with it anyway. It's referred so in the docs and the options --summary-trend-stats, --summary-time-unit, --no-summary exist... so, to be consistent, this option should feature summary in its name

[imiric]

To add another variation to consider 😄, how about --summary-output? "Summary" makes sense to me, since it's distinct from the actual metrics results and it is a summary of the results anyway, and "output" would be consistent with --out (perhaps this should be --output?), except one is the location of the metrics results and the other the location of the summary results. We could make it longer and more explicit with --results-summary-output and --results-output respectively.

[na--]

To add another variation to consider smile, how about --summary-output

👍, to me it seems slightly better than --summary-export, but it might be slightly confusing or conflicting with the rest of the "outputs", or with the desire to make this new flag independent from --no-summary above: #1168 (comment)

--results-summary-output and --results-output

As I mentioned in the previous comment, I feel like we should have summary somewhere in there

[mstoykov]

I still think that we can move the data under extra to the level above as there is zero value added from this and also probably will be less code ;)

[cuonglm]

Fixed

[mstoykov]

I don't think you need neither min nor max as well as probably most of the variables in the other cases of this switch

[cuonglm]

Fixed.

[mstoykov]

There is still no support for thresholds and by the looks of it- it's going to be pretty simple to add.
You need to check that the metric has Thresholds and if it does ... just add them possibly as map
of Threshold.Source to Threshold.LastFailed to the metric itself ... Like:

        "http_req_duration{url:http://httpbin.org/post}": {
            "avg": 117.039762276,
            "max": 228.920154,
            "med": 116.05152949999999,
            "min": 113.477511,
            "p(90)": 117.4600813,
            "p(95)": 118.38552544999999,
            "thresholds": {
                "max<1000": false
            }
        },

Although maybe it would be good idea to have a separate place for all the thresholds either in addition or just there ... cc @na--, @imiric ?

[cuonglm]

This will get overwritten at 5d558ee#diff-cc7288c8b0cb64e706ac188a9e524b7fR444 for non Trend metrics

Fixed

[cuonglm]

@na-- @mstoykov @imiric can you guys re-review this?

Hey guys, a gentle ping.

[imiric]

LGTM, just remember to cleanup/squash before merge.

[na--]

LGTM, though we might want to do some corner case tests when we merge it (different and nested groups, custom metrics, tagged thresholds, custom trend percentile columns, etc.). And yeah, squash the commits before merging.

[mstoykov]

LGTM, although I think that a few more tests will be a good idea, but definitely a lot of manual testing with different scripts is a must before release.

Also IMO we should be using Metric.Summary, possibly after changing it a bit.

[cuonglm]

LGTM, although I think that a few more tests will be a good idea, but definitely a lot of manual testing with different scripts is a must before release.

Also IMO we should be using Metric.Summary, possibly after changing it a bit.

Thanks, I don't know about it. Scratch the surface looking, m.Summary().Summary looks weird.