Aggregated metrics in JSON format #274

jpallari · 2017-07-04T08:20:29Z

Changes summary

Added a new command line parameter report: the file where the aggregated metrics are written to in JSON format.

Related issues

Resolves #271

liclac · 2017-07-04T16:17:05Z

run.go

@@ -69,8 +70,13 @@ const (
 	CollectorJSON     = "json"
 	CollectorInfluxDB = "influxdb"
 	CollectorCloud    = "cloud"
+
+	SummaryFormatText = SummaryFormat(iota)


Consistency: else where we use the form SummaryFormatText SummaryFormat = iota.

liclac · 2017-07-04T16:19:33Z

run.go

@@ -588,60 +627,131 @@ loop:
 	}
 	fmt.Fprintf(color.Output, "\n")

+	// Print summary
+	summaryError := printSummary(fs, summaryFile, summaryFormat, engine, atTime)


Just call this err; I think this can be shortened to if err := printSummary(...); err != nil without the line length getting ridiculous too?

liclac · 2017-07-04T16:24:04Z

run.go

+	return nil
+}
+
+func printSummary(fs afero.Fs, filename string, summaryFormat SummaryFormat, engine *core.Engine, atTime time.Duration) error {


I don't like that this does two things.

The cyclomatic complexity of actionRun is ridiculous, but it's something I'm going to do something about as soon as I can (we need proper packages!). Just have it call a writeSummary() function that writes to the file if one is given instead.

liclac · 2017-07-04T16:25:51Z

run.go

@@ -150,6 +156,17 @@ var commandRun = cli.Command{
 			Usage:  "output metrics to an external data store (format: type=uri)",
 			EnvVar: "K6_OUT",
 		},
+		cli.StringFlag{
+			Name: "summary-file",


I like the name summary or report better here; also give it a shorthand flag!

Any suggestions on what letter to use for the shorthand flag?

-s or -r works; this is so useful that it deserves a flag imo.

liclac · 2017-07-04T16:26:19Z

run.go

@@ -150,6 +156,17 @@ var commandRun = cli.Command{
 			Usage:  "output metrics to an external data store (format: type=uri)",
 			EnvVar: "K6_OUT",
 		},
+		cli.StringFlag{
+			Name: "summary-file",
+			Usage: "file where the summary is written (default: standard output)",


Make it default to - instead of writing it here.

liclac · 2017-07-04T16:27:15Z

run.go

+			Name: "summary-format",
+			Usage: "file format for summary, one of: text, json",
+			EnvVar: "K6_SUMMARY_FILE",
+			Value: "text",


Let's default to JSON if a summary file is given instead of making it mutually exclusive with the text summary like this. Or YAML, if you prefer that.

liclac · 2017-07-04T16:29:35Z

stats/stats.go

+func (m *Metric) Summary() *Summary {
+	return &Summary{
+		Metric: m,
+		Samples: m.Sink.Format(),


This field name is misleading imo.

Any suggestions on the name?

Summary maybe?

Also: for consistency, all field names should have camelCase names when serialized to/from JSON.

liclac · 2017-07-04T16:30:13Z

Those are some linter errors from CircleCI too.

jpallari · 2017-07-04T16:47:09Z

Thanks for the review. I'll address your points in full when I get the chance.

There's one thing I didn't ask about earlier. These changes assume that you always want to output the summary in either text or JSON format to either stdout or a file. Is this style OK, or would the preferred style be to always print the text summary to stdout, and optionally write the summary as JSON to a file?

liclac · 2017-07-04T17:21:39Z

Always print the summary to stdout, optionally write to a file.

jpallari · 2017-07-08T18:55:36Z

I think the latest build failure is unrelated. I couldn't reproduce it locally. Is it possible to retrigger the build?

liclac · 2017-07-31T21:35:43Z

Could you try merging from master?

liclac · 2017-10-03T18:42:36Z

run.go

@@ -24,6 +24,7 @@ import (
 	"archive/tar"
 	"bytes"
 	"context"
+	jsonenc "encoding/json"


Please don't rename this import, adapt your variable names instead. It's too ubiquitous and it'd be inconsistent with what we're doing in other places.

The reason it's renamed is because there's already an import to something else named json: github.com/loadimpact/k6/stats/json

Ah, my bad.

liclac · 2017-10-03T22:33:27Z

Really sorry for being so slow on this, I somehow missed that you pushed those commits until I went back through some old issues. Please merge with master again, right now your branch only seems to be failing to build due to not having 72fa552.

- Mutexes don't have any sensible JSON serialization - Attempting to serialize a parent node will lead to infinite recursion.

Instead of choosing to print out the summary in either a human readable format or in JSON, always print out to stdout, and optionally write the JSON summary to a file.

liclac · 2017-11-21T15:59:53Z

Heads up: this was broken in the CLI rewrite (#110) and will be skipping v0.18.0, but I'll do my best to get it back into the next version.

liclac suggested changes Jul 4, 2017

View reviewed changes

jpallari force-pushed the feature/json-summary branch from 38130ba to 34a03f1 Compare July 7, 2017 07:48

jpallari force-pushed the feature/json-summary branch from 34a03f1 to 04159b6 Compare July 10, 2017 08:01

jpallari force-pushed the feature/json-summary branch 2 times, most recently from 22c357e to 555d6da Compare August 1, 2017 07:24

liclac suggested changes Oct 3, 2017

View reviewed changes

Jaakko Pallari and others added 6 commits October 4, 2017 19:01

Refactor result printing

ea7dbe9

File for writing summary

a5955a1

Skip JSON serialization for unserializable fields

8fdedcc

- Mutexes don't have any sensible JSON serialization - Attempting to serialize a parent node will lead to infinite recursion.

JSON summary

1793bc2

Always print summary in a human readable format

e079ab6

Instead of choosing to print out the summary in either a human readable format or in JSON, always print out to stdout, and optionally write the JSON summary to a file.

Fix gometalinter warnings

d98ba72

jpallari force-pushed the feature/json-summary branch from 555d6da to d98ba72 Compare October 4, 2017 18:01

liclac approved these changes Oct 4, 2017

View reviewed changes

liclac merged commit 87423b7 into grafana:master Oct 4, 2017

jpallari deleted the feature/json-summary branch October 5, 2017 07:35

ppcano mentioned this pull request Oct 31, 2017

JSON -report does not work. #355

Closed

ppcano mentioned this pull request Nov 20, 2017

How to access built-in metrics after the test completion #351

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregated metrics in JSON format #274

Aggregated metrics in JSON format #274

jpallari commented Jul 4, 2017 •

edited

Loading

liclac Jul 4, 2017

liclac Jul 4, 2017

liclac Jul 4, 2017

liclac Jul 4, 2017

jpallari Jul 4, 2017

liclac Jul 4, 2017

liclac Jul 4, 2017

liclac Jul 4, 2017

liclac Jul 4, 2017

jpallari Jul 4, 2017

liclac Jul 4, 2017

liclac commented Jul 4, 2017

jpallari commented Jul 4, 2017

liclac commented Jul 4, 2017

jpallari commented Jul 8, 2017 •

edited

Loading

liclac commented Jul 31, 2017

liclac Oct 3, 2017

jpallari Oct 4, 2017 •

edited

Loading

liclac Oct 4, 2017

liclac commented Oct 3, 2017

liclac commented Nov 21, 2017

Aggregated metrics in JSON format #274

Aggregated metrics in JSON format #274

Conversation

jpallari commented Jul 4, 2017 • edited Loading

Changes summary

Related issues

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liclac commented Jul 4, 2017

jpallari commented Jul 4, 2017

liclac commented Jul 4, 2017

jpallari commented Jul 8, 2017 • edited Loading

liclac commented Jul 31, 2017

Choose a reason for hiding this comment

jpallari Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liclac commented Oct 3, 2017

liclac commented Nov 21, 2017

jpallari commented Jul 4, 2017 •

edited

Loading

jpallari commented Jul 8, 2017 •

edited

Loading

jpallari Oct 4, 2017 •

edited

Loading