suppress headers in output for influx when they are the same #8122

corylanou · 2017-03-10T19:55:27Z

If you encountered a chunked result while processing the results of a query, you would get the headers inserted for no reason. This PR suppresses the header when they are the same so as to not inflate the output. This is very useful when doing queries and grabbing a line count for adhoc testing, not to mention it created issues with csv layouts.

There is no good way to add testing for this without a larger refactor, so I've done extensive scenario adhoc testing. I'll list those below, but if you are reviewing this PR, the most important thing to look for is any scenarios I might of missed.

CSV Output

Before Change

> select * from m2
name,time,area,cpu,mem,region,val
m2,1489160933298193430,,,,west,8
m2,1489160955106239552,north,99.8,123254,,
m2,1489160980553494642,south,98.8,123525,,
m2,1489160999872615861,east,97.8,123523,,
m2,1489161005520282539,east,97.8,123522,,
name,time,area,cpu,mem,region,val
m2,1489161007072346588,east,97.8,123521,,
m2,1489161024602161179,east,97.8,123520,,

After Change

> select * from m2
name,time,area,cpu,mem,region,val
m2,1489160933298193430,,,,west,8
m2,1489160955106239552,north,99.8,123254,,
m2,1489160980553494642,south,98.8,123525,,
m2,1489160999872615861,east,97.8,123523,,
m2,1489161005520282539,east,97.8,123522,,
m2,1489161007072346588,east,97.8,123521,,
m2,1489161024602161179,east,97.8,123520,,

Column Output

Before Change

> select * from m2
name: m2
time                area  cpu  mem    region val
----                ----  ---  ---    ------ ---
1489160933298193430                   west   8
1489160955106239552 north 99.8 123254
1489160980553494642 south 98.8 123525
1489160999872615861 east  97.8 123523
1489161005520282539 east  97.8 123522

name: m2
time                area cpu  mem    region val
----                ---- ---  ---    ------ ---
1489161007072346588 east 97.8 123521
1489161024602161179 east 97.8 123520

After Change

> select * from m2
name: m2
time                area  cpu  mem    region val
----                ----  ---  ---    ------ ---
1489160933298193430                   west   8
1489160955106239552 north 99.8 123254
1489160980553494642 south 98.8 123525
1489160999872615861 east  97.8 123523
1489161005520282539 east  97.8 123522
1489161007072346588 east  97.8 123521
1489161024602161179 east  97.8 123520

Multiple Measurement + Chunked

Before Change

> select * from m2
name: m2
time                area  cpu  mem    region val
----                ----  ---  ---    ------ ---
1489160933298193430                   west   8
1489160955106239552 north 99.8 123254
1489160980553494642 south 98.8 123525
1489160999872615861 east  97.8 123523
1489161005520282539 east  97.8 123522

name: m2
time                area cpu  mem    region val
----                ---- ---  ---    ------ ---
1489161007072346588 east 97.8 123521
1489161024602161179 east 97.8 123520

> select * from /m*/
name: m1
time                area cpu field mem region val
----                ---- --- ----- --- ------ ---
1489160894168214149                    east   1
1489160896124618043                    east   2
1489160897460385000                    east   3
1489160898924532600                    east   4
1489160900644363008                    east   5

name: m1
time                area cpu field mem region val
----                ---- --- ----- --- ------ ---
1489160907827927527                    west   6
1489160910315118036                    west   7

name: m2
time                area  cpu  field mem    region val
----                ----  ---  ----- ---    ------ ---
1489160933298193430                         west   8
1489160955106239552 north 99.8       123254
1489160980553494642 south 98.8       123525
1489160999872615861 east  97.8       123523
1489161005520282539 east  97.8       123522

name: m2
time                area cpu  field mem    region val
----                ---- ---  ----- ---    ------ ---
1489161007072346588 east 97.8       123521
1489161024602161179 east 97.8       123520

name: m3
time                area cpu field mem region val
----                ---- --- ----- --- ------ ---
1489161208341811442          0

After Change

> select * from /m*/
name: m1
time                area cpu field mem region val
----                ---- --- ----- --- ------ ---
1489160894168214149                    east   1
1489160896124618043                    east   2
1489160897460385000                    east   3
1489160898924532600                    east   4
1489160900644363008                    east   5
1489160907827927527                    west   6
1489160910315118036                    west   7

name: m2
time                area  cpu  field mem    region val
----                ----  ---  ----- ---    ------ ---
1489160933298193430                         west   8
1489160955106239552 north 99.8       123254
1489160980553494642 south 98.8       123525
1489160999872615861 east  97.8       123523
1489161005520282539 east  97.8       123522
1489161007072346588 east  97.8       123521
1489161024602161179 east  97.8       123520

name: m3
time                area cpu field mem region val
----                ---- --- ----- --- ------ ---
1489161208341811442          0

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated

e-dard

Just some suggested refactoring and the question about flush

e-dard · 2017-03-14T15:10:51Z

cmd/influx/cli/cli.go

@@ -777,38 +778,103 @@ func (c *CommandLine) writeJSON(response *client.Response, w io.Writer) {
 	fmt.Fprintln(w, string(data))
 }

