cli: add timestamp as tag in tsdump datadog upload

[aa-joshi]

Previously, we updated upload_id identifier to include timestamp as part of based on uploadID. This change adds upload year, month & day tags as part of tsdump upload.

Epic: None
Part of: CRDB-44379
Release note: None

[cockroach-teamcity]

This change is 

[arjunmahishi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi and @angles-n-daemons)

---

pkg/cli/tsdump_upload.go line 95 at r1 (raw file):

	if debugTimeSeriesDumpOpts.clusterLabel != "" {
		clusterTagValue = debugTimeSeriesDumpOpts.clusterLabel
	} else if serverCfg.ClusterName != "" {

Will this ever be true? I think this is generating a "default cluster settings" object. I traced back the code. I don't think ClusterName is being set anywhere. I could be wrong. But worth verifying once to trim down this function.

---

pkg/cli/tsdump_upload.go line 98 at r1 (raw file):

		clusterTagValue = serverCfg.ClusterName
	} else {
		clusterTagValue = fmt.Sprintf("cluster-debug-%d", timeutil.Now().Unix())

The timestamp will anyway be appended in the newUploadID function right? We should skip the -%d time in this else case

---

pkg/cli/tsdump_upload.go line 99 at r1 (raw file):

	} else {
		clusterTagValue = fmt.Sprintf("cluster-debug-%d", timeutil.Now().Unix())
	}

NIT: We can shorten the if-else ladder if we handle the default cluster name first.

Suggestion:

	clusterTagValue := fmt.Sprintf("cluster-debug-%d", timeutil.Now().Unix())
	if debugTimeSeriesDumpOpts.clusterLabel != "" {
		clusterTagValue = debugTimeSeriesDumpOpts.clusterLabel
	} else if serverCfg.ClusterName != "" {
		clusterTagValue = serverCfg.ClusterName
	}

---

pkg/cli/tsdump_upload.go line 225 at r1 (raw file):

	tags = append(tags, makeDDTag(uploadIDTag, d.uploadID))

	year, month, day := timeutil.Now().Date()

Should this be using d.uploadTime instead? (for consistence)

---

pkg/cli/tsdump_test.go line 192 at r1 (raw file):

	defer leaktest.AfterTest(t)()
	defer log.Scope(t).Close(t)
	defer testutils.TestingHook(&newUploadID, func(cluster string, uploadTime time.Time) string {

I don't think we need this hook anymore. newUploadID is a pure function now. The output is determined by getCurrentTime. Since that is already taken care of, I think we can skip this hook. I think we can make the function a non-variable also now.

---

pkg/cli/zip_upload_test.go line 111 at r1 (raw file):

	defer leaktest.AfterTest(t)()
	defer log.Scope(t).Close(t)
	defer testutils.TestingHook(&newUploadID, func(string, time.Time) string {

Same as above. We can skip this change in this PR if you want.

[aa-joshi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angles-n-daemons and @arjunmahishi)

---

pkg/cli/tsdump_upload.go line 95 at r1 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

Will this ever be true? I think this is generating a "default cluster settings" object. I traced back the code. I don't think ClusterName is being set anywhere. I could be wrong. But worth verifying once to trim down this function.

Done.

---

pkg/cli/tsdump_upload.go line 98 at r1 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

The timestamp will anyway be appended in the newUploadID function right? We should skip the -%d time in this else case

Done.

---

pkg/cli/tsdump_upload.go line 225 at r1 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

Should this be using d.uploadTime instead? (for consistence)

Thanks for Pointing it out!. Done

---

pkg/cli/tsdump_test.go line 192 at r1 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

I don't think we need this hook anymore. newUploadID is a pure function now. The output is determined by getCurrentTime. Since that is already taken care of, I think we can skip this hook. I think we can make the function a non-variable also now.

Done.

---

pkg/cli/zip_upload_test.go line 111 at r1 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

Same as above. We can skip this change in this PR if you want.

Here it is needed because we are calling timeutil.Now() in method invocation

[arjunmahishi]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @angles-n-daemons)

[aa-joshi]

TFTR!

bors r+

[craig]

Build succeeded:

• examples_orms
• windows_build
• acceptance
• macos_amd64_build
• linux_amd64_fips_build
• lint
• linux_amd64_build
• local_roachtest_fips
• local_roachtest
• check_generated_code
• docker_image_amd64
• unit_tests
• linux_arm64_build
• macos_arm64_build