server: create api to query persisted stats by date range by xinhaoz · Pull Request #69238 · cockroachdb/cockroach

xinhaoz · 2021-08-23T14:59:00Z

This commit creates a new API endpoint /_status/combinedstmts
to fetch combined in-memory and persisted statements and
transactions from crdb_internal.statement_statistics
and crdb_internal.transaction_statistics. The request
supports optional start and end parameters which
represent the unix time at which the data was aggregated.

The parameteres start, end and combined have also been
added to the StatementsRequest message. Setting combined
to true will forward the request to fetch data from the
new combined api, using the start and end parameters
provided.

Release justification: Category 2 low-risk updates to new
functionality

Release note (api change): New endpoint /_status/combinedstmts
to retrieve persisted and in-memory statements from
crdb_internal.statement_statistics and
crdb_internal.transaction_statistics by aggregated_ts
range. The request supports optional query string
parameters start and end, which are the date range in unix
time. The response returned is currently the response
expected from /_status/statements.

/_status/statements has also been udpated to support
the parameters combined, start, and end.
If combined is true, then the statements endpoint will
use /_status/combinedstmts with the optional parameters
start and end.

cockroach-teamcity · 2021-08-23T14:59:07Z

This change is

maryliag

Reviewed 1 of 5 files at r1, 4 of 5 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @xinhaoz)