+func tagsEqual(prev, current map[string]string) bool {
+	if prev == nil && current == nil {


You can remove this check; nil maps have zero length.

e-dard · 2017-03-14T15:13:05Z

cmd/influx/cli/cli.go

+	if prev == nil && current == nil {
+		return true
+	}
+	if len(prev) != len(current) {


You can probably just remove this too. I checked the source of DeepEqual and one of the first things it does it check the length of two maps.

e-dard · 2017-03-14T15:13:32Z

cmd/influx/cli/cli.go

+	if prev == nil && current == nil {
+		return true
+	}
+	if len(prev) != len(current) {


Remove redundant checks, as with tagsEqual.

e-dard · 2017-03-14T15:14:02Z

cmd/influx/cli/cli.go

+	if prev.Name != current.Name {
+		return false
+	}
+	if prev.Tags == nil && current.Tags == nil {


You can remove this check.

e-dard · 2017-03-14T15:15:06Z

cmd/influx/cli/cli.go

+		return false
+	}
+
+	return true


How about you remove 807–812 and instead do:

return tagsEqual(prev.Tags, current.Tags) && columnsEqual(prev.Columns, current.Columns)

e-dard · 2017-03-14T15:17:42Z

cmd/influx/cli/cli.go

 	}
+	csvw.Flush()


What's the reason for flushing less often?

You only need to call flush once. It's always streaming. And if you call flush to early, it actually messes up the alignment on the output (didn't read the source, but assuming it resets the column width tracking). There is no benefit to calling flush more than once regardless.

e-dard · 2017-03-14T15:20:17Z

cmd/influx/cli/cli.go

+		// if we are not at the end of the results, and we aren't suppressing headers, put in a new line
+		// to break up the results
+		// We don't print one if we are the first result, or the last result
+		if !suppressHeaders && i > 0 && i < len(response.Results) {


Shouldn't this be ... && i < len(response.Results)-1 {? The last result will have an index < len(response.Results).

This is putting line returns between results, and it outputs it before the call to the headers. So the actual fix is to take out the check for i < len(response.Results) as it is always true. Good catch.

e-dard · 2017-03-14T15:20:52Z

cmd/influx/cli/cli.go

 	}
+	writer.Flush()


Again, flush moving down. I'm sure this is fine but I'm not sure on the reasoning behind it.

Same as above.

corylanou · 2017-03-14T19:08:08Z

@e-dard comments addressed. Take another gander when you get a chance. Thanks.

corylanou · 2017-03-14T19:09:10Z

also, this PR is based off of #8119 (chunked/size settings) to be able to test it out, so I'll rebase/merge once that one is merged.

jsternberg · 2017-03-15T15:23:44Z

This might be more simple to implement by just starting to switch the CLI to using the alternative client. The alternative client already concatenates partial responses so you wouldn't have to check the headers.

You can also just check to see if the statement id has changed or not now that the statement id gets returned with the response. I think it would be easier than checking the headers for equality.

corylanou · 2017-03-15T15:26:58Z

Moving to a new client is bigger change, and one I agree should be done, but this fixes a real problem now.

jsternberg

Seems fine to me then. I'll prep a change in the next week for the new client.

corylanou added the in progress label Mar 10, 2017

corylanou force-pushed the cjl-influx-chunked branch from f625e8e to a322571 Compare March 13, 2017 16:18

corylanou force-pushed the cjl-influx-suppress-headers branch 2 times, most recently from 265b9a8 to f46a9e2 Compare March 13, 2017 16:21

corylanou added the review label Mar 13, 2017

corylanou requested review from e-dard and jsternberg March 13, 2017 20:11

e-dard suggested changes Mar 14, 2017

View reviewed changes

corylanou force-pushed the cjl-influx-suppress-headers branch from f46a9e2 to d7696d1 Compare March 14, 2017 16:45

e-dard approved these changes Mar 14, 2017

View reviewed changes

jsternberg approved these changes Mar 15, 2017

View reviewed changes

corylanou force-pushed the cjl-influx-chunked branch 2 times, most recently from 83c3a8a to e07d845 Compare March 17, 2017 23:26

corylanou changed the base branch from cjl-influx-chunked to master March 28, 2017 15:52

corylanou added 2 commits March 28, 2017 12:48

add chunked/chunksize as setting/options

8094822

suppress headers in output for influx when they are the same

9f674cc

corylanou force-pushed the cjl-influx-suppress-headers branch from d7696d1 to 9f674cc Compare March 28, 2017 17:50

rbetts removed the in progress label Mar 30, 2017

corylanou merged commit 7a6243e into master Apr 3, 2017

corylanou removed the review label Apr 3, 2017

mark-rushakoff deleted the cjl-influx-suppress-headers branch September 7, 2017 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

suppress headers in output for influx when they are the same #8122

suppress headers in output for influx when they are the same #8122

corylanou commented Mar 10, 2017 •

edited

Loading

e-dard left a comment

e-dard Mar 14, 2017

e-dard Mar 14, 2017

e-dard Mar 14, 2017

e-dard Mar 14, 2017

e-dard Mar 14, 2017

e-dard Mar 14, 2017

corylanou Mar 14, 2017

e-dard Mar 14, 2017

corylanou Mar 14, 2017

e-dard Mar 14, 2017

corylanou Mar 14, 2017

corylanou commented Mar 14, 2017

corylanou commented Mar 14, 2017

jsternberg commented Mar 15, 2017

corylanou commented Mar 15, 2017

jsternberg left a comment

suppress headers in output for influx when they are the same #8122

suppress headers in output for influx when they are the same #8122

Conversation

corylanou commented Mar 10, 2017 • edited Loading

CSV Output

Before Change

After Change

Column Output

Before Change

After Change

Multiple Measurement + Chunked

Before Change

After Change

Required for all non-trivial PRs

e-dard left a 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

corylanou commented Mar 14, 2017

corylanou commented Mar 14, 2017

jsternberg commented Mar 15, 2017

corylanou commented Mar 15, 2017

jsternberg left a comment

Choose a reason for hiding this comment

corylanou commented Mar 10, 2017 •

edited

Loading