cli: Enable cockroach debug tsdump to automatically create tsdump.yaml

[daniel-crlabs]

Release note (cli change): cockroach debug tsdump creates tsdump.yaml

The changes provided by this PR will enable users when creating a tsdump in raw format, to automatically create a tsdump.yaml file containing the storeID: nodeID mappings. This file is required by Technical Support in order to be able to load tsdump raw files for troubleshooting purposes, so this feature will automate the process of creating such yaml file.

This change will expedite the TSEs ability to investigate issues requiring the tsdump, as it eliminates the
requirement for TSEs to also obtain the debug zip in order to create the tsdump.yaml file.

The tsdump.yaml is only created automatically if the flag --format=raw is used, if the user choses any other format the yaml file will not be created. The tsdump.yaml will be automatically created in /tmp/tsump.yaml.

In addition to using the --format=raw flag, an user can pass the --yaml flag, allowing the user to chose where to save the yaml file, instead of the default location (/tmp).

Usage:

cockroach debug tsdump --host <host>:<port> --format raw --yaml=/some_path/tsdump.yaml > /some_path/tsdump.gob

Authored by: Daniel Almeida

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

[cockroach-teamcity]

This change is 

[daniel-crlabs]

I happened to get one of those automated emails about a stale issue, and long and behold, it was for a request made 2 years ago to have this feature possible. I'm adding it here for reference purposes to show this is not the first time this has come up, as the changes I have created in this PR will solve exactly this problem and fulfill this requirement.

cli: tsdump should also create tsdump.gob.yaml #80478

[dhartunian]

Thanks for filing this PR!

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

---

-- commits line 17 at r1:
nit: tsdump in path.

---

-- commits line 24 at r1:
you should add more detail for the docs team here. I think two things are worth highlighting (and will be repeated from text above):

1. raw format automatically creates yaml
2. new flag --yaml that controls output filename

---

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

	"github.com/cockroachdb/cockroach/pkg/cli/clierrorplus"
	"github.com/cockroachdb/cockroach/pkg/cli/clisqlclient" // Added by Daniel Almeida to enable yaml to be created

no need for this comment. information about who and why someone added a dependency can be gleaned from the git history and your commit message already.

---

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

	from, to     timestampValue
	clusterLabel string
	yaml         string // Added by Daniel Almeida to enable yaml to be created

no need for this comment

---

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

	to:           timestampValue(timeutil.Now().Add(24 * time.Hour)),
	clusterLabel: "",
	yaml:         "/tmp/tsdump.yaml", // Added by Daniel Almeida to enable yaml to be created. Default location is /tmp/tsdump.yaml

ditto. no need for this comment.

---

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

// Daniel Almeida function to obtain storeID to nodeID mappings when creating raw tsdump
// We automatically create the tsdump.yaml whenever creating a raw tsdump, as this is required for loading raw time series data

The format for function docstrings is detailed here: https://go.dev/blog/godoc

So in this case you would write:

// createYAML generates and writes the yaml to...

---

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

	defer file.Close()

	ctx := context.Background()

you can pass in the ctx from the caller of createYAML instead of making a new one here.

---

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

	ctx := context.Background()
	sqlConn, err := makeSQLClient(ctx, "tsdump-node-to-store-mapping", useSystemDb)

to follow patterns in rest of the pkg/cli codebase I would use cockroach tsdump as the appname here.

[daniel-crlabs]

Thanks for filing this PR!

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

-- commits line 17 at r1: nit: tsdump in path.

-- commits line 24 at r1: you should add more detail for the docs team here. I think two things are worth highlighting (and will be repeated from text above):

1. raw format automatically creates yaml
2. new flag --yaml that controls output filename

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

	"github.com/cockroachdb/cockroach/pkg/cli/clierrorplus"
	"github.com/cockroachdb/cockroach/pkg/cli/clisqlclient" // Added by Daniel Almeida to enable yaml to be created

no need for this comment. information about who and why someone added a dependency can be gleaned from the git history and your commit message already.

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

	from, to     timestampValue
	clusterLabel string
	yaml         string // Added by Daniel Almeida to enable yaml to be created

no need for this comment

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

	to:           timestampValue(timeutil.Now().Add(24 * time.Hour)),
	clusterLabel: "",
	yaml:         "/tmp/tsdump.yaml", // Added by Daniel Almeida to enable yaml to be created. Default location is /tmp/tsdump.yaml

ditto. no need for this comment.

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

// Daniel Almeida function to obtain storeID to nodeID mappings when creating raw tsdump
// We automatically create the tsdump.yaml whenever creating a raw tsdump, as this is required for loading raw time series data

The format for function docstrings is detailed here: https://go.dev/blog/godoc

So in this case you would write:

// createYAML generates and writes the yaml to...

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

	defer file.Close()

	ctx := context.Background()

you can pass in the ctx from the caller of createYAML instead of making a new one here.

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

	ctx := context.Background()
	sqlConn, err := makeSQLClient(ctx, "tsdump-node-to-store-mapping", useSystemDb)

to follow patterns in rest of the pkg/cli codebase I would use cockroach tsdump as the appname here.

Sorry I forgot to comment on the ticket (I thought posting the updates to the code would trigger a new review automatically). Please review when you have a chance, I've done all the changes.

[dhartunian]

@daniel-crlabs now that this PR is approved, if you post a github comment with the textbors r+ this PR will enter the merge queue and end up in the master branch.

Backport PRs will automatically be created by the bot and I will approve those once they're up as well. Backport PRs can be merged after approval with the standard GitHub merge button (no bors required)

Also, please open a PR to add your name to the AUTHORS file 🎉 (https://github.com/cockroachdb/cockroach/blob/master/AUTHORS)

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @daniel-crlabs)

[daniel-crlabs]

Thank you, appreciate the work on this.

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[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 a980c77 to blathers/backport-release-22.2-114046: 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 22.2.x failed. See errors above.

---

error creating merge commit from a980c77 to blathers/backport-release-23.1-114046: 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 23.1.x failed. See errors above.

---

error creating merge commit from a980c77 to blathers/backport-release-23.2-114046: 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 23.2.x failed. See errors above.

---

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

[daniel-crlabs]

Hi @dhartunian I'm a bit confused by all of the errors above, is this a concern?

[mattcrdb]

ping @dhartunian

[dhartunian]

Backport for 23.2 is up. 23.1 is a lot more tricky. Can we get by with the 23.2 backport alone?

[mattcrdb]

We'll need 23.1 and 22.2 (6 mo support remaining) as well. Customers will choose to stay one version back for a major version so backporting to 23.2 will leave us little benefit for this change in the short term.

[nickelnickel]

To @mattcrdb's point we need 23.1. Having to collect debug.zip in addition to a tsdump is a major pain point with a number of customers.

Not totally ideal, but can customers use cockroach sql <connection string> -e "select store_id || ': ' || node_id from crdb_internal.kv_store_status;" | tail -n +2 > tsdump.gob.yaml to get this information in versions that don't have this implemented?