sql: add metric for queries run with optimizer #26981

justinj · 2018-06-26T14:47:25Z

Suggested by @knz.

Release note (sql change): A new metric sql.optimizer.count has been
added which tracks the number of queries with with the experimental
cost-based optimizer.

cockroach-teamcity · 2018-06-26T14:47:33Z

This change is

knz · 2018-06-27T10:03:27Z

This is nice. Thanks!

While you're at it, perhaps print out the value of the metric at the end of logic tests with t.Log()?

Reviewed 6 of 6 files at r1.
Review status: complete! 0 of 0 LGTMs obtained

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

	if optUsed {
		m.SQLOptCount.Inc(1)

Move this below automaticRetryCount == 0 - we only count once, upon the first execution attempt.

Comments from Reviewable

knz · 2018-06-27T12:00:31Z

Review status: complete! 0 of 0 LGTMs obtained

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

Previously, knz (kena) wrote…

Move this below automaticRetryCount == 0 - we only count once, upon the first execution attempt.

Oh hm. I realize this last comment is based on an undocumented assumption. I think it would be good that you satisfy yourself that:

this method (recordStatementSummary) is indeed called again on every attempt, with an increasing value of automaticRetryCount every time (perhaps you'll want to add a comment above the function and under the conditional below to explain this fully to future readers)
you indeed only want to count unique issues of the queries with opt enabled, not executions

I also realize as I am writing this that the call to RecordStatement() below may not make sense if there are automatic retries -- what does it record exactly for every attempt after the first? Please file an issue with that question and cc me and david on it. thanks

Comments from Reviewable

justinj · 2018-06-27T14:57:45Z

I ran into enough roadblocks printing out the value that I don't think it's worth the time right now.

Review status: complete! 0 of 0 LGTMs obtained

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

Previously, knz (kena) wrote…

Oh hm. I realize this last comment is based on an undocumented assumption. I think it would be good that you satisfy yourself that:

this method (recordStatementSummary) is indeed called again on every attempt, with an increasing value of automaticRetryCount every time (perhaps you'll want to add a comment above the function and under the conditional below to explain this fully to future readers)

you indeed only want to count unique issues of the queries with opt enabled, not executions

I also realize as I am writing this that the call to RecordStatement() below may not make sense if there are automatic retries -- what does it record exactly for every attempt after the first? Please file an issue with that question and cc me and david on it. thanks

done, and opened #27015

Comments from Reviewable

knz · 2018-06-28T11:01:30Z

thank you

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

Comments from Reviewable

knz · 2018-06-28T11:17:17Z

Oy I completely forgot something: we also want this to go to the reg server.
It's relatively simple, you do this as follows:

add an "opt" flag in roachpb/app_stats.proto StatementStatisticsKey
add it also to stmtKey in sql/app_stats.go
add the parameter to (*appStats) recordStatement() in app_stats.go
propagate the new field in stmtKey to StatementStatisticsKey in (*sqlStats) getStmtStats() in the same file
invent a character for (stmtKey) flags() to report the flag. Feel free to choose anything that's not valid at the start of a SQL query nor a potential comment character, if you lack inspiration you can take *.

rytaft · 2018-06-29T21:25:53Z

This is great, but it seems like we might also want to track which statements are actually planned by the optimizer, without falling back to the 2.0 planner. Is there a way to do that?

Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Review status: complete! 1 of 0 LGTMs obtained

Comments from Reviewable

knz · 2018-06-30T01:46:49Z

This is great, but it seems like we might also want to track which statements are actually planned by the optimizer, without falling back to the 2.0 planner. Is there a way to do that?

If we plug the statement stat key like I recommend in my previous comment, the opt and non-opt statements will be counted separately (and reported to our reg server!) That's exactly what we need for PM.

justinj · 2018-06-30T18:26:29Z

@rytaft optMode gets mutated in this function, so its value at this point reflects whether the optimizer was used or not, which I think measures what you described, unless I'm misunderstanding?

rytaft · 2018-06-30T20:47:09Z

Oops sorry I was confused. Yep, this is what I was talking about.

It might also be interesting to track the number of queries that fell back, vs. those that were just planned by the 2.0 planner in the first place. Maybe we can already figure that out from settings? Are the cluster/session settings also tracked in the same place?

Comments from Reviewable

justinj · 2018-07-16T20:12:53Z

Sorry, forgot about this! I think you're right that that's important @rytaft - I added a sql.optimizer.fallback.count metric, PTAL.

rytaft · 2018-07-16T21:12:29Z

Nice!

rytaft

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

knz

but please file my request above in a separate issue, and assign the issue to me and the project "Telemetry". Thank you.

Reviewed 1 of 6 files at r1, 5 of 5 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale)

knz · 2018-07-17T10:52:39Z

this one: #26981 (comment)

justinj · 2018-07-17T14:12:11Z

TFTR!

bors r+

craig · 2018-07-17T14:28:53Z

Build failed

GitHub CI (Cockroach)

Release note (sql change): A new metric sql.optimizer.count has been added which tracks the number of queries with with the experimental cost-based optimizer.

justinj · 2018-07-17T14:46:42Z

looks like a flake

bors r+

craig · 2018-07-17T15:02:52Z

Build failed

GitHub CI (Cockroach)

justinj · 2018-07-18T14:55:58Z

flake

bors r+

@knz

26981: sql: add metric for queries run with optimizer r=justinj a=justinj Suggested by @knz. Release note (sql change): A new metric sql.optimizer.count has been added which tracks the number of queries with with the experimental cost-based optimizer. 27651: sql: move lease acquisition out of tableState r=vivekmenezes a=vivekmenezes This change removes lease acquisition from the tableState struct and makes lease acquisition a part of LeaseStore. Future work: Move acquireFreshest and release code paths out of tableState. The eventual goal is to have tableState be a simple cache of table descriptors with the leasing code separated out and only responsible for the expiration time of the latest descriptor in the cache. related to #23510 Release note: None Co-authored-by: Justin Jaffray <justin@cockroachlabs.com> Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>

craig · 2018-07-18T15:30:37Z

Build succeeded

GitHub CI (Cockroach)

justinj requested review from knz, rytaft, mrtracy and a team June 26, 2018 14:47

justinj mentioned this pull request Jun 27, 2018

sql: clarify behaviour of RecordStatement in metrics #27015

Closed

justinj force-pushed the opt-metric branch from 8ce6633 to 8b8be6d Compare June 27, 2018 14:57

justinj force-pushed the opt-metric branch from 8b8be6d to 576fa83 Compare July 16, 2018 20:12

rytaft approved these changes Jul 16, 2018

View reviewed changes

knz approved these changes Jul 17, 2018

View reviewed changes

justinj mentioned this pull request Jul 17, 2018

sql: send optimizer statistics to reg server #27638

Closed

sql: add metric for queries run with optimizer

4d8fb20

Release note (sql change): A new metric sql.optimizer.count has been added which tracks the number of queries with with the experimental cost-based optimizer.

justinj force-pushed the opt-metric branch from 576fa83 to 4d8fb20 Compare July 17, 2018 14:36

craig bot merged commit 4d8fb20 into cockroachdb:master Jul 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add metric for queries run with optimizer #26981

sql: add metric for queries run with optimizer #26981

justinj commented Jun 26, 2018

cockroach-teamcity commented Jun 26, 2018

knz commented Jun 27, 2018

knz commented Jun 27, 2018

justinj commented Jun 27, 2018

knz commented Jun 28, 2018

knz commented Jun 28, 2018

rytaft commented Jun 29, 2018

knz commented Jun 30, 2018

justinj commented Jun 30, 2018

rytaft commented Jun 30, 2018

justinj commented Jul 16, 2018

rytaft commented Jul 16, 2018

rytaft left a comment

knz left a comment

knz commented Jul 17, 2018

justinj commented Jul 17, 2018

craig bot commented Jul 17, 2018

justinj commented Jul 17, 2018

craig bot commented Jul 17, 2018

justinj commented Jul 18, 2018

craig bot commented Jul 18, 2018

sql: add metric for queries run with optimizer #26981

sql: add metric for queries run with optimizer #26981

Conversation

justinj commented Jun 26, 2018

cockroach-teamcity commented Jun 26, 2018

knz commented Jun 27, 2018

knz commented Jun 27, 2018

justinj commented Jun 27, 2018

knz commented Jun 28, 2018

knz commented Jun 28, 2018

rytaft commented Jun 29, 2018

knz commented Jun 30, 2018

justinj commented Jun 30, 2018

rytaft commented Jun 30, 2018

justinj commented Jul 16, 2018

rytaft commented Jul 16, 2018

rytaft left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz commented Jul 17, 2018

justinj commented Jul 17, 2018

craig bot commented Jul 17, 2018

Build failed

justinj commented Jul 17, 2018

craig bot commented Jul 17, 2018

Build failed

justinj commented Jul 18, 2018

craig bot commented Jul 18, 2018

Build succeeded