Skip to content

Commit

Permalink
kvserver: include Raft application on leaseholder in request trace
Browse files Browse the repository at this point in the history
Before this patch, the decoding and application of a Raft command was
not included in the recording of the request that generated the
respective command, even in the case where consensus is synchronous with
respect to the request (i.e. non-AsyncConsensus requests). This was
because, although we plumb tracing information below Raft, the span
under which Raft processing was occurring was Fork()ed from the parent
span (i.e. the request evaluation's span). The reasons why that was are
not good:

1) forking (as opposed to creating a regular child) was introduced in
   cockroachdb#39425. I'm not sure whether there was a particular reason for this
   decision. Perhaps there was fear at the time about the child
   outliving the parent - although generally it doesn't because,
   in the case of async consensus, we fork a parent which I think will
   outlive the child:
   https://github.com/cockroachdb/cockroach/blame/13669f9c9bd92a4c3b0378a558d7735f122c4e72/pkg/kv/kvserver/replica_raft.go#L157

   In case of sync consensus requests, it's possible for the Raft application
   span to outlive the evaluation span in cases when the application part
   (as opposed to the consensus part) can be done async (see
   CanAckBeforeApplication). Still, regardless of the exact details of the
   lifetimes, with time it has become clear that it's appropriate to create
   a child when it's expected that it will usually not outlive the parent
   even if, on occasion, it can outlive it.

2) forked spans used to be included in the parent's recording until cockroachdb#59815.
   This explains why we regressed here - Raft application used to be
   included in recordings properly.

This patch makes the application span be a regular child, and so the
raft application span is included in the request trace.

Touches cockroachdb#70864. That issue asks for a new tracing feature but I think
what motivated it is the application info missing from query traces.
This patch is sufficient for that.

Release note (general change): Tracing transaction commits now includes
details about replication.
  • Loading branch information
andreimatei committed Nov 18, 2021
1 parent d064059 commit f058121
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 2 deletions.
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/replica_application_decoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (d *replicaDecoder) createTracingSpans(ctx context.Context) {
for it.init(&d.cmdBuf); it.Valid(); it.Next() {
cmd := it.cur()
if cmd.IsLocal() {
cmd.ctx, cmd.sp = tracing.ForkSpan(cmd.proposal.ctx, opName)
cmd.ctx, cmd.sp = tracing.ChildSpan(cmd.proposal.ctx, opName)
} else if cmd.raftCmd.TraceData != nil {
// The proposal isn't local, and trace data is available. Extract
// the remote span and start a server-side span that follows from it.
Expand Down
1 change: 0 additions & 1 deletion pkg/kv/kvserver/replica_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8359,7 +8359,6 @@ func TestFailureToProcessCommandClearsLocalResult(t *testing.T) {
r.mu.Unlock()

tr := tc.store.cfg.AmbientCtx.Tracer
tr.TestingRecordAsyncSpans() // we assert on async span traces in this test
opCtx, getRecAndFinish := tracing.ContextWithRecordingSpan(ctx, tr, "test-recording")
defer getRecAndFinish()

Expand Down

0 comments on commit f058121

Please sign in to comment.