backup: Export tenant backup schedule metric #141156

edwardguo-crl · 2025-02-11T17:05:53Z

Export Cloud tenant backup schedule completion metric with tenant id tag.

Epic: None

Release note: None

blathers-crl · 2025-02-11T17:05:57Z

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 · 2025-02-11T17:06:03Z

This change is

edwardguo-crl · 2025-02-11T17:06:59Z

Quick question: Is there a way I can test the exported metric before running it in staging? @msbutler

kev-cao · 2025-02-11T17:09:42Z

pkg/backup/schedule_exec.go

@@ -578,6 +584,12 @@ func init() {
 						Measurement: "Jobs",
 						Unit:        metric.Unit_TIMESTAMP_SEC,
 					}),
+					RpoTenantMetric: NewExportedGaugeVec(metric.Metadata{
+						Name:        "schedules.BACKUP.tenant-last-completed-time",
+						Help:        "The unix timestamp of the most recently completed backup by a tenant schedule specified as maintaining this metric",


This isn't a "tenant schedule" but rather a schedule for backing up a tenant. So the schedule still exists on the host cluster, but backs up the tenant.

kev-cao · 2025-02-11T17:12:00Z

pkg/backup/schedule_exec.go

@@ -362,6 +363,11 @@ func (e *scheduledBackupExecutor) backupSucceeded(
 		e.metrics.RpoMetric.Update(details.(jobspb.BackupDetails).EndTime.GoTime().Unix())
 	}

+	if details.(jobspb.BackupDetails).SpecificTenantIds != nil {


We have the UpdatesLastBackupMetric for determining the "special" schedule that will update the metric — should we do the same for these backup tenant schedules?

Thoughts @msbutler?

yeah, it looks like the update for this metric should be in the UpdatesLastBackupmetric branch above.

kev-cao · 2025-02-11T17:16:19Z

pkg/backup/schedule_exec.go

@@ -362,6 +363,11 @@ func (e *scheduledBackupExecutor) backupSucceeded(
 		e.metrics.RpoMetric.Update(details.(jobspb.BackupDetails).EndTime.GoTime().Unix())
 	}

+	if details.(jobspb.BackupDetails).SpecificTenantIds != nil {
+		e.metrics.RpoTenantMetric.Update(map[string]string{"tenant_id": details.(jobspb.BackupDetails).SpecificTenantIds},


SpecificTenantIDs is a []string, so you'll want to loop over them and emit a metric for each one.

kev-cao · 2025-02-11T17:20:30Z

@edwardguo-crl I'm not aware of a way to use a custom Cockroach binary for the clusters in your local CC deployment, and my gut tells me that we don't have a mechanism built for that. @benbardin any insights on this?

kev-cao · 2025-02-11T17:25:33Z

@edwardguo-crl The commit should ideally be prefixed with the package it affects and indicated in the PR title as well. We also generally don't link to the CC ticket in this repo.

nit: I think the PR description can be a bit more specific — there's no mechanism in place that makes the metric "cloud only". I'd be a bit more descriptive in what the metric actually represents as well.

msbutler · 2025-02-11T17:25:37Z

@edwardguo-crl thanks for opening this! A few general process nits:

it looks like you're linking the crdb pr to a cc issue. In general, crdb pr's shoudl be linked to crdb issues. could you open a corresponding crdb issue instead?
once you have the crdb issue, you can follow the general crdb commit guidelines: this pr should be named: backup: export tenant ..... The commit message should contain: Fixes {crdb_issue}.

msbutler · 2025-02-11T17:31:44Z

Is there a way I can test the exported metric before running it in staging?

@edwardguo-crl i would augment a test in backup_test.go that creates a backup of several tenants and verify the metrics were updated using some of the infra used in https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/metric_test.go

pkg/backup/schedule_exec.go

kev-cao · 2025-02-24T19:07:09Z

pkg/backup/schedule_exec.go

@@ -578,6 +588,12 @@ func init() {
 						Measurement: "Jobs",
 						Unit:        metric.Unit_TIMESTAMP_SEC,
 					}),
+					RpoTenantMetric: metric.NewExportedGaugeVec(metric.Metadata{
+						Name:        "schedules.BACKUP.tenant-last-completed-time",
+						Help:        "The Unix timestamp of the most recently completed backup by host schedule for the tenant",


Something like "The unix timestamp of the most recently completed tenant backup by a host schedule specified as maintaining this metric" would be more fitting and similar to the existing description.

I like: "The unix timestamp of the most recently completed host scheduled backup by virtual cluster specified as maintaining this metric"

kev-cao · 2025-02-24T19:08:21Z

pkg/backup/schedule_exec_test.go

+			RpoTenantMetric: metric.NewExportedGaugeVec(metric.Metadata{}, []string{"tenant_id"}),
+		},
+	}
+	env := scheduledjobs.ProdJobSchedulerEnv


It seems like our existing tests use a test environment instead of the production scheduler environment. I'm not familiar with this part of the codebase though, so @msbutler would have a better idea on how to best do the testing here.

I don't think the env matters here. @edwardguo-crl this is an awesome test!

msbutler

@edwardguo-crl awesome job with the test! this patch is nearly there.

msbutler · 2025-02-25T14:34:20Z

pkg/backup/schedule_exec.go

@@ -574,10 +581,16 @@ func init() {
 					ExecutorPTSMetrics: &pm,
 					RpoMetric: metric.NewGauge(metric.Metadata{
 						Name:        "schedules.BACKUP.last-completed-time",
-						Help:        "The unix timestamp of the most recently completed backup by a schedule specified as maintaining this metric",
+						Help:        "The unix timestamp of the most recently completed tenant backup by a host schedule specified as maintaining this metric",


I would revert this change. The metric is bumped by a schedule within the cluster, so "host" is misleading. We also typically refer to backups taken within a tenant as just a "backup". A "tenant backup" typically refers to a backup of a tenant.

msbutler · 2025-02-25T14:34:55Z

pkg/backup/schedule_exec.go

 						Measurement: "Jobs",
 						Unit:        metric.Unit_TIMESTAMP_SEC,
 					}),
+					RpoTenantMetric: metric.NewExportedGaugeVec(metric.Metadata{
+						Name:        "schedules.BACKUP.tenant-last-completed-time",


nit: we're trying to scrub all mentions of "tenant" in our codebase and docs. Use virtual cluster instead.

how about this metric name: "schedules.BACKUP.last-completed-time-by-virtual_cluster"

msbutler · 2025-02-25T14:41:16Z

pkg/backup/schedule_exec.go

@@ -578,6 +588,12 @@ func init() {
 						Measurement: "Jobs",
 						Unit:        metric.Unit_TIMESTAMP_SEC,
 					}),
+					RpoTenantMetric: metric.NewExportedGaugeVec(metric.Metadata{
+						Name:        "schedules.BACKUP.tenant-last-completed-time",
+						Help:        "The Unix timestamp of the most recently completed backup by host schedule for the tenant",


I like: "The unix timestamp of the most recently completed host scheduled backup by virtual cluster specified as maintaining this metric"

msbutler · 2025-02-25T14:47:39Z

pkg/backup/schedule_exec_test.go

+	return tenantIDs
+}
+
+func verifyRPOTenantMetricLabels(


could you also pass in and verify the expected end time?

msbutler · 2025-02-25T14:48:05Z

pkg/backup/schedule_exec_test.go

+			RpoTenantMetric: metric.NewExportedGaugeVec(metric.Metadata{}, []string{"tenant_id"}),
+		},
+	}
+	env := scheduledjobs.ProdJobSchedulerEnv


I don't think the env matters here. @edwardguo-crl this is an awesome test!

msbutler · 2025-02-25T14:49:40Z

pkg/backup/schedule_exec_test.go

+		err := executor.backupSucceeded(ctx, nil, schedule, details, env)
+		require.NoError(t, err)
+
+		expectedTenantIDs := []string{"system", "2"}


nit: also verify that executor.metrics.RpoMetric.Value() was also updated in this case?

I don't think there is a Value() method in RpoMetric. I verified the Gauge metric value in a separate function.

Export Cloud tenant backup schedule completion metric with tenant id tag. Issue: cockroachdb#141167 Epic: None Release note: None

edwardguo-crl requested review from msbutler and kev-cao February 11, 2025 17:05

kev-cao requested changes Feb 11, 2025

View reviewed changes

edwardguo-crl force-pushed the cc-30018 branch from 1bf628b to c7c940a Compare February 11, 2025 17:35

edwardguo-crl changed the title ~~[CC-30018] Export tenant backup schedule metric~~ [backup] Export tenant backup schedule metric Feb 11, 2025

edwardguo-crl force-pushed the cc-30018 branch from c7c940a to 4c39fda Compare February 11, 2025 20:13

edwardguo-crl changed the title ~~[backup] Export tenant backup schedule metric~~ backup: Export tenant backup schedule metric Feb 11, 2025

edwardguo-crl force-pushed the cc-30018 branch from 4c39fda to 87eeef8 Compare February 11, 2025 20:28

edwardguo-crl force-pushed the cc-30018 branch from 87eeef8 to 039b0d1 Compare February 24, 2025 18:54

edwardguo-crl marked this pull request as ready for review February 24, 2025 18:55

edwardguo-crl requested a review from a team as a code owner February 24, 2025 18:55

edwardguo-crl force-pushed the cc-30018 branch from 039b0d1 to 510fd82 Compare February 24, 2025 18:58

kev-cao reviewed Feb 24, 2025

View reviewed changes

edwardguo-crl force-pushed the cc-30018 branch 3 times, most recently from eb3a930 to 081f236 Compare February 24, 2025 22:01

msbutler reviewed Feb 25, 2025

View reviewed changes

edwardguo-crl force-pushed the cc-30018 branch from 081f236 to 7485eaf Compare February 25, 2025 18:41

edwardguo-crl requested review from a team as code owners February 25, 2025 18:41

edwardguo-crl requested review from a team and removed request for golgeek, arjunmahishi, xinhaoz, mw5h and a team February 25, 2025 21:48

edwardguo-crl force-pushed the cc-30018 branch 4 times, most recently from bd86e18 to e30ec71 Compare February 26, 2025 17:48

backup: Export tenant backup schedule metric

af4d934

Export Cloud tenant backup schedule completion metric with tenant id tag. Issue: cockroachdb#141167 Epic: None Release note: None

edwardguo-crl force-pushed the cc-30018 branch from e30ec71 to af4d934 Compare February 27, 2025 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backup: Export tenant backup schedule metric #141156

backup: Export tenant backup schedule metric #141156

edwardguo-crl commented Feb 11, 2025 •

edited

Loading

blathers-crl bot commented Feb 11, 2025

cockroach-teamcity commented Feb 11, 2025

edwardguo-crl commented Feb 11, 2025

kev-cao Feb 11, 2025

kev-cao Feb 11, 2025

msbutler Feb 11, 2025

kev-cao Feb 11, 2025

kev-cao commented Feb 11, 2025

kev-cao commented Feb 11, 2025 •

edited

Loading

msbutler commented Feb 11, 2025

msbutler commented Feb 11, 2025

kev-cao Feb 24, 2025

msbutler Feb 25, 2025

kev-cao Feb 24, 2025

msbutler Feb 25, 2025

msbutler left a comment

msbutler Feb 25, 2025

msbutler Feb 25, 2025

msbutler Feb 25, 2025

msbutler Feb 25, 2025

msbutler Feb 25, 2025

msbutler Feb 25, 2025

msbutler Feb 25, 2025

edwardguo-crl Feb 25, 2025

backup: Export tenant backup schedule metric #141156

Are you sure you want to change the base?

backup: Export tenant backup schedule metric #141156

Conversation

edwardguo-crl commented Feb 11, 2025 • edited Loading

blathers-crl bot commented Feb 11, 2025

cockroach-teamcity commented Feb 11, 2025

edwardguo-crl commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kev-cao commented Feb 11, 2025

kev-cao commented Feb 11, 2025 • edited Loading

msbutler commented Feb 11, 2025

msbutler commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardguo-crl commented Feb 11, 2025 •

edited

Loading

kev-cao commented Feb 11, 2025 •

edited

Loading