sql: add full table or index scan count metric

[barryhe2000]

Previously, this metric was not available, but is now added so that users can
see when full table scans or full index scans are being used for their queries.

Fixes: #58653

Release note (ui change): User can see time series custom chart in admin ui
for full table or index scans.

[cockroach-teamcity]

This change is 

[yuzefovich]

Nice work! I have some nits, but the code looks good to me in general.

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

---

pkg/sql/conn_executor.go, line 298 at r1 (raw file):

func makeMetrics(internal bool) Metrics {
	// fullTableIndexScan is only instantiated for non-internal queries

nit: missing period.

---

pkg/sql/conn_executor_exec.go, line 917 at r1 (raw file):

	flags := planner.curPlan.flags

	// We don't execute the statement if:

nit: this comment should be moved below, to right before returning an error that stops the statement execution.

---

pkg/sql/exec_util.go, line 478 at r1 (raw file):

	}
	MetaFullTableOrIndexScan = metric.Metadata{
		Name:        "sql.full.table.index.scan.count",

nit: I think I'd use sql.full_index_scan.count ("full table" scan is a "full primary index" scan) or just sql.full_scan.count. Maybe @jordanlewis or @asubiotto have a better suggestion.

---

pkg/sql/executor_statement_metrics.go, line 143 at r1 (raw file):

	FailureCount *metric.Counter

	// FullTableOrIndexScanCount counts the number of full table or index scans.

nit: I'd mention that it is only for user-initiated queries.

[barryhe2000]

Fixed the nits that you mentioned @yuzefovich

[asubiotto]

Nice! Did you manage to find a testing strategy for metrics?

[awoods187]

Mind posting a sample output for a workload that we know about--say TPC-C?

[yuzefovich]

Do we wanna add a dashboard for this metric? I initially thought it wasn't necessary since I assumed that the full table/index scan info is useful only during the debugging, but I'm not sure whether that assumption is correct - it might make sense to have a dashboard for it at the very bottom of SQL dashboard. Thoughts?

[asubiotto]

I'm not convinced it's useful as a SQL dashboard level metric. If everything is well, that graph should flatline at zero, which isn't very interesting. It can currently be accessed through the debug page

[asubiotto]

I think what @awoods187 is saying though, is if we could see the output of this graph (regardless of where it ends up being) for a sample workload.

[barryhe2000]

Metric currently looks something like this

[awoods187]

I actually think we should add this as a chart in the DB Console. Operators often want some measurement of how many scans are occurring at the cluster level--in many cases full table scans are the best option so I wouldn't expect this metric to be zero. I like the way it looks in the custom chart and think we should keep this metric at the bottom of SQL reports.

[asubiotto]

OK. In that case, @barryhe2000 note that you probably want to use a non-negative rate option so the timeseries doesn't show the count since the beginning of time, rather the count in a given time interval.

[yuzefovich]

There are now 4 commits in this PR, we should clean that up so that only one commit remains.

Reviewed 5 of 5 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @barryhe2000)

---

pkg/sql/conn_executor.go, line 904 at r5 (raw file):

	// distSQLMetrics contains distributed metrics to be accounted for.
	distSQLMetrics *execinfra.DistSQLMetrics

This is no longer needed.

[yuzefovich]

Reviewed 1 of 1 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @barryhe2000)

[yuzefovich]

Reviewed 1 of 1 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[barryhe2000]

bors r+

[craig]

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[craig]

GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like.

[yuzefovich]

bors r-

Hm, I'm guessing the bors is complaining about the license/cla check.

Also, I see that you added the chart - I think we discussed putting it at the very bottom of the SQL dashboard, and currently it's in the middle.

[barryhe2000]

CI was failing a test because the chart wasn't there, I can move it. Is there anything doable about the license/cla check? Not sure why it isn't passing

[barryhe2000]

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)