-- commits, line 15 at r3 ([raw file](https://github.com/cockroachdb/cockroach/blob/b7fe6df24ccfd348fcd610b39c6fe4fe1913b6c7/-- commits#L15)):
Release justification: Category 2 low-risk updates to new functionality

pkg/server/persisted_statement_stats.go, line 1 at r3 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: update the file name to be combined_statements_stats

pkg/server/persisted_statement_stats.go, line 41 at r3 (raw file):

	startTime := timeutil.Unix(req.Start, 0)
	endTime := timeutil.Unix(req.End, 0)

nit: remove blank line

pkg/server/persisted_statement_stats.go, line 43 at r3 (raw file):

	statements, err := collectCombinedStatements(ctx, s.internalExecutor, startTime, endTime)

nit: remove blank line

pkg/server/persisted_statement_stats.go, line 61 at r3 (raw file):

func collectCombinedStatements(
	ctx context.Context, ie *sql.InternalExecutor, start time.Time, end time.Time,

we should treat start and end as optional and only add to the query if we have a value for it. This way is more generic and we can use in different cases (e.g. everything before a date, everything after a date, data between two dates and data with no restriction)

pkg/server/persisted_statement_stats.go, line 111 at r3 (raw file):

			Key: serverpb.StatementsResponse_ExtendedStatementStatisticsKey{
				KeyData: metadata.Key,
				NodeID:  roachpb.NodeID(0),

what is this 0 for?

pkg/server/persisted_statement_stats.go, line 118 at r3 (raw file):

		statements = append(statements, stmt)

nit: remove blank line

pkg/server/persisted_statement_stats.go, line 129 at r3 (raw file):

func collectCombinedTransactions(
	ctx context.Context, ie *sql.InternalExecutor, start time.Time, end time.Time,

similar to statements, start and end should be optional and properly handled

pkg/server/persisted_statement_stats.go, line 163 at r3 (raw file):

		app := string(tree.MustBeDString(row[0]))

nit: remove blank line

pkg/server/persisted_statement_stats.go, line 193 at r3 (raw file):

		transactions = append(transactions, stmt)

nit: remove blank line

Azhng

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)

pkg/server/persisted_statement_stats.go, line 129 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

similar to statements, start and end should be optional and properly handled

👍

also nit: you can do

func collectedCombinedTransactions(
  ctx context.Context, ie sqlutil.InternalExecutor, start, end time.Time,
) ...

pkg/server/persisted_statement_stats.go, line 61 at r4 (raw file):

func collectCombinedStatements(
	ctx context.Context, ie *sql.InternalExecutor, start time.Time, end time.Time,

nit: use sqlutils.InternalExecutor interface

pkg/server/persisted_statement_stats.go, line 95 at r4 (raw file):

		var stats roachpb.StatementStatistics
		statsJson := tree.MustBeDJSON(row[1]).JSON

nit: personally i think we should use JSON since it's an acronym

pkg/server/persisted_statement_stats.go, line 134 at r4 (raw file):

	query :=
		`SELECT
				app_name,

nit: master now has fingerprint_id field added to the protobuf message, let's also select that column as well.

pkg/server/persisted_statement_stats.go, line 156 at r4 (raw file):

	var ok bool
	for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) {
		row := it.Cur()

check if row is nil. It can happen for some reason 🤷

pkg/server/persisted_statement_stats.go, line 162 at r4 (raw file):

		}

		app := string(tree.MustBeDString(row[0]))

also check if you have expected number of columns in your tree.Datums

pkg/server/persisted_statement_stats.go, line 168 at r4 (raw file):

		var stats roachpb.TransactionStatistics
		statsJson := tree.MustBeDJSON(row[2]).JSON
		err = sqlstatsutil.DecodeTxnStatsStatisticsJSON(statsJson, &stats)

nit: compact this line and the following lines as:

if err = sqlstatsutil.DecodeTxnStatsStatisticsJSON(statsJSON, &stats); err != nil {
  return nil, err
}

pkg/server/persisted_statement_stats.go, line 174 at r4 (raw file):

		}

		var metadata roachpb.CollectedTransactionStatistics

roachpb.CollectedTransactionStatistics contains both metadata and statistics, you only need to create one object and pass it as a pointer to the decoder. See example.

Also same goes for the handling for roachpb.CollectedStatementStatistics

pkg/server/persisted_statement_stats.go, line 176 at r4 (raw file):

		var metadata roachpb.CollectedTransactionStatistics
		metadataJson := tree.MustBeDJSON(row[3]).JSON
		err = sqlstatsutil.DecodeTxnStatsMetadataJSON(metadataJson, &metadata)

ditto

pkg/server/persisted_statement_stats.go, line 182 at r4 (raw file):

		}

		stmt := serverpb.StatementsResponse_ExtendedCollectedTransactionStatistics{

s/stmt/txnStats

pkg/server/persisted_statement_stats.go, line 196 at r4 (raw file):

	}

	if err != nil {

seems like a redundant error check? Unless I missed something?

pkg/server/persisted_statement_stats.go, line 203 at r4 (raw file):

}

func datumToStmtFingerprintId(datum tree.Datum) (roachpb.StmtFingerprintID, error) {

this method already exists in pkg/sql/sqlstats/persistedsqlstats/stmt_reader.go as a helper function. How about let's refactor this into the pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil package?

xinhaoz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @xinhaoz)

pkg/server/persisted_statement_stats.go, line 61 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

we should treat start and end as optional and only add to the query if we have a value for it. This way is more generic and we can use in different cases (e.g. everything before a date, everything after a date, data between two dates and data with no restriction)

Done.

pkg/server/persisted_statement_stats.go, line 111 at r3 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

what is this 0 for?

I thought it might be necessary on the frontend, but I checked and it doesn't seem to be so removed it.

pkg/server/persisted_statement_stats.go, line 129 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

👍

also nit: you can do

func collectedCombinedTransactions(
  ctx context.Context, ie sqlutil.InternalExecutor, start, end time.Time,
) ...

Done.

pkg/server/persisted_statement_stats.go, line 61 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: use sqlutils.InternalExecutor interface

The internal executor on statusServer uses sql.InternalExecutor so keeping this as is

pkg/server/persisted_statement_stats.go, line 134 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: master now has fingerprint_id field added to the protobuf message, let's also select that column as well.

Done

pkg/server/persisted_statement_stats.go, line 156 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

check if row is nil. It can happen for some reason 🤷

Done.

pkg/server/persisted_statement_stats.go, line 162 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

also check if you have expected number of columns in your tree.Datums

Done.

pkg/server/persisted_statement_stats.go, line 168 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: compact this line and the following lines as:

if err = sqlstatsutil.DecodeTxnStatsStatisticsJSON(statsJSON, &stats); err != nil {
  return nil, err
}

Done.

pkg/server/persisted_statement_stats.go, line 174 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

roachpb.CollectedTransactionStatistics contains both metadata and statistics, you only need to create one object and pass it as a pointer to the decoder. See example.

Also same goes for the handling for roachpb.CollectedStatementStatistics

Done.

pkg/server/persisted_statement_stats.go, line 176 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

ditto

Done.

pkg/server/persisted_statement_stats.go, line 182 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

s/stmt/txnStats

Done.

pkg/server/persisted_statement_stats.go, line 196 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

seems like a redundant error check? Unless I missed something?

Done.

pkg/server/persisted_statement_stats.go, line 203 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

this method already exists in pkg/sql/sqlstats/persistedsqlstats/stmt_reader.go as a helper function. How about let's refactor this into the pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil package?

Done.

ajwerner

drive-by review, didn't look at much

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @xinhaoz)

pkg/server/persisted_statement_stats.go, line 142 at r4 (raw file):

				aggregated_ts >= $1 AND aggregated_ts <= $2`

	it, err := ie.QueryIteratorEx(ctx, "combined-txns-by-interval", nil,

You need to make sure you close this iterator. On some of your early return error paths it won't be closed.

pkg/server/persisted_statement_stats.go, line 156 at r4 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Done.

It really should not happen if ok is true.

maryliag

Reviewed 5 of 7 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @xinhaoz)

pkg/server/combined_statement_stats.go, line 74 at r6 (raw file):

	if start != nil || end != nil {
		buffer.WriteString("WHERE ")
	}

nit: since the default case is not having a start and end, you can add a else and return here (so it won't need to do those extra checks)

Azhng

Reviewed 2 of 7 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @xinhaoz)

pkg/server/combined_statement_stats.go, line 61 at r6 (raw file):

		Statements:            statements,
		Transactions:          transactions,
		LastReset:             timeutil.Now(),

This should use the actual reset time: s.sqlServer.pgServer.SQLServer.GetSQLStatsProvider().GetLastReset()

pkg/server/combined_statement_stats.go, line 74 at r6 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: since the default case is not having a start and end, you can add a else and return here (so it won't need to do those extra checks)

usually we do explicit checks on the early return paths. E.g.

if start == nil && end == nil {
  return "", args
}

pkg/server/combined_statement_stats.go, line 87 at r6 (raw file):

	if end != nil {
		argNum := len(args) + 1
		buffer.WriteString(fmt.Sprintf("aggregated_ts <= $%d", argNum))

minor nit: perhaps just use len(args) + 1 here to simplify things a tiny bit

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/utils.go, line 18 at r6 (raw file):

)

func DatumToUint64(d tree.Datum) (uint64, error) {

nit: comments on exported function or linter will complain

xinhaoz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @xinhaoz)

pkg/server/combined_statement_stats.go, line 74 at r6 (raw file):

Previously, Azhng (Archer Zhang) wrote…

usually we do explicit checks on the early return paths. E.g.
if start == nil && end == nil {
  return "", args
}

Done.

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/utils.go, line 18 at r6 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: comments on exported function or linter will complain

Done

Azhng

Let's write some tests for the new endpoint. Existing test example: TestStatusAPITransactions, TestStatusAPIStatements. Also you want to implement the same thing in the tenantStatusServer in pkg/server/tenant_status.go to support serverless. This is a bit annoying but eventually we will replaced the current statusServer with the new tenantStatusServer.

Reviewed 1 of 5 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)

-- commits, line 4 at r7:
Incomplete release note, also this should be in the last thing in the commit message.

-- commits, line 6 at r7:
nit: /_status/persistedstmts

-- commits, line 7 at r7:
nit: also crdb_internal.transaction_statistics

pkg/server/combined_statement_stats.go, line 61 at r6 (raw file):

Previously, Azhng (Archer Zhang) wrote…

This should use the actual reset time: s.sqlServer.pgServer.SQLServer.GetSQLStatsProvider().GetLastReset()

What about the fix for LastReset field?

pkg/server/combined_statement_stats.go, line 104 at r7 (raw file):

				fingerprint_id,
				metadata,
				statistics

just realized that you also want to select the plan columns from the system table. See example

Also we want to select the aggregated_ts column as well

pkg/server/combined_statement_stats.go, line 123 at r7 (raw file):

	var statements []serverpb.StatementsResponse_CollectedStatementStatistics
	var ok bool
	for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) {

nit: this err is not checked

pkg/server/combined_statement_stats.go, line 198 at r7 (raw file):

	var transactions []serverpb.StatementsResponse_ExtendedCollectedTransactionStatistics
	var ok bool
	for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) {

ditto

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/utils.go, line 18 at r6 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Done

super nit: period in the end.

maryliag

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @xinhaoz)

pkg/server/status_test.go, line 1467 at r12 (raw file):

		{
			query:         `INSERT INTO posts VALUES (1, 'foo')`,
			fingerprinted: `INSERT INTO posts VALUES (_, _)`,

INSERT INTO posts VALUES (_, '_')

maryliag

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @xinhaoz)

pkg/server/combined_statement_stats.go, line 228 at r12 (raw file):

		app := string(tree.MustBeDString(row[0]))
		aggregatedTs := tree.MustBeDTimestampTZ(row[1]).Time
		fingerprintID, err := sqlstatsutil.DatumToUint64(row[2])

you're not checking for the error here

Azhng

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @Azhng, @maryliag, and @xinhaoz)

pkg/server/status_test.go, line 1444 at r16 (raw file):

	defer log.Scope(t).Close(t)

	testCluster := serverutils.StartNewTestCluster(t, 3, base.TestClusterArgs{})

Spin up test cluster using:

params, _ := tests.CreateTestServerParams()

cluster := serverutils.StartNewTestCluster(t, 3 /* numNodes */, base.TestClusterArgs{
	ServerArgs: params,
})

This should fix the CI errors for "descriptor not found".

Same goes for the tenant tests.

Explanation: the tests.CreateTestServerParams() overrides the AS OF SYSTEM TIME clause in the read statement for reading SQL stats from the system table. See: #69346.

This commit creates a new API endpoint /_status/combinedstmts to fetch combined in-memory and persisted statements and transactions from crdb_internal.statement_statistics and crdb_internal.transaction_statistics. The request supports optional start and end parameters which represent the unix time at which the data was aggregated. The parameteres start, end and combined have also been added to the StatementsRequest message. Setting combined to true will forward the request to fetch data from the new combined api, using the start and end parameters provided. Release justification: Category 2 low-risk updates to new functionality Release note (api change): New endpoint /_status/combinedstmts to retrieve persisted and in-memory statements from crdb_internal.statement_statistics and crdb_internal.transaction_statistics by aggregated_ts range. The request supports optional query string parameters start and end, which are the date range in unix time. The response returned is currently the response expected from /_status/statements. /_status/statements has also been udpated to support the parameters combined, start, and end. If combined is true, then the statements endpoint will use /_status/combinedstmts with the optional parameters start and end.

maryliag

Reviewed 1 of 2 files at r14, 1 of 2 files at r15, 5 of 7 files at r16, 2 of 2 files at r18, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @xinhaoz)

pkg/server/status_test.go, line 1554 at r18 (raw file):

		}

		// Sanity check numeric stat values

nit: period

pkg/server/status_test.go, line 1859 at r18 (raw file):

	}

	// Test no params

