server: refactor debug zip logfiles fetching

[ridwanmsharif]

Use the GetFiles endpoint instead for logfile collection
instead of using the current enpoint that serves logs in the
form of entries. This method is more intuitive with what the
zip client actually wants to do.

For backwards compatibility reasons, the old method must still
be supported.

Release note: None

[cockroach-teamcity]

This change is 

[benesch]

Hm, this has been sitting for a while. Is @petermattis the right person to review this?

[ridwanmsharif]

Put this up one weekend to refactor logfile fetching, I think @andreimatei was familiar with the heap fetching stuff code and suggested we make the change, and so could review this PR.

[andreimatei]

LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cli/zip.go, line 302 at r1 (raw file):

					ctx, cancel := timeoutCtx(baseCtx)
					defer cancel()
					// Use older method of log fetching for backwards compatibility.

pls use more words for this comment - say what the "old" method and the new one are.
Also, this method is way too long and indented already - would you mind extracting all the logs code in a new method (probably with two sub-methods for the two ways of fetching the files

---

pkg/cli/zip.go, line 334 at r1 (raw file):

							}
						}
					} else {

nit: put the expected code path first - which is the new code you're running
(except if the exceptional code path is very short - like an early error return which lets you unindent the rest of the function)

---

pkg/cli/zip.go, line 338 at r1 (raw file):

							NodeId:   id,
							Type:     serverpb.FileType_LOG,
							ListOnly: true,

why do we list only to download all of them after? If you're concerned about total file size, comment on that.

---

pkg/cli/zip.go, line 342 at r1 (raw file):

						}); err != nil {
							if err := z.createError(prefix+"/logs", err); err != nil {
								return err

extracting all this code into a separate method will hopefully let you avoid this need to indent once more after every error check...

---

pkg/server/status.go, line 626 at r1 (raw file):

	case serverpb.FileType_HEAP: // Requesting for saved Heap Profiles.
		dir = filepath.Join(s.admin.server.cfg.HeapProfileDirName, heapDir)
	case serverpb.FileType_LOG: // Requesting for saved Heap Profiles.

wrong comment

---

pkg/server/status_test.go, line 195 at r1 (raw file):

	defer leaktest.AfterTest(t)()

	if log.V(3) {

what's this about? Sounds funky; can we make the test robust enough for higher levels?

---

pkg/server/serverpb/status.proto, line 304 at r1 (raw file):

// Represents the type of file.
enum FileType { 

trailing space

---

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

}

// GetLogDir returns the log directory

what error can this return?
If it's never supposed to happen, consider asserting on it and not returning it.

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[cockroach-teamcity]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Ridwan Sharif seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}