sql: Add telemetry for some partitioning UX improvements

[rohany]

Add telemetry for the following operations:

• show partitions
• show ranges
• show locality
• show create table (intending to capture usage of new partitioning info)

Work towards https://github.com/cockroachlabs/registration/issues/228

Release justification: Low risk improvement to monitoring.

Release note: None

[cockroach-teamcity]

This change is 

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @rohany, and @solongordon)

---

pkg/sql/sqltelemetry/show.go, line 19 at r1 (raw file):

)

var showTelemetryCounters map[string]telemetry.Counter

this might be a little safer with an enum type as the key (with a String() method) instead of a string key. that way if it's used in different contexts it's harder to mess up the key name

---

pkg/sql/sqltelemetry/show.go, line 29 at r1 (raw file):

		telemetry.Inc(counter)
	} else {
		showTelemetryCounters[showType] = telemetry.GetCounterOnce(fmt.Sprintf("sql.show.%s", showType))

I think theoretically there is a race condition here, since two threads could be calling IncrementShowCounter for the same string, and both could end up in this branch. GetCounterOnce will panic if the counter exists

I think it would be safest to do all the GetCounterOnce calls in the init function. for example, take a look at pkg/sql/opt/telemetry.go

That would also be easier to do if there were an enum type and the init function just iterates over the enum values.

[rohany]

Good catches @rafiss. I wanted to have this so that you could just add different telemetry counters without having to touch show.go, but it seems better to explicitly have to add an enum to opt in.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @solongordon)

---

pkg/sql/sqltelemetry/show.go, line 19 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this might be a little safer with an enum type as the key (with a String() method) instead of a string key. that way if it's used in different contexts it's harder to mess up the key name

Done.

---

pkg/sql/sqltelemetry/show.go, line 29 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

I think theoretically there is a race condition here, since two threads could be calling IncrementShowCounter for the same string, and both could end up in this branch. GetCounterOnce will panic if the counter exists

I think it would be safest to do all the GetCounterOnce calls in the init function. for example, take a look at pkg/sql/opt/telemetry.go

That would also be easier to do if there were an enum type and the init function just iterates over the enum values.

Done.

[rafiss]

looks great!

Reviewed 3 of 8 files at r1, 5 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @solongordon)

[rohany]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)