cli: add --{from,to} to tsdump command

[tbg]

This allows restricting which range of datapoints is pulled by
./cockroach debug tsdump via the --from and --to flags.

This command also touches up the tsdump command a little bit.

Release justification: low-risk observability change
Release note (cli change): The debug tsdump command now accepts
--from and --to flags that limit for which dates timeseries
are exported.

[cockroach-teamcity]

This change is 

[knz]

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/cli/debug.go, line 1569 at r1 (raw file):

	f.Var(&debugTimeSeriesDumpOpts.format, "format", "output format (text, csv, tsv, raw)")
	f.Var(&debugTimeSeriesDumpOpts.from, "from", "oldest timestamp to include")
	f.Var(&debugTimeSeriesDumpOpts.to, "to", "newest timestamp to include")

might be worth specifying whether they are inclusive or exclusive

---

pkg/cli/tsdump.go, line 78 at r1 (raw file):

			// Stdout does not support Sync in all situations, for example when piping
			// out from roachprod via `roachprod ssh foo:1 -- [...] tsdump > foo.bar`,
			// so ignore the error.

Can you explain this a bit more? I am pretty sure that sync is supported in that case, and more generally we want that write errors on the output stream translate into a process error especially when redirecting to a file locally.

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

---

pkg/cli/tsdump.go, line 78 at r1 (raw file):

Previously, knz (kena) wrote…

Can you explain this a bit more? I am pretty sure that sync is supported in that case, and more generally we want that write errors on the output stream translate into a process error especially when redirecting to a file locally.

I don't have much more to explain, look here's a repro:

roachprod create -n 1 $USER-ouch
roachprod stage $USER-ouch release v21.1.7
roachprod start $USER-ouch
roachprod ssh $USER-ouch -- ./cockroach debug tsdump --format=raw --insecure > foo.txt
ERROR: sync /dev/stdout: invalid argument
Failed running "debug tsdump"

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

---

pkg/cli/tsdump.go, line 78 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I don't have much more to explain, look here's a repro:

roachprod create -n 1 $USER-ouch
roachprod stage $USER-ouch release v21.1.7
roachprod start $USER-ouch
roachprod ssh $USER-ouch -- ./cockroach debug tsdump --format=raw --insecure > foo.txt
ERROR: sync /dev/stdout: invalid argument
Failed running "debug tsdump"

uber-go/zap#328 (comment)

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/cli/tsdump.go, line 78 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

uber-go/zap#328 (comment)

oh my, I had misread this entire time.
We never ever cared for a Sync() here. What we want is a Flush(). That should always work, and if it doesn't that's an error we want to see.

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

---

pkg/cli/tsdump.go, line 78 at r1 (raw file):

Previously, knz (kena) wrote…

oh my, I had misread this entire time.
We never ever cared for a Sync() here. What we want is a Flush(). That should always work, and if it doesn't that's an error we want to see.

os.Stdout is a *os.File, which does not have Flush().

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)

---

pkg/cli/debug.go, line 1569 at r1 (raw file):

Previously, knz (kena) wrote…

might be worth specifying whether they are inclusive or exclusive

Done.

---

pkg/cli/tsdump.go, line 78 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

os.Stdout is a *os.File, which does not have Flush().

Updated the comment.

[knz]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/cli/tsdump.go, line 78 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Updated the comment.

I had forgotten that go's os.File are not buffered.
IMHO we can drop the Sync (and the comment) entirely. It does not buy us anything.

Separately, it may be worth adding a bufferedwriter on this. Writing to the file row by row via the CSV writer probably results in terrible write performance. (And then add a Flush and check the error)

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)

---

pkg/cli/tsdump.go, line 78 at r1 (raw file):

Previously, knz (kena) wrote…

I had forgotten that go's os.File are not buffered.
IMHO we can drop the Sync (and the comment) entirely. It does not buy us anything.

Separately, it may be worth adding a bufferedwriter on this. Writing to the file row by row via the CSV writer probably results in terrible write performance. (And then add a Flush and check the error)

Added write buffering, PTAL.

[tbg]

bors r=knz
TFTR!

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[tbg]

blathers backport 21.1

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from ddfa4db to blathers/backport-release-21.1-69491: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.1 failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[tbg]

Looked at this, there are a few annoying conflicts, so I'm going to abandon the attempt to backport this, since we don't have a concrete use case yet and 21.2 is around the corner.