kvserver: incorporate remote tracing spans from snapshots #86436

AlexTalks · 2022-08-19T06:51:37Z

This adds collected tracing spans into a SnapshotResponse object in
order to incorporate remote traces from the receiver side of a snapshot
into the client's (i.e. the sender's) context.

Release justification: Low-risk observability change.
Release note: None

cockroach-teamcity · 2022-08-19T06:51:44Z

This change is

andreimatei

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks and @andreimatei)

pkg/kv/kvserver/store_snapshot.go line 400 at r1 (raw file):

		if req.Header != nil {
			err := errors.New("client error: provided a header mid-stream")
			return noSnap, sendSnapshotError(stream, err, nil)

pls comment all the nils, as per the style guide.
Or better, make a sendSnapshotErrWithTrace.

pkg/kv/kvserver/store_snapshot.go line 1034 at r1 (raw file):

		}
	}
	rCtx, rSp := tracing.EnsureChildSpan(ctx, s.cfg.Tracer(), "receive snapshot data")

let's reassign ctx like before unless there's a very good reason not too. It's too easy to use the wrong context when there's a ctx in scope.

pkg/kv/kvserver/store_snapshot.go line 1049 at r1 (raw file):

	if err := s.processRaftSnapshotRequest(applyCtx, header, inSnap); err != nil {
		return sendSnapshotError(stream, errors.Wrap(err.GoError(), "failed to apply snapshot"),
			sp.GetConfiguredRecording())

nit: lift a rec := sp.GetConfiguredRecording() and use it in both cases

pkg/kv/kvserver/store_snapshot.go line 1057 at r1 (raw file):

}

func sendSnapshotError(stream incomingSnapshotStream, err error, recordedSpans tracingpb.Recording) error {

consider renaming recordedSpans -> trace

pkg/kv/kvserver/store_snapshot.go line 1057 at r1 (raw file):

}

func sendSnapshotError(stream incomingSnapshotStream, err error, recordedSpans tracingpb.Recording) error {

the trace deserves a comment. And say it can be missing.

pkg/kv/kvserver/store_snapshot.go line 1423 at r1 (raw file):

// importSnapshotTracingSpans incorporates traces collected in a
// (Delegated)SnapshotResponse into the current context's tracing span.
func importSnapshotTracingSpans(ctx context.Context, resp kvserverpb.ResponseWithTracingSpans) {

Do we really need this ResponseWithTrancingSpans interface? It seems simpler and better to just pass a recording to this function directly.
I also think it'd be better if this took the span directly - one of the callers has a reference to it, and it'd be better to see that variable be used. At which point, consider not having this function at all.

pkg/kv/kvserver/store_snapshot.go line 1428 at r1 (raw file):

		span := tracing.SpanFromContext(ctx)
		if span == nil {
			log.Warningf(

I think this warning is a bad idea. It relies on the fact that that, on the server side, sp.GetRecording() returns nil for RecordingOff. That might not be true in the future, and it's not even true today when the server span's recording is enabled dynamically from /tracez.

pkg/kv/kvserver/store_snapshot.go line 1475 at r1 (raw file):

		return err
	}
	importSnapshotTracingSpans(ctx, resp)

if only the APPLIED message carries a trace, let's only import in response to that one.
It would probably be hard for multiple messages to carry traces; if they did and the traces overlapped, importing the overlapping part multiple times would be an error.

pkg/kv/kvserver/store_snapshot.go line 1563 at r1 (raw file):

			return errors.Wrapf(err, "%s: expected EOF, got resp=%v with error", to, unexpectedResp)
		}
		importSnapshotTracingSpans(ctx, resp)

resp or unexpectedResp?
In any case, I'd remove this import altogether.

pkg/kv/kvserver/kvserverpb/raft.proto line 206 at r1 (raw file):

}

message SnapshotResponse {

please comment either here or on the service RPC declaration how many messages the client should expect

pkg/kv/kvserver/kvserverpb/raft.proto line 217 at r1 (raw file):

  string message = 2;
  reserved 3;
  repeated util.tracing.tracingpb.RecordedSpan collected_spans = 4 [(gogoproto.nullable) = false];

this needs a comment. Do all responses carry it or only the APPLIED one?

AlexTalks

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks and @andreimatei)

pkg/kv/kvserver/store_snapshot.go line 400 at r1 (raw file):