kvserver: instrument RaftTransport workers with pprof labels

[pav-kv]

The unused arguments in the method signature were used to identify goroutines
in traces. This no longer works after Go 1.17 started passing arguments via
registers.

This commit adds pprof labels when starting these goroutines, to have a cleaner
code, more readable traces, and to work around the new Go convention.

Release note: None

[cockroach-teamcity]

This change is 

[erikgrinaker]

I think these parameters are there on purpose, because they allow us to identify which node/class the goroutine belongs to in goroutine dumps. This is very useful during debugging, if something has stalled somewhere, since there can be lots of these goroutines running and we need to know which is which.

Might be worth a comment about this.

[pav-kv]

@erikgrinaker Good to know that, I'll add a comment.

[erikgrinaker]

That said, I believe the recent changes to Go function call conventions in 1.17 may have broken this. I forget what the current state is, maybe you remember @tbg? We should really be using pprof labels instead, if possible.

[erikgrinaker]

Yeah, there's something about it here:

cockroach/pkg/kv/kvserver/replica_send.go

Lines 117 to 136 in c7396ee

	// sendWithoutRangeID used to be called sendWithRangeID, accepted a `_forStacks
	// roachpb.RangeID` argument, and had the description below. Ever since Go
	// switched to the register-based calling convention though, this stopped
	// working, giving essentially random numbers in the goroutine dumps that were
	// misleading. It has thus been "disarmed" until Go produces useful values
	// again.
	//
	// See (internal): https://cockroachlabs.slack.com/archives/G01G8LK77DK/p1641478596004700
	//
	// sendWithRangeID takes an unused rangeID argument so that the range
	// ID will be accessible in stack traces (both in panics and when
	// sampling goroutines from a live server). This line is subject to
	// the whims of the compiler and it can be difficult to find the right
	// value, but as of this writing the following example shows a stack
	// while processing range 21 (0x15) (the first occurrence of that
	// number is the rangeID argument, the second is within the encoded
	// BatchRequest, although we don't want to rely on that occurring
	// within the portion printed in the stack trace):
	//
	// github.com/cockroachdb/cockroach/pkg/storage.(*Replica).sendWithRangeID(0xc420d1a000, 0x64bfb80, 0xc421564b10, 0x15, 0x153fd4634aeb0193, 0x0, 0x100000001, 0x1, 0x15, 0x0, ...)

FWIW, we often call these _forStacks in function definitions to mark them as such:

cockroach/pkg/kv/kvserver/replica_rangefeed.go

Lines 142 to 144 in 566dff2

	func (r *Replica) rangeFeedWithRangeID(
	_forStacks roachpb.RangeID, args *roachpb.RangeFeedRequest, stream rangefeed.Stream,
	) *roachpb.Error {

[tbg]

The unused nodeID parameter was introduced in https://github.com/cockroachdb/cockroach/pull/8529/files#diff-1fa05e654163c64041e809fec538526cedc6723e47797f1f4e5df8993a1e60a9R247 and I don't think the intention was to be able to pull the nodeID from a stack trace. I also don't think the connection class parameter has this purpose, or we wouldn't have tacked it on at the end.

As Erik pointed out, the "correct" way to do this now is pprof annotations, which were added to the goroutine profile output recently!

If you don't mind, @pavelkalinnikov, add that here. Basically, we want to wrap this call

cockroach/pkg/kv/kvserver/raft_transport.go

Lines 713 to 715 in 0b73408

	if err := t.processQueue(toNodeID, ch, stats, stream, class); err != nil {
	log.Warningf(ctx, "while processing outgoing Raft queue to node %d: %s:", toNodeID, err)
	}

in a pprof.Do:

		pprof.Do(ctx, pprof.Labels("remote_node_id", toNodeID.String()), func(ctx context.Context) {
			if err := t.processQueue(toNodeID, ch, stats, stream, class); err != nil {
				log.Warningf(ctx, "while processing outgoing Raft queue to node %d: %s:", toNodeID, err)
			}
		})

You can then ./dev build, run a (say) three-node cluster, and navigate to /debug/pprof/goroutine?debug=1 and look for a corresponding labels: line like this (but referencing your label).

---

For Replica.Send (i.e. fix up sendWithoutRangeID to use the same pattern) it's unfortunately harder due to the need for allocations while also wanting to reuse incoming label sets if there is one, which the API also makes difficult to discover in a low-alloc way (though not impossible). Filed #85948 for this.

[pav-kv]

@tbg @erikgrinaker Done, PTAL.

[pav-kv]

If you don't mind, @pavelkalinnikov, add that here. Basically, we want to wrap this call

@tbg I wrapped it a bit higher up the stack, in the place where the worker is launched.

[tbg]

LGTM, thanks!

[erikgrinaker]

Very nice, thanks!

[pav-kv]

bors r=tbg,erikgrinaker

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)