Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @shermanCRL, and @tbg)

-- commits, line 4 at r2:
This patch moves a bunch of sites to use stopper.Tracer(). I think this generally is not a great thing because the fact that the Stopper library has a Tracer is fairly incidental; it doesn't seem like a good idea to me to couple too many people to it.
I have introduced the stopper.Tracer() method recently, in lack of a better idea about how to deal with some initialization hairballs across the different kinds of test servers and contraptions that we have. In an ideal world with more structure, this method would not be exported. So I have something to do with this method, but I've been using it generally only for construction of testing scaffolding, not for "end users" that access the Tracer to create spans with it. In all fairness, I also have introduced one use it which is in the "end user" category because it was expedient, so I'm not without sin. I don't want to hypocritically hold this patch to a different standard, so I'm not against any use stopper.Tracer() when an alternative way of plumbing the Tracer is not easy enough, but where there are alternatives, I think we should take them. In particular, a bunch of callers do ...Store.stopper.Tracer(). I think it'd be better if the store directly had an exposed the Tracer, and then whoever has access to a Store can us it. Perhaps there are other similar heavy objects like that can expose references to a Tracer to satisfy a bunch of call sites at once (for example the ExecutorConfig is another good place to hang a Tracer from - and you've already made an ExecCfg.Tracer() method, but it still delegates to a stopper underneath).
If you prefer, I can give this a shot and see how many uses of stopper.Tracer() remain.

pkg/cli/start.go, line 424 at r2 (raw file):

	// goroutine started below has completed.  TODO(andrei): we don't
	// close the span on the early returns below.
	ctx = ambientCtx.AnnotateCtx(ctx)

consider inlining this below as serverCfg.Tracer.EnsureChildSpan(ambientCtx.AnnotateCtx(ctx), "...").

pkg/kv/db.go, line 270 at r2 (raw file):

// Tracer retrieves the Tracer instance for this DB handle.
func (db *DB) Tracer() *tracing.Tracer {

I think we should avoid exporting this method if we can; I think the DB interface should be as clean as possible and this doesn't belong. This method is only used in kvnemesis, right? Let's burden that guy (the Applier) with keeping track of what Tracer needs to be used for what db/server.

pkg/kv/kvclient/kvcoord/dist_sender.go, line 462 at r2 (raw file):

// Tracer returns this distributer sender's tracer.
func (ds *DistSender) Tracer() *tracing.Tracer {

At the very least, let's not export this method (which would also let you get rid of the superfluous comment which also has a spelling error). But also this method seems to have a single caller, so I wouldn't introduce it at all (the method is not even consistently used inside the DistSender, as the DistSender.RangeFeed() method doesn't use it).

Also, accessing the tracer from the stopper from the rpcContext is at least one, if not two, steps too far in my opinion. I think it's already bad that the DistSender accesses the rpcContext's stopper (in fact it's even worse cause the Stopper is actually in a ContextOptions embedded by the rpcContext). I think it's unfortunate that rpc.Context exports a Stopper like that. My bet is that it didn't originally actually intend to export it, but it happened by accident because of that embedding. The DistSender shouldn't need to know that the rpcContext has a Stopper in it, and then accessing the Tracer in the Stopper seems like an even more superfluous coupling. Let's have the DistSender take in the Tracer explicitly through the DistSenderConfig.

pkg/kv/kvclient/kvcoord/dist_sender_rangefeed.go, line 62 at r2 (raw file):

	ctx = ds.AnnotateCtx(ctx)
	ctx, sp := tracing.EnsureChildSpan(ctx, ds.rpcContext.Stopper.Tracer(), "dist sender")

if the DistSender gets a Tracer field, let's use it here too.

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 489 at r2 (raw file):

	}

	ctx = tc.AnnotateCtx(ctx)

consider inlining this call below

pkg/sql/exec_util.go, line 1252 at r2 (raw file):

// Tracer returns the tracer for this config.
func (cfg *ExecutorConfig) Tracer() *tracing.Tracer {
	return cfg.RPCContext.Stopper.Tracer()

let's find a different way to plug the Tracer to the ExecutorConfig.

pkg/util/log/ambient_context.go, line 106 at r2 (raw file):

//
// For background operations, context.Background() should be passed; however, in
// that case it is strongly recommended to open a span if possible (using

nit: since you're taking blame around here, consider s/it is strongly recommended/consider, to soften the language. I think such strong language was uncalled for in the first place, and becomes even more questionable now that we're referencing a different library. There's no strong coupling between this log library and the tracing one, and this comment shouldn't bully people into thinking otherwise.

pkg/util/log/ambient_context.go, line 107 at r2 (raw file):

// For background operations, context.Background() should be passed; however, in
// that case it is strongly recommended to open a span if possible (using
// tracing.EnsureChildSpan or the EnsureChildSpan on the nearest Tracer instance,

In the context (ha!) of context.Background(), EnsureChildSpan() will work, but is not the right thing to use - because you know that you won't get a child. Just reference Tracer.StartSpan{Ctx}().

pkg/util/log/ambient_context.go, line 108 at r2 (raw file):

// that case it is strongly recommended to open a span if possible (using
// tracing.EnsureChildSpan or the EnsureChildSpan on the nearest Tracer instance,
// usually available via a Stopper).

I'd definitely remove the mention of the stopper, as discussed in the commit-level comment.

pkg/util/log/ambient_context.go, line 165 at r3 (raw file):

// when a test does not have sufficient details to instantiate a fully
// fledged server AmbientContext.
func MakeTestingAmbientContext() AmbientContext {

I think this function is very unusual and shouldn't exist. You don't need a constructor for a zero value. At the very least, make it a static var EmptyAmbientContext = AmbientContext{}. But I don't feel a need for it; I think everybody should construct the zero value themselves.

pkg/util/tracing/tracer.go, line 1272 at r2 (raw file):

}

// EnsureChildSpan is a helper method that calls the

I think it'd be nice to not have both versions of EnsureChildSpan. We already have way too many ways to create a span.
The stand-alone function exists for symmetry with ChildSpan (which doesn't need a Tracer), so I don't know if I'd prefer the Tracer method. Also, if we do something here, we should do the same for EnsureForkSpan.

I also question whether EnsureChildSpan/EnsureForkSpan actually need to exist, as opposed to tr.StartSpan(WithParent(SpanFromContext(ctx)). WithParent() can handle a nil parent, I think. EnsureChildSpan has this thing with the options pool, which we can't ask every caller to deal with. But I think that, one way or another, we can get an API that does involve a slice of variadic arguments escaping for a stupid reason (for example, we could move away from the variadic API completely, which I've been pondering).

I guess I'd encourage you to not do anything here for now, if you don't have strong opinions.