sql: add crdb_internal.reset_sql_stats() builtin #62175

Azhng · 2021-03-17T22:01:20Z

Previously, there was no mechanism to immediately clear SQL statistics. Users
would have to wait until the reset interval expires. This commit creates a
builtin to immediately clears SQL stats.

Release note (sql change): SQL stats can now be cleared using
crdb_internal.reset_sql_stats()

Addresses #33315

cockroach-teamcity · 2021-03-17T22:01:26Z

This change is

Azhng · 2021-03-18T03:59:50Z

Couldn't recreate the CI failures both locally and on GCE worker. Will take another look tomorrow.

Azhng · 2021-03-18T17:17:34Z

Fixed the test. CI is green. Ready for review

asubiotto

Nice! @maryliag would be great if you could review as well even if it's just to ask questions.

Reviewed 10 of 15 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng)

docs/generated/http/full.md, line 2839 at r1 (raw file):

## SQLStatisticsReset

`POST /_status/sqlstatisticsreset`

I would change the name here to be more in line with the builtin. It also sounds more "active", i.e. "resetsqlstats" instead of "sqlstatisticsreset". I also think it's worth it to shorten statistics to stats here and elsewhere.

docs/generated/http/full.md, line 2873 at r1 (raw file):

| Field | Type | Label | Description | Support status |
| ----- | ---- | ----- | ----------- | -------------- |
| num_of_nodes_resetted | [int32](#cockroach.server.serverpb.SQLStatisticsResetResponse-int32) |  | Number of nodes that have resetted their SQL stats. | [reserved](#support-status) |

nit: here and elsewhere s/resetted/reset.

Also, why return the number of nodes reset? Is it used anywhere?

docs/generated/sql/functions.md, line 2717 at r1 (raw file):

<tr><td><a name="crdb_internal.range_stats"></a><code>crdb_internal.range_stats(key: <a href="bytes.html">bytes</a>) &rarr; jsonb</code></td><td><span class="funcdesc"><p>This function is used to retrieve range statistics information as a JSON object.</p>
</span></td></tr>
<tr><td><a name="crdb_internal.reset_sql_statistics"></a><code>crdb_internal.reset_sql_statistics() &rarr; <a href="bool.html">bool</a></code></td><td><span class="funcdesc"><p>This function can be used to clear the collected SQL-related statistics.</p>

nit: s/can be used/is used and s/SQL-related/SQL

pkg/server/sql_stats.go, line 23 at r1 (raw file):

)

