sql: add metrics to schema changer

[ajwerner]

It's far from exhaustive and hardly tested but something is better than
nothing.

Touches #52078

Release note (general change): Added some metrics surrounding schema changes.

[cockroach-teamcity]

This change is 

[thoszhang]

Thanks for picking this up.

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

---

pkg/sql/schema_changer.go, line 557 at r1 (raw file):

// no-op, as we'll fail to find the job for our mutation in the jobs registry.
func (sc *SchemaChanger) exec(ctx context.Context) error {
	sc.metrics.RunningSchemaChanges.Inc(1)

I think this should wrap the entirety of Resume (in which case we'd want a separate metric for ongoing OnFailOrCancel rollbacks, I think).

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)

---

pkg/sql/schema_changer.go, line 557 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I think this should wrap the entirety of Resume (in which case we'd want a separate metric for ongoing OnFailOrCancel rollbacks, I think).

Fair enough. I wanted to add a gauge on schema changes in retry backoff but was annoyed by what it would take to test it so I removed it. I was trying to make those two states different. I suppose it might make more sense to do what you're suggesting and then additionally have a gauge to deal with the retry case. I'll add a testing knob to make testing that reasonable.

[thoszhang]

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

---

pkg/sql/schema_changer.go, line 557 at r1 (raw file):

Previously, ajwerner wrote…

Fair enough. I wanted to add a gauge on schema changes in retry backoff but was annoyed by what it would take to test it so I removed it. I was trying to make those two states different. I suppose it might make more sense to do what you're suggesting and then additionally have a gauge to deal with the retry case. I'll add a testing knob to make testing that reasonable.

I was more concerned with getting all the invocations of exec() in the resumer being represented as a single logical schema change in the metrics. But I guess it really actually depends on what we want to measure. Do we want DROP DATABASE CASCADE to be a single schema change or not? I find it hard to predict exactly what metrics would be most useful here.

[thoszhang]

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

---

pkg/sql/schema_changer.go, line 557 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I was more concerned with getting all the invocations of exec() in the resumer being represented as a single logical schema change in the metrics. But I guess it really actually depends on what we want to measure. Do we want DROP DATABASE CASCADE to be a single schema change or not? I find it hard to predict exactly what metrics would be most useful here.

Maybe the more comprehensive solution is that the job callbacks get per-job-type gauges at the job registry level (or something), and we keep schema-specific gauges for individual calls to exec(), so it would basically be a per-descriptor gauge. I'm actually fine with this PR as is.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)

---

pkg/sql/schema_changer.go, line 557 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Maybe the more comprehensive solution is that the job callbacks get per-job-type gauges at the job registry level (or something), and we keep schema-specific gauges for individual calls to exec(), so it would basically be a per-descriptor gauge. I'm actually fine with this PR as is.

I've added #55438 to deal with general metrics around jobs which makes this more sane. In light of that, I'll keep this as is.

[thoszhang]

sgtm

Reviewed 6 of 6 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

[ajwerner]

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained

[craig]

Build succeeded:

• GitHub CI (Cockroach)