sql: fix interaction between stmt bundles and tracing

[yuzefovich]

Previously, we wouldn't generate the bundle if the verbose tracing was
already enabled on the cluster because we wouldn't call
instrumentationHelper.Finish where we actually generate the bundle.
This would result in empty responses for EXPLAIN ANALYZE (DEBUG) as
well as the requests for stmt diagnostics being stuck in "waiting"
state.

Fixes: #69398.

Release note (bug fix): Previously, if the tracing
(sql.trace.txn.enable_threshold cluster setting) was enabled on the
cluster, the statement diagnostics collection (EXPLAIN ANALYZE (DEBUG)) wouldn't work. This is now fixed.

Release justification: low-risk fix to a long-standing bug.

[cockroach-teamcity]

This change is 

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

---

pkg/sql/instrumentation.go, line 187 at r1 (raw file):

			// to fetch the trace from it, but the span won't be finished.
			ih.sp = sp
			return ctx, ih.collectBundle

Radu, thoughts about just having return ctx, true?

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/instrumentation.go, line 99 at r1 (raw file):

	sp *tracing.Span
	// onFinish, if non-nil, will be called in instrumentationHelper.Finish.

Can't this be just a shouldFinishSpan flag?

---

pkg/sql/instrumentation.go, line 187 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Radu, thoughts about just having return ctx, true?

I think we need to Finish if we want to savePlanForStats? That code would annotate the explainPlan. But then again it looks like recordStatementSummary would happen before Finish anyway..

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

---

pkg/sql/instrumentation.go, line 99 at r1 (raw file):

Previously, RaduBerinde wrote…

Can't this be just a shouldFinishSpan flag?

Done.

---

pkg/sql/instrumentation.go, line 187 at r1 (raw file):

Previously, RaduBerinde wrote…

I think we need to Finish if we want to savePlanForStats? That code would annotate the explainPlan. But then again it looks like recordStatementSummary would happen before Finish anyway..

Yeah, looks like recordStatementSummary happens before Finish, and Finish doesn't impact savePlanForStats in any way, so I'll just return true if we're collecting the bundle.

[RaduBerinde]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/instrumentation.go, line 187 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Yeah, looks like recordStatementSummary happens before Finish, and Finish doesn't impact savePlanForStats in any way, so I'll just return true if we're collecting the bundle.

SGTM

[yuzefovich]

TFTR!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 996db2a to blathers/backport-release-21.1-69975: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}