cli: render debug merge-logs output in color

[postamar]

Previously, the log entries printed by the 'cockroach debug merge-logs'
command were not rendered in color. This command takes log files,
typically from a debug.zip, and merges them in a single stream. In so
doing, it prefixes each entry with a string identifying its pre-merged
origin on-disk. Typically, this is a node number.

This commit renders the merged log entries in color using the existing
legacy (v1) color scheme. Furthermore, the prefix backgrounds are also
colored in a deterministic pseudorandom fashion to help identify the
origin of each log entry.

Release note (cli change): The 'cockroach debug merge-logs' command now
renders in color by default.

[cockroach-teamcity]

This change is 

[knz]

cc @cameronnunez you might be interested in co-reviewing this?

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

---

pkg/cli/debug_merge_logs.go, line 65 at r1 (raw file):

		}
		err = log.FormatLegacyEntryPrefixTTY(prefixBytes, w)
		if err != nil {

in an ideal world, the debug merge-log command would obey the customized output logging format for the stderr sink.
This way the command would output JSON if the user requested that. Right now the code (and your PR) enforces the output format be crdb-v1-tty.

This approach will make-do for now, but we'll want to switch over around when #65633 merges.

---

pkg/util/log/format_crdb_v1.go, line 69 at r1 (raw file):

	{
		n := len(cp)
		counter := adler32.Checksum(prefix) % uint32(n-1)

I find this logic hard to read. What was wrong with simply prefixCode := adler32.Checksum(prefix) % uint32(len(cp)-1) ?

the "worst" that can happen is that some nodes do not get a color? But that's "a color" on its own too..

---

pkg/util/log/format_crdb_v1.go, line 88 at r1 (raw file):

			return err
		}
		if _, err = w.Write([]byte("\033[7m")); err != nil {

it's odd to see a constant string inside this file. Maybe add the escape to the ttycolor package?

---

pkg/util/log/format_crdb_v1.go, line 97 at r1 (raw file):

	}
	if prefixCode != ttycolor.Reset {
		if _, err = w.Write(cp[ttycolor.Reset]); err != nil {

please consider handling this with a defer
you can merge the errors with errors.CombineErrors().

[postamar]

Thanks for these comments! I'll address them in a couple of weeks when I'll be back.

[postamar]

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

---

pkg/cli/debug_merge_logs.go, line 65 at r1 (raw file):

Previously, knz (kena) wrote…

in an ideal world, the debug merge-log command would obey the customized output logging format for the stderr sink.
This way the command would output JSON if the user requested that. Right now the code (and your PR) enforces the output format be crdb-v1-tty.

This approach will make-do for now, but we'll want to switch over around when #65633 merges.

Understood.

---

pkg/util/log/format_crdb_v1.go, line 69 at r1 (raw file):

Previously, knz (kena) wrote…

I find this logic hard to read. What was wrong with simply prefixCode := adler32.Checksum(prefix) % uint32(len(cp)-1) ?

the "worst" that can happen is that some nodes do not get a color? But that's "a color" on its own too..

I simply didn't realize that ttycolor was one that we maintained, I incorrectly assumed it was a third-party package. I submitted a pull request to ttycolor for your review. Currently this PR doesn't build because the vendored version hasn't been updated yet (naturally).

---

pkg/util/log/format_crdb_v1.go, line 88 at r1 (raw file):

Previously, knz (kena) wrote…

it's odd to see a constant string inside this file. Maybe add the escape to the ttycolor package?

Done.

---

pkg/util/log/format_crdb_v1.go, line 97 at r1 (raw file):

Previously, knz (kena) wrote…

please consider handling this with a defer
you can merge the errors with errors.CombineErrors().

Done.

[knz]

LGTM modulo the two recommended changes

Reviewed 7 of 7 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

---

pkg/cli/debug_merge_logs.go, line 65 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

Understood.

you can add a TODO in the code to that effect. Thanks

---

pkg/util/log/format_crdb.go, line 70 at r2 (raw file):

		defer func() {
			_, errReset := w.Write(cp[ttycolor.Reset])
			if errReset != nil {

no need for the conditional. CombineErrors already does this for you.

[postamar]

Thanks for the review!

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

---

pkg/cli/debug_merge_logs.go, line 65 at r1 (raw file):

Previously, knz (kena) wrote…

you can add a TODO in the code to that effect. Thanks

Done, thanks, I wasn't sure what you wanted me to do about this.

---

pkg/util/log/format_crdb.go, line 70 at r2 (raw file):

Previously, knz (kena) wrote…

no need for the conditional. CombineErrors already does this for you.

Neat!

[postamar]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[erikgrinaker]

Is there a way to disable this? It makes it really hard to look at the logs in an editor, with all of the ANSI code noise. I don't really think it makes sense to colorize the output when it's not going to a TTY, i.e. cockroach debug merge-logs > combined.log shouldn't output color.

[knz]

we don't typically add for changes on merged PRs. I'll file an issue.

[erikgrinaker]

Already did: #69692.