tracing: enable always-on tracing by default

[irfansharif]

This is follow-up work from #58712, where we measured the overhead for
always-on tracing and found it to be minimal/acceptable. Lets switch
this on by default to shake the implications of doing so. We can
reasonably expect two kinds of fallout:

1. Unexpected blow up in memory usage due to resource leakage (which is
   a can be problem now that we're always maintaining open spans in an
   internal registry, see *: spans that are never explicitly Finish()-ed #58721)

2. Performance degradation due to tracing overhead per-request
   (something tracing,sql: introduce BenchmarkTracing #58712) was spot checking for.

For 1 we'll introduce a future test in a separate PR. For 2, we'll
monitor roachperf over the next few weeks.

---

Also moved some of the documentation for the cluster setting into a
comment form above. Looking at what's rendered in our other cluster
settings (SHOW ALL CLUSTER SETTINGS), we default to a very pity,
unwrapped description.

Release note: None

[cockroach-teamcity]

This change is 

[tbg]

Wow, so much red in CI! Curious what this is all about.

[irfansharif]

Probably the span leak we found in #58902. I'm not sure what to do about it yet.

[tbg]

How can it be that? We're not checking for span leaks in this PR.

[irfansharif]

Was a stab in the dark, but I was thinking it could be due to some memory blow up. I'll take a closer look in the morning.

[irfansharif]

Looks like it was something else, but I'm not really sure what to do about it. With always on tracing, even spans with "nothing in them" generaterecordings. Cause it's always on, we'll at least generate the portion of the trace with the operation name.

0.000ms      0.000ms    === operation:table reader

With the legacy tracing mode, cause we weren't explicitly recording, we used to return nil traces when requested:

cockroach/pkg/util/tracing/crdbspan.go

Lines 136 to 143 in ff1442b

	if m == modeLegacy && s.recordingType() == RecordingOff {
	// In legacy tracing (pre always-on), we avoid allocations when the
	// Span is not actively recording.
	//
	// TODO(tbg): we could consider doing the same when background tracing
	// is on but the current span contains "nothing of interest".
	return nil
	}

Now that this is not nil, we'll attach it to the our processor's trailingMeta here:

cockroach/pkg/sql/execinfra/processorsbase.go

Lines 710 to 712 in ff1442b

	if trace := pb.span.GetRecording(); trace != nil {
	pb.trailingMeta = append(pb.trailingMeta, execinfrapb.ProducerMetadata{TraceData: trace})
	}

Most of the failing tests aren't expecting this:

cockroach/pkg/sql/flowinfra/server_test.go

Lines 109 to 113 in fb03099

	metas = ignoreLeafTxnState(metas)
	metas = ignoreMetricsMeta(metas)
	if len(metas) != 0 {
	t.Errorf("unexpected metadata: %v", metas)
	}

I'm not sure what to do here. At first I figured if it's just tests, we could ignore the metas with trace data. Using something like:

// ignoreTraceData takes a slice of metadata and returns the entries
// excluding the ones with trace data.
func ignoreTraceData(metas []execinfrapb.ProducerMetadata) []execinfrapb.ProducerMetadata {
	res := make([]execinfrapb.ProducerMetadata, 0)
	for _, m := range metas {
		if m.TraceData == nil {
			res = append(res, m)
		}
	}
	return res
}

But I'm not sure that's right, and also it doesn't help TestCSVImportCanBeResumed, which fails legitimately (the import process doesn't actually resume with always-on tracing). I think it's because we use trailingMeta in our control flow?

cockroach/pkg/sql/execinfra/processorsbase.go

Lines 627 to 632 in fb03099

	// trailingMeta always has priority; it seems like a good idea because it
	// causes metadata to be sent quickly after it is produced (e.g. the error
	// passed to MoveToDraining()).
	if len(pb.trailingMeta) > 0 {
	return pb.popTrailingMeta()
	}

I'm don't know what the right patch here would be. Should we not include the trace data in trailing meta if there are no remote+children spans (aka behave like earlier)? Should we change our handling of trailingMeta to possibly account for these "spans with nothing in them"? Perhaps @asubiotto or @yuzefovich have ideas; I'm lost.

[yuzefovich]

I don't see that we need to do anything special at this moment (other than silencing, in tests, these TraceData meta as you suggested). TestCSVImportCanBeResumed fails legitimately because there is a bug in the implementation of the import processor, try this diff:

diff --git a/pkg/ccl/importccl/import_processor.go b/pkg/ccl/importccl/import_processor.go
index 9da93ace86..9fdb3ed043 100644
--- a/pkg/ccl/importccl/import_processor.go
+++ b/pkg/ccl/importccl/import_processor.go
@@ -135,16 +135,15 @@ func (idp *readImportDataProcessor) Next() (rowenc.EncDatumRow, *execinfrapb.Pro
        // Once the import is done, send back to the controller the serialized
        // summary of the import operation. For more info see roachpb.BulkOpSummary.
        countsBytes, err := protoutil.Marshal(idp.summary)
+       idp.MoveToDraining(err)
        if err != nil {
-               idp.MoveToDraining(err)
                return nil, idp.DrainHelper()
        }
 
-       idp.MoveToDraining(nil /* err */)
        return rowenc.EncDatumRow{
                rowenc.DatumToEncDatum(types.Bytes, tree.NewDBytes(tree.DBytes(countsBytes))),
                rowenc.DatumToEncDatum(types.Bytes, tree.NewDBytes(tree.DBytes([]byte{}))),
-       }, idp.DrainHelper()
+       }, nil
 }

The contract of RowSource.Next method is that only one of row and meta is non-nil, and it was broken.

[irfansharif]

Thanks for the tip, sent it out #59115. @tbg / @knz, mind rubber stamping the second commit here?

[irfansharif]

bors r+

[craig]

Build failed:

• GitHub CI (Cockroach)

[irfansharif]

curl: (22) The requested URL returned error: 504 GATEWAY_TIMEOUT

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)