sql: add read/write metrics

[andy-kimball]

sql: add index_bytes_written metric

Add new sql.statements.index_bytes_written.count metric that counts the number
of primary and secondary index bytes modified by SQL statements.

sql: add index_rows_written metric

Add new sql.statements.index_rows_written.count metric that counts the number
of primary and secondary index rows modified by SQL statements.

sql: add bytes_read metric

Add new sql.statements.bytes_read.count metric that counts the number of
bytes scanned by SQL statements. This is the same value that's collected by
SQL stats for each statement and transaction, except in aggregated metric
form.

[cockroach-teamcity]

This change is 

[yuzefovich]

Nice! I only have some nits.

Thanks for cleaning up the stale comments! Let's also adjust the stale comment in ValuesLegalForInsertFastPath.

Currently there are some failures about generating cockroachdb_metrics.go - I hope we can avoid that via #156335.

@yuzefovich reviewed 5 of 5 files at r1, 19 of 19 files at r2, 19 of 19 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian)

---

pkg/sql/tablewriter.go line 149 at r2 (raw file):

	// The mutation operators add one request to the KV batch for each index
	// entry that's written.
	tb.indexRowsWritten += int64(len(tb.b.Requests()))

nit: for symmetry with other spots let's modify rowsWritten before indexRowsWritten here and below.

---

pkg/sql/metric_test.go line 313 at r2 (raw file):

						s, sql.MetaStatementIndexRowsWritten, lastRowsWritten, tc.indexRowsWritten)
					require.NoError(t, err)
					lastRowsWritten = actual

Shouldn't we compare lastRowsWritten to actual here?

---

pkg/sql/execinfrapb/data.proto line 347 at r2 (raw file):

    // IndexRowsWritten is the number of primary and secondary index rows
    // modified while executing a statement. It is always >= RowsWritten.
    optional int64 index_rows_written = 13 [(gogoproto.nullable) = false];

nit: this can use ID 5 since it's internal to Metrics protobuf - "outer" IDs are only applicable to oneof value part. Ditto for index_bytes_written.

---

pkg/sql/exec_util.go line 1539 at r3 (raw file):

		Help:        "Number of primary and secondary index bytes modified by SQL statements",
		Measurement: "SQL Statements",
		Unit:        metric.Unit_COUNT,

nit: we usually use metric.Unit_BYTES for metrics that are in bytes.

---

pkg/sql/conn_executor_exec.go line 3291 at r2 (raw file):

	// indexRowsWritten is the number of rows written to primary and secondary
	// indexes. It is always >= rowsWritten.
	indexRowsWritten int64

nit: consider extending recently added TestTopLevelQueryStats a bit to cover indexRowsWritten as well.

---

pkg/sql/conn_executor_exec.go line 3291 at r3 (raw file):

	// indexBytesWritten is the number of bytes written to primary and secondary
	// indexes.
	indexBytesWritten int64

nit: maybe place indexBytesWritten after indexRowsWritten for consistency.

[andy-kimball]

I was going to update the ValuesLegalForInsertFastPath comment in a different PR, since it's actually an orthogonal issue that I just happened to discover while working on this PR.

[andy-kimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @nameisbhaskar, @williamchoe3, and @yuzefovich)

---

pkg/sql/conn_executor_exec.go line 3291 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: consider extending recently added TestTopLevelQueryStats a bit to cover indexRowsWritten as well.

Done

---

pkg/sql/conn_executor_exec.go line 3291 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: maybe place indexBytesWritten after indexRowsWritten for consistency.

heh, I placed it before to be consistent with how bytesRead and rowsRead are ordered. I can change those as well, though.

---

pkg/sql/exec_util.go line 1539 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: we usually use metric.Unit_BYTES for metrics that are in bytes.

Hm, I'd fixed that, but must have neglected to push. Fixed now.

---

pkg/sql/tablewriter.go line 149 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: for symmetry with other spots let's modify rowsWritten before indexRowsWritten here and below.

Done

---

pkg/sql/execinfrapb/data.proto line 347 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: this can use ID 5 since it's internal to Metrics protobuf - "outer" IDs are only applicable to oneof value part. Ditto for index_bytes_written.

true, fixed

---

pkg/sql/metric_test.go line 313 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Shouldn't we compare lastRowsWritten to actual here?

The comparisons are actually done inside checkCounterDelta. actual is actually no longer needed, so I removed.

[yuzefovich]

, just one comment on the test adjustment.

Updating the ValuesLegalForInsertFastPath comment separately SGTM.

@yuzefovich reviewed 19 of 19 files at r4, 18 of 19 files at r5, 21 of 21 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @nameisbhaskar, and @williamchoe3)

---

pkg/sql/distsql_running_test.go line 1229 at r6 (raw file):

	// The callback will send number of rows read and rows written (for each
	// ProducerMetadata.Metrics object) on these channels, respectively.
	rowsReadCh, rowsWrittenCh := make(chan int64), make(chan int64)

We'll need a bit more adjustments here for indexRowsWritten - creating a channel, send'ing and recv'ing from the channel, and then asserting the sum equals the expected count.

[andy-kimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @nameisbhaskar, @williamchoe3, and @yuzefovich)

---

pkg/sql/distsql_running_test.go line 1229 at r6 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

We'll need a bit more adjustments here for indexRowsWritten - creating a channel, send'ing and recv'ing from the channel, and then asserting the sum equals the expected count.

whoops, i got distracted and didn't finish that, fixed now

[andy-kimball]

bors r=yuzefovich

[craig]

Build succeeded:

• examples_orms
• local_roachtest
• linux_amd64_build
• acceptance
• docker_image_amd64
• local_roachtest_fips
• check_generated_code
• linux_amd64_fips_build
• unit_tests
• lint