nit: period

pkg/server/status_test.go, line 1861 at r18 (raw file):

	// Test no params
	testPath("statements", expectedStatements)
	// Test combined=true forwards to CombinedStatements

nit: period

pkg/server/status_test.go, line 1951 at r18 (raw file):

	}

	// Test with no query params

nit: period

xinhaoz · 2021-08-27T17:08:44Z

bors r+

craig · 2021-08-27T18:37:14Z

This PR was included in a batch that was canceled, it will be automatically retried

craig · 2021-08-27T19:58:38Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-08-27T21:10:51Z

This PR was included in a batch that was canceled, it will be automatically retried

craig · 2021-08-28T06:58:02Z

Build succeeded:

GitHub CI (Cockroach)

xinhaoz force-pushed the persisted-stmts-api branch 4 times, most recently from 22d2b5d to 91d893c Compare August 23, 2021 18:20

xinhaoz marked this pull request as ready for review August 23, 2021 18:24

xinhaoz requested a review from a team as a code owner August 23, 2021 18:24

xinhaoz requested review from a team and Azhng August 23, 2021 18:25

Azhng mentioned this pull request Aug 23, 2021

sql: persistent SQL Stats main tracking issue #64743

Closed

24 tasks

xinhaoz force-pushed the persisted-stmts-api branch 3 times, most recently from f801e7c to b7fe6df Compare August 23, 2021 21:11

