dump: Rework command to encode #318

xla · 2018-07-14T11:42:48Z

In order to give the command more utility and play well with pipeline idea of the vegeta toolchain we decided per discussion[0] to change the semantics of dump. We offer now a general encode command which can transform any supported input encoding to any supported output encoding. We anticipate that in conjunction with advocated auxiliary tools, like jq, a wide range of use-cases can be covered without extending vegeta itself.

[0] #302 (comment)

Closes #302

Background

Checklist

Git commit messages conform to community standards.
Each Git commit represents meaningful milestones or atomic units of work.
~~- [ ] Changed or added code is covered by appropriate tests.~~

golangci · 2018-07-14T11:43:02Z

encode.go

+		fs.PrintDefaults()
+	}
+
+	return command{


github.com/tsenart/vegeta.command composite literal uses unkeyed fields

Ugh, OK, let's comply. Other places are in transgression too.

golangci · 2018-07-14T11:43:02Z

encode.go

+	return command{
+		fs,
+		func(args []string) error {
+			fs.Parse(args)


Error return value of fs.Parse is not checked

Since we introduced @golangci, let's make it happy: _ = fs.Parse()

tsenart · 2018-07-14T14:52:39Z

README.md

-      Dumper [json, csv] (default "json")
-  -inputs string
-      Input files (comma separated) (default "stdin")
+encode command:


This listing of command's flags in vegeta -h was alright when commands only took flags but no positional arguments. Since this is changing, I believe we need to re-evaluate.

Either we display the full help out put of each sub command here, or we only display a list of sub command names, and users will have to defer to vegeta subcmd -h to read more.

What's your take?

I will implement this in a follow up PR.

tsenart · 2018-07-14T14:56:52Z

README.md

-#### `-output`
-Specifies the output file to which the dump will be written to.
+#### `-from`
+Specifies the decoding format of the inputs.


I think it'd be more correct to say "the encoding format of the inputs".

tsenart · 2018-07-14T14:58:57Z

README.md

+encoding](https://golang.org/pkg/encoding/gob).
+
+##### `json`
+Decodes/encodes results as JSON objects.


To be correct, it's JSONLines: http://jsonlines.org/

tsenart · 2018-07-14T15:00:21Z

encode.go

+
+func encodeCmd() command {
+	var (
+		fs     = flag.NewFlagSet("encode", flag.ExitOnError)


This should be flag.NewFlagSet("vegeta encode", ...)

Why? vegeta is already the main namespace of the binary.

Consistency with the other sub commands. Also, the different flag sets aren't directly associated in any way. This command could potentially work by itself if the parent didn't exist.

tsenart · 2018-07-14T15:01:03Z

encode.go

+		fs     = flag.NewFlagSet("encode", flag.ExitOnError)
+		from   = fs.String("from", "gob", "Input decoding [csv, gob, json]")
+		to     = fs.String("to", "json", "Output encoding [csv, gob, json]")
+		output = fs.String("output", "", "Output file")


The default output should be stdout

This doesn't add up really. We expect output to be a filename, if we expect stdout as ivalue it is inconsistent. They don't have the same meaning or refer to the same thing. Arguably we shoul document the default case better.

We expect output to be a filename.

Sure, and if you so wished you should be able to pass /dev/stdout to it and it'd just work. stdout is in this case a symbolic name that resolves to that path in practice.

Plus, it documents the behaviour, from the user perspective, it makes it explicit in the output of vegeta encode -h that the output will be to stdout, instead of that being implicit.

tsenart · 2018-07-14T15:11:29Z

encode.go

+		decFn = vegeta.NewCSVDecoder
+	case encodingJSON:
+		decFn = vegeta.NewJSONDecoder
+	default:


Do we need to rely on the default case to play nice with the pipes as per your comment? The default "gob" could come from the flags outside this function.

Good point, let's error in if we don't know the given encoding.

tsenart · 2018-07-14T15:16:29Z

encode.go

+
+	decs := []vegeta.Decoder{}
+
+	if len(inputs) > 0 {


We have some simpler code to handle this already. i.e. https://github.com/tsenart/vegeta/blob/master/report.go#L33

Because the file function returns os.Stdin for the string stdin. This block of code can become:

srcs := make([]vegeta.Decoder, len(files)) for i, f := range inputs { in, err := file(f, false) if err != nil { return err } defer in.Close() srcs[i] = vegeta.NewDecoder(in) } dec := vegeta.NewRoundRobinDecoder(srcs...)

I find the file function an anti-pattern and don't really understand the gains. os.Create and os.Open are so much more expresisve, a fact hidden behind an opaque boolean. Having string literals to refer to the the input and output file handles is unnecesssary indirection imho.

Yeah, file is slightly conflated. We can take out the file creation part from it and use os.Create directly where needed, thus scoping the symbolic resolution to opening files only, not creating (since you don't create stdout or stdin).

func open(file string) (*os.File, error) { switch file { case "stdout": return os.Stdout, nil case "stdin": return os.Stdin, nil default: return os.Open(file) }

tsenart · 2018-07-14T15:18:08Z

encode.go

+		decs = append(decs, decFn(os.Stdin))
+	}
+
+	in := vegeta.NewRoundRobinDecoder(decs...)


I am wondering what makes most sense to do. Shall we read results in a round-robin fashion and re-encode each out as they come or should we instead read all of them, sort them and then write them out. Perhaps this should be a flag.

What is the upside of the sorting? And sorting per input, over all inputs, by what criteria?

I think we can delegate this to external tools, indeed.

cat results.bin | vegeta encode | jq -s 'sort_by(.timestamp)'

tsenart · 2018-07-14T15:19:04Z

encode.go

+
+	var out vegeta.Encoder
+
+	if output != "" {


output should never be empty. The default stdout ought to be set in the flags. Then you can reuse the file function as in the report command.

Additionally, if you open the output first and then validate the to value, you don't need encoderFunc.

var enc vegeta.Encoder switch to { case encodingCSV: enc = vegeta.NewCSVEncoder(out) case encodingJSON: enc = vegeta.NewJSONEncoder(out) case encodingGob: enc = vegeta.NewEncoder(out) }

To your first comment see #318 (comment)

To the second comment I usually try to check all parameters before acquiring system resources (fds, conns, etc.).

To the second comment I usually try to check all parameters before acquiring system resources (fds, conns, etc.).

What does it gain us here?

tsenart · 2018-07-14T15:23:09Z

encode.go

+		out = encFn(os.Stdout)
+	}
+
+	for {


We need to handle signals gracefully as in the report command.

Will extend!

golangci · 2018-08-18T16:14:14Z

encode.go

+	}
+
+	return command{fs, func(args []string) error {
+		fs.Parse(args)


Error return value of fs.Parse is not checked

In order to give the command more utility and play well with pipeline idea of the vegeta toolchain we decided per discussion[0] to change the semantics of dump. We offer now a general encode command which can transofrm any input encoding to any of the supported output encodings. We anticipate that in conjunction with advocated auxiliary tools, like\ jq, a wide range of use-cases can be covered without extending vegeta itself. [0] https://github.com/tsenart/vegeta/issues/302#issuecomment-403935450<Paste> Closes #302

golangci · 2018-08-18T17:03:56Z

encode.go

+	}
+
+	return command{fs, func(args []string) error {
+		fs.Parse(args)


Error return value of fs.Parse is not checked

xla added the enhancement label Jul 14, 2018

xla self-assigned this Jul 14, 2018

xla requested a review from tsenart July 14, 2018 11:42

golangci reviewed Jul 14, 2018

View reviewed changes

tsenart requested changes Jul 14, 2018

View reviewed changes

golangci reviewed Aug 18, 2018

View reviewed changes

encode.go Outdated

}

return command{fs, func(args []string) error {

fs.Parse(args)

Copy link

golangci Aug 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error return value of fs.Parse is not checked

xla and others added 6 commits August 18, 2018 18:28

README: Improve wording

869ca32

encode: Include "vegeta" in FlagSet name

297a32e

encode: Reuse constants for flag usage strings

95a0473

encode: Auto-detect input encoding

d72f3f9

encode: Handle interrupt signal

f01a574

tsenart force-pushed the 302-dump-to-encode branch from abbed82 to f01a574 Compare August 18, 2018 16:30

encode: Improved command usage documentation

9d45d92

golangci reviewed Aug 18, 2018

View reviewed changes

encode.go

}

return command{fs, func(args []string) error {

fs.Parse(args)

Copy link

golangci Aug 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error return value of fs.Parse is not checked

tsenart added 2 commits August 18, 2018 19:15

dump: Return deprecation error on dump command

442432c

report: Return deprecation error on plot reporter

a5cd801

tsenart force-pushed the 302-dump-to-encode branch from 29734fd to a5cd801 Compare August 18, 2018 17:16

tsenart added 2 commits August 18, 2018 19:18

encode: Update README

ba2df25

vegeta: Exclude deprecated cmds from help message

08190d6

tsenart approved these changes Aug 18, 2018

View reviewed changes

tsenart merged commit c9bc7f7 into master Aug 18, 2018

tsenart deleted the 302-dump-to-encode branch August 18, 2018 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dump: Rework command to encode #318

dump: Rework command to encode #318

xla commented Jul 14, 2018 •

edited by tsenart

Loading

golangci Jul 14, 2018

tsenart Jul 14, 2018

golangci Jul 14, 2018

tsenart Jul 14, 2018

tsenart Jul 14, 2018

tsenart Aug 18, 2018

tsenart Jul 14, 2018

tsenart Jul 14, 2018

tsenart Jul 14, 2018

xla Jul 14, 2018

tsenart Jul 17, 2018

tsenart Jul 14, 2018

xla Jul 14, 2018

tsenart Jul 17, 2018

tsenart Jul 17, 2018

tsenart Jul 14, 2018

xla Jul 14, 2018

tsenart Jul 14, 2018

xla Jul 14, 2018

tsenart Jul 17, 2018

tsenart Jul 14, 2018

xla Jul 14, 2018

tsenart Jul 17, 2018

tsenart Jul 14, 2018

tsenart Jul 14, 2018

xla Jul 14, 2018

tsenart Jul 17, 2018

tsenart Jul 14, 2018

xla Jul 14, 2018

golangci Aug 18, 2018

golangci Aug 18, 2018

dump: Rework command to encode #318

dump: Rework command to encode #318

Conversation

xla commented Jul 14, 2018 • edited by tsenart Loading

Background

Checklist

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

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

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

xla commented Jul 14, 2018 •

edited by tsenart

Loading