Reviewed 35 of 35 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

LGTM

Review status: all files reviewed at latest revision, 3 unresolved discussions.

storage/replica.go, line 1516 [r1] (raw file):
A Span is in scope here. Does it matter that we're passing the context instead of that span?

storage/replica_command.go, line 52 [r1] (raw file):
Didn't you just argue against this in #5184?

I feel like maybe we should be passing the context around and getting the span from the context when we need it. (Didn't I just argue against that in #5184? I think the difference is that the span is a global/contextual thing while the command ID and index are more part of the semantics of the particular function.)

One potential reason to pass the context when all we need is a span: the log.Infoc family of methods. We're backing away from structured logging but these methods can still add some extra context to log messages and would let us avoid adding things like store ID and range ID to messages on an ad-hoc basis. I think it would be a good idea to expand their use (and if we're not going to use them more we should just get rid of the few places where we do use them).

storage/engine/mvcc.go, line 50 [r1] (raw file):
This is used for more than just tests and more than just mvcc calls. Should it live in tracing instead (and without the last line of the comment)? I'm not quite sure if it's worthwhile to have a special name for nil.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 3 unresolved discussions.

storage/replica_command.go, line 52 [r1] (raw file):
I'd prefer to see a context.Context passed around everywhere. Seems more idiomatic.

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

storage/replica.go, line 1516 [r1] (raw file):
That's a good point - it does matter if SpanFromContext created the Span. SpanFromContext should also return a new context, but the interface would be awkward - I'll take care of this separately.

storage/replica_command.go, line 52 [r1] (raw file):
I didn't do that because if we don't have a trace, we want to create one, but we can't do that without a tracer. Sure, we can put that into the context as well, but that doesn't see right. But maybe we should figure out the right way to do it - see comment below.

I've also wanted to hook tracing up with logging, but it didn't seem important enough so far.

storage/engine/mvcc.go, line 50 [r1] (raw file):
There's a little bit of a conflict here. On the one hand, we want to avoid nil traces because that means checking in all of the code base that sp != nil before calling sp.LogEvent.
On the other hand, as this change demonstrates, it's awkward to have a Span everywhere. It's also awkward to try to wrap the Span in something else that fixes these shortcomings.

I think there's a good solution once we've figured out how to combine logging and tracing. I'll suggest a PR shortly - the idea would be that all (usual) logging calls take a context which will log to a span contained in the trace (if any) as well. Additionally, we have log.Trace(ctx, msg) (punting on the lazy Printf version for now) which can be used to only log to the trace but not to anywhere else.

Comments from the review on Reviewable.io

Resuscitated, PTAL. Don't look at the diff. I suggest skipping lightly over *_test.go and to concentrate on storage/engine, though I think all changes are trivial (unless I accidentally snuck anything else in).

One question is whether the uses of context.Background() in non-test code should be context.TODO(). See storage/replica.go, storage/stores.go and storage/replica_raftstorage.go.

Review status: 0 of 33 files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

I recall initially wanting to plumb everywhere, but it didn't really make a whole lot of sense. There might be the odd Context that should be a TODO() but I think I looked at the vast majority and decided they should be Background() (i.e. we're not really interested in connecting them to some other upstream context).

Thanks for the quick review, @petermattis.