maryliag suggested changes Aug 23, 2021

View reviewed changes

xinhaoz force-pushed the persisted-stmts-api branch from b7fe6df to 8f5850f Compare August 23, 2021 21:57

Azhng suggested changes Aug 23, 2021

View reviewed changes

xinhaoz force-pushed the persisted-stmts-api branch from 8f5850f to c828a49 Compare August 24, 2021 13:36

xinhaoz commented Aug 24, 2021

View reviewed changes

xinhaoz requested review from Azhng and maryliag August 24, 2021 13:38

ajwerner reviewed Aug 24, 2021

View reviewed changes

xinhaoz force-pushed the persisted-stmts-api branch from c828a49 to f050d53 Compare August 24, 2021 13:48

maryliag reviewed Aug 24, 2021

View reviewed changes

Azhng reviewed Aug 24, 2021

View reviewed changes

xinhaoz force-pushed the persisted-stmts-api branch from f050d53 to 70a8090 Compare August 24, 2021 15:46

xinhaoz commented Aug 24, 2021

View reviewed changes

xinhaoz requested review from Azhng and maryliag August 24, 2021 15:53

Azhng reviewed Aug 24, 2021

View reviewed changes

xinhaoz force-pushed the persisted-stmts-api branch from 70a8090 to 1eb6ebc Compare August 24, 2021 18:04

