cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

sql,sqlstats: lift out fields required for logging from `dispatchToExecutionEngine`

Open xinhaoz opened this issue 1 year ago • 2 comments

sql: record topLevelQueryStats in instrumentationhelper

Previously topLevelQueryStats were being passed directly into the logging function after execution in dispatchToExecutionEngine. This commit persists the top level stats of the query execution into the planner's instrumentation helper, similar to what we do with sampled query exec stats. This allows us to move the logging operation in dispatchToExecutionEngine to execStmtInOpenState.

Part of: https://github.com/cockroachdb/cockroach/issues/122722

Release note: None

sql, sqlstats: construct statement fingerprint id on demand when logging

Currently we construct the stmt fingerprint id at various places to pass to the logging function. We only need the statement fingerprint id for telemetry logging, so this construction is wasteful in other scenarios. This commit moves the stmt fingerprint id generation to the logging function.

Summary:

  • Record the current stmt fingerprint id in the stats collector when writing sql stats. This value is reset at the start of stmt exec.
  • Construct stmt fingerprint at the time of logging based on the AST in the planner if it was not available in the stats collector.

This change is motivated by the desire to move logging from disatpchToExecutionEngine to execStmtInOpenState. See issue for details.

Part of: https://github.com/cockroachdb/cockroach/issues/122722

xinhaoz avatar May 15 '24 20:05 xinhaoz

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

blathers-crl[bot] avatar May 15 '24 20:05 blathers-crl[bot]

This change is Reviewable

cockroach-teamcity avatar May 15 '24 20:05 cockroach-teamcity

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

Previously, abarganier (Alex Barganier) wrote…

Just confirming - the fingerprint will still be recomputed further down in the maybeLogStatement call for this scenario, right?

Yup, that's right.

xinhaoz avatar May 21 '24 16:05 xinhaoz

TFTR! bors r=abarganier

xinhaoz avatar May 21 '24 17:05 xinhaoz