func (s *statusServer) SQLStatisticsReset(

cc @yuzefovich when you added the contention API you mentioned a v2 status server, should this method be implemented there?

As mentioned before, I think it'd be nicer to s/SQLStatisticsReset/ResetSQLStats

pkg/server/sql_stats.go, line 72 at r1 (raw file):

			response.NumOfNodesResetted++
		},
		func(nodeID roachpb.NodeID, err error) {

Empty fn?

pkg/server/stats_test.go, line 264 at r1 (raw file):

func resetClusterStats(ctx context.Context, cluster serverutils.TestClusterInterface) {
	// Flush stats at the beginning of the test.

nit: remove comment, I think this belongs outside the function (as you've done)

pkg/server/stats_test.go, line 287 at r1 (raw file):

	// Flush stats at the beginning of the test.
	resetClusterStats(ctx, testCluster)

Why do the stats need to be reset at the start?

pkg/server/stats_test.go, line 300 at r1 (raw file):

	statsPreReset, err := status.Statements(ctx, &serverpb.StatementsRequest{})
	if err != nil {
		t.Fatal(err)

Consider using require (e.g. require.NoError in this case) to help with vebosity in this test.

pkg/server/tenant_status.go, line 119 at r1 (raw file):

) (*serverpb.SQLStatisticsResetResponse, error) {
	// TODO(azhng): Currently a tenant status server only reset its local SQL stats,
	// 	this needs to be updated once the pod-to-pod communication is implemented.

I think it'd be nice to add a top-level comment about this rather than just have it in this function.

pkg/server/testserver.go, line 763 at r1 (raw file):

	// Since SQLStatusServer requires a pointer to SQLServer, it is constructed after SQLServer is
	// constructed and retrospectively added to SQLServer's execCfg.

nit: I would just delete this comment.

pkg/server/serverpb/status.go, line 28 at r1 (raw file):

	ListContentionEvents(context.Context, *ListContentionEventsRequest) (*ListContentionEventsResponse, error)
	ListLocalContentionEvents(context.Context, *ListContentionEventsRequest) (*ListContentionEventsResponse, error)
	SQLStatisticsReset(ctx context.Context, request *SQLStatisticsResetRequest) (*SQLStatisticsResetResponse, error)

cc @Azhng in a follow up PR, could you add the actual stats endpoints? This should support us displaying stats in a tenant's DB Console. If it doesn't work it'd be nice to know this sooner rather than later cc @jordanlewis

pkg/sql/sem/tree/eval.go, line 3271 at r1 (raw file):

// SQLStatisticsResetter is an interface embedded in EvalCtx which can be used by
// the builtins to reset SQL stats.
type SQLStatisticsResetter interface {

nit: s/SQLStatisticsResetter/SQLStatsResetter

pkg/sql/sem/tree/eval.go, line 3272 at r1 (raw file):

// the builtins to reset SQL stats.
type SQLStatisticsResetter interface {
	SQLStatsReset(ctx context.Context) error

Nice.

s/SQLStatsReset/ResetSQLStats and comment that the interface exists to avoid a dependency

maryliag

Just a few nit picking

docs/generated/http/full.md

pkg/server/serverpb/status.proto

pkg/server/tenant_status.go

pkg/server/stats_test.go

Azhng

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

docs/generated/http/full.md, line 2873 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: here and elsewhere s/resetted/reset.

Also, why return the number of nodes reset? Is it used anywhere?

My thoughts here is that since resetting the stats requiring fan-out RPCs to all nodes in the cluster, it is possible for number of RPCs to fail for various reasons.
If the reset operation only partially failed, it is difficult to communicate the partial failure using a simple boolean. Hence I opted for a numeric value here.
But yes I should probably change the return value of the builtin from boolean to an int value.

pkg/server/sql_stats.go, line 72 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Empty fn?

It is empty because I think iterateNodes does not check for nil before calling errorFn. I was using statement.go as a reference, and seems like it simply ignored the error if the RPC fails. Since the result of RPC is communicate through number of nodes where reset operations were successful, I don't think additional error handling logic is required here.

Though I think the alternative is to modify the iterateNodes to check for nil before calling errorFn. I don't have particular preference for either, what's your thoughts on this?

yuzefovich

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

pkg/server/sql_stats.go, line 23 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

cc @yuzefovich when you added the contention API you mentioned a v2 status server, should this method be implemented there?

As mentioned before, I think it'd be nicer to s/SQLStatisticsReset/ResetSQLStats

My thinking is that for now it is ok to implement things as v1 only, but soon (in 21.2 release) we'll need to implement everything as v2, so it's up to Archer to decide when to do it.

Azhng

@asubiotto can you PTAL ?

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

docs/generated/http/full.md, line 2839 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I would change the name here to be more in line with the builtin. It also sounds more "active", i.e. "resetsqlstats" instead of "sqlstatisticsreset". I also think it's worth it to shorten statistics to stats here and elsewhere.

Done.

docs/generated/http/full.md, line 2873 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

reset, not resetted

Done.

pkg/server/sql_stats.go, line 23 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

My thinking is that for now it is ok to implement things as v1 only, but soon (in 21.2 release) we'll need to implement everything as v2, so it's up to Archer to decide when to do it.

Done.

pkg/server/stats_test.go, line 287 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Why do the stats need to be reset at the start?

Hmm seems like it's not needed. Removed

pkg/server/stats_test.go, line 300 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Consider using require (e.g. require.NoError in this case) to help with vebosity in this test.

Done.

pkg/server/stats_test.go, line 313 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

the previous test had but found: and this one doesn't have the :. Change one of them so they're the same

Done.

pkg/server/tenant_status.go, line 119 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I think it'd be nice to add a top-level comment about this rather than just have it in this function.

Done.

pkg/server/tenant_status.go, line 122 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

NumOfNodesReset

Done.

pkg/server/testserver.go, line 763 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: I would just delete this comment.

Removed.

pkg/server/serverpb/status.go, line 28 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

cc @Azhng in a follow up PR, could you add the actual stats endpoints? This should support us displaying stats in a tenant's DB Console. If it doesn't work it'd be nice to know this sooner rather than later cc @jordanlewis

Sounds good, will send a follow up PR after this.

pkg/server/serverpb/status.proto, line 1020 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

comment and variable name (make sure to fix all places this variable is called): reset and not resetted

Done.

pkg/sql/sem/tree/eval.go, line 3272 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Nice.

s/SQLStatsReset/ResetSQLStats and comment that the interface exists to avoid a dependency

ResetSQLStats is already a function in conn_executor.go. I suppose I can rename this to ResetClusterSQLStats.

asubiotto

Reviewed 2 of 13 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @Azhng, and @maryliag)

pkg/server/sql_stats.go, line 72 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

It is empty because I think iterateNodes does not check for nil before calling errorFn. I was using statement.go as a reference, and seems like it simply ignored the error if the RPC fails. Since the result of RPC is communicate through number of nodes where reset operations were successful, I don't think additional error handling logic is required here.

Though I think the alternative is to modify the iterateNodes to check for nil before calling errorFn. I don't have particular preference for either, what's your thoughts on this?

So why not use this mechanism instead of returning the number of nodes in the response? I think it's not good usability to have the NumNodesReset field because you're just pushing the error handling logic one layer up. The caller shouldn't have to care or know the details about how this RPC is executed. I think what you should do is remove that field and instead combine errors in this function and return an error as normal.

asubiotto

Reviewed 1 of 13 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @Azhng, and @maryliag)

pkg/server/tenant_status.go, line 119 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Done.

Sorry, I meant a top-level file comment. i.e. all functions need to be updated once we have pod-to-pod communication. Even better, we probably would remove this tenant status server entirely and use the normal status server once we remove any dependencies unavailable to tenants.

Azhng · 2021-03-23T18:38:33Z

Appears to be a flake.

Azhng

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

pkg/server/sql_stats.go, line 72 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

So why not use this mechanism instead of returning the number of nodes in the response? I think it's not good usability to have the NumNodesReset field because you're just pushing the error handling logic one layer up. The caller shouldn't have to care or know the details about how this RPC is executed. I think what you should do is remove that field and instead combine errors in this function and return an error as normal.

Changed the response to have an array of Error protobuf objects.

pkg/server/tenant_status.go, line 119 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Sorry, I meant a top-level file comment. i.e. all functions need to be updated once we have pod-to-pod communication. Even better, we probably would remove this tenant status server entirely and use the normal status server once we remove any dependencies unavailable to tenants.

Done

asubiotto

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

pkg/server/sql_stats.go, line 72 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Changed the response to have an array of Error protobuf objects.

What I meant is use errors.CombineErrors and return the result as an error here (i.e. nil, err) this will return the error as a GRPC error

Azhng

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

pkg/server/sql_stats.go, line 72 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

What I meant is use errors.CombineErrors and return the result as an error here (i.e. nil, err) this will return the error as a GRPC error

Done.

asubiotto

Reviewed 1 of 15 files at r1, 1 of 13 files at r2, 10 of 10 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag)

Azhng · 2021-03-24T15:35:06Z

TFTR!

bors r=asubiotto

craig · 2021-03-24T16:36:38Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-03-24T17:41:53Z

Merge conflict.

Previously, there was no mechanism to immediately clear SQL statistics. Users would have to wait until the reset interval expires. This commit creates a builtin to immediately clears SQL stats. Release note (sql change): SQL stats can now be cleared using crdb_internal.reset_sql_stats()

Azhng · 2021-03-24T20:50:41Z

bors r=asubiotto

craig · 2021-03-24T23:07:25Z

Build succeeded:

GitHub CI (Cockroach)

rafiss · 2021-03-25T01:34:09Z

Just passing by and this looks great! Nice work.

I noticed that the PR description says Addresses #33315 -- just as a heads up, that will not automatically close the issue. If you use Closes #33315 or Fixes #33315, then it would. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

Azhng · 2021-03-25T02:41:33Z

Hi @rafiss , thanks for the heads up. Though this PR only partially addressed the issue by adding a builtin and an RPC endpoint, we are still working on adding the frontend components that will allow uses to reset SQL stats from db console.

rafiss · 2021-03-25T02:54:36Z

Gotcha! I was wondering if it was on purpose but just wanted to point it out in case

Azhng requested a review from asubiotto March 17, 2021 22:01

Azhng requested review from a team as code owners March 17, 2021 22:01

Azhng force-pushed the reset-stats branch from 634c5b0 to 49a93c3 Compare March 18, 2021 00:57

Azhng force-pushed the reset-stats branch from 49a93c3 to 6b178cd Compare March 18, 2021 16:10

asubiotto suggested changes Mar 19, 2021

View reviewed changes

maryliag reviewed Mar 19, 2021

View reviewed changes

docs/generated/http/full.md Outdated Show resolved Hide resolved

pkg/server/serverpb/status.proto Outdated Show resolved Hide resolved

pkg/server/tenant_status.go Outdated Show resolved Hide resolved

pkg/server/stats_test.go Outdated Show resolved Hide resolved

Azhng commented Mar 19, 2021

View reviewed changes

Azhng force-pushed the reset-stats branch 4 times, most recently from efd0ee9 to 1d86a2b Compare March 22, 2021 18:22

Azhng changed the title ~~sql: add crdb_internal.reset_sql_statistics() builtin~~ sql: add crdb_internal.reset_sql_stats() builtin Mar 22, 2021

yuzefovich reviewed Mar 22, 2021

View reviewed changes

Azhng force-pushed the reset-stats branch from 1d86a2b to 9c6dbf0 Compare March 22, 2021 19:12

Azhng commented Mar 22, 2021

View reviewed changes

asubiotto suggested changes Mar 23, 2021

View reviewed changes

asubiotto reviewed Mar 23, 2021

View reviewed changes

Azhng force-pushed the reset-stats branch from 9c6dbf0 to 1ca7a4b Compare March 23, 2021 17:29

Azhng commented Mar 23, 2021

View reviewed changes

asubiotto reviewed Mar 23, 2021

View reviewed changes

Azhng force-pushed the reset-stats branch from 1ca7a4b to 74739c0 Compare March 23, 2021 19:18

Azhng commented Mar 23, 2021

View reviewed changes

asubiotto approved these changes Mar 24, 2021

View reviewed changes

Azhng force-pushed the reset-stats branch from 74739c0 to 700013f Compare March 24, 2021 17:52

craig bot merged commit 8aace6d into cockroachdb:master Mar 24, 2021

Azhng linked an issue Mar 25, 2021 that may be closed by this pull request

roachtest: sqlsmith: crdb_internal.reset_sql_stats(): sql stats resetter not set #62587

Closed

Azhng added the backport-21.1.x label Mar 25, 2021

rafiss added this to the 21.1 milestone Apr 22, 2021

This was referenced May 21, 2021

release-21.1: sql: add crdb_internal.reset_sql_stats() builtin #65566

Closed

release-21.1: backport crdb_internal.reset_sql_stats #65674

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add crdb_internal.reset_sql_stats() builtin #62175

sql: add crdb_internal.reset_sql_stats() builtin #62175

Azhng commented Mar 17, 2021 •

edited

Loading

cockroach-teamcity commented Mar 17, 2021

Azhng commented Mar 18, 2021

Azhng commented Mar 18, 2021

asubiotto left a comment

maryliag left a comment

Azhng left a comment

yuzefovich left a comment

Azhng left a comment

asubiotto left a comment

asubiotto left a comment

Azhng commented Mar 23, 2021

Azhng left a comment

asubiotto left a comment

Azhng left a comment

asubiotto left a comment

Azhng commented Mar 24, 2021

craig bot commented Mar 24, 2021

craig bot commented Mar 24, 2021

Azhng commented Mar 24, 2021

craig bot commented Mar 24, 2021

rafiss commented Mar 25, 2021

Azhng commented Mar 25, 2021

rafiss commented Mar 25, 2021

sql: add crdb_internal.reset_sql_stats() builtin #62175

sql: add crdb_internal.reset_sql_stats() builtin #62175

Conversation

Azhng commented Mar 17, 2021 • edited Loading

cockroach-teamcity commented Mar 17, 2021

Azhng commented Mar 18, 2021

Azhng commented Mar 18, 2021

asubiotto left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

asubiotto left a comment

Choose a reason for hiding this comment

asubiotto left a comment

Choose a reason for hiding this comment

Azhng commented Mar 23, 2021

Azhng left a comment

Choose a reason for hiding this comment

asubiotto left a comment

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

asubiotto left a comment

Choose a reason for hiding this comment

Azhng commented Mar 24, 2021

craig bot commented Mar 24, 2021

craig bot commented Mar 24, 2021

Azhng commented Mar 24, 2021

craig bot commented Mar 24, 2021

rafiss commented Mar 25, 2021

Azhng commented Mar 25, 2021

rafiss commented Mar 25, 2021

Azhng commented Mar 17, 2021 •

edited

Loading