maryliag reviewed Aug 26, 2021

View reviewed changes

xinhaoz force-pushed the persisted-stmts-api branch 10 times, most recently from e5cc53a to bbbf2ea Compare August 27, 2021 02:06

Azhng reviewed Aug 27, 2021

View reviewed changes

xinhaoz force-pushed the persisted-stmts-api branch from bbbf2ea to a712beb Compare August 27, 2021 13:43

xinhaoz force-pushed the persisted-stmts-api branch from a712beb to 63eb73a Compare August 27, 2021 15:23

maryliag reviewed Aug 27, 2021

View reviewed changes

craig bot merged commit b8223fb into cockroachdb:master Aug 28, 2021

cockroach-teamcity mentioned this pull request Aug 28, 2021

server: create api to query persisted stats by date range cockroachdb/docs#11159

Closed

xinhaoz deleted the persisted-stmts-api branch August 30, 2021 15:17

jseldess mentioned this pull request Sep 8, 2021

server: create api to query persisted stats by date range cockroachdb/docs#11539

Closed

Conversation

xinhaoz commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Aug 23, 2021

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng left a comment

Choose a reason for hiding this comment

Uh oh!

xinhaoz left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng left a comment

Choose a reason for hiding this comment

Uh oh!

xinhaoz left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng left a comment

Choose a reason for hiding this comment

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng left a comment

Choose a reason for hiding this comment

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

xinhaoz commented Aug 27, 2021

Uh oh!

craig bot commented Aug 27, 2021

Uh oh!

craig bot commented Aug 27, 2021

Uh oh!

craig bot commented Aug 27, 2021

Uh oh!

craig bot commented Aug 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xinhaoz commented Aug 23, 2021 •

edited

Loading