Skip to content

Commit

Permalink
kvserver: annotate queues with profiler labels
Browse files Browse the repository at this point in the history
Prior to this commit, different types of queues (consistency, merge, replica…) reuse baseQueue as the base
implementation for queues. But goroutine dump does not explicitly show which queue called the function specifically
which makes debugging complicated. This commit adds every queue’s name as a log tag to some queue implementation
interfaces including maybeAdd and processLoop.
  • Loading branch information
wenyihu6 committed May 17, 2023
1 parent 7b86c7c commit 5cd58f7
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 1 deletion.
1 change: 1 addition & 0 deletions pkg/kv/kvserver/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ go_library(
"//pkg/util/metric",
"//pkg/util/metric/aggmetric",
"//pkg/util/mon",
"//pkg/util/pprofutil",
"//pkg/util/protoutil",
"//pkg/util/quotapool",
"//pkg/util/retry",
Expand Down
17 changes: 16 additions & 1 deletion pkg/kv/kvserver/queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"container/heap"
"context"
"fmt"
"runtime/pprof"
"sync/atomic"
"time"

Expand All @@ -29,6 +30,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/hlc"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/metric"
"github.com/cockroachdb/cockroach/pkg/util/pprofutil"
"github.com/cockroachdb/cockroach/pkg/util/quotapool"
"github.com/cockroachdb/cockroach/pkg/util/stop"
"github.com/cockroachdb/cockroach/pkg/util/syncutil"
Expand Down Expand Up @@ -692,7 +694,17 @@ func (bq *baseQueue) maybeAdd(ctx context.Context, repl replicaInQueue, now hlc.
// it may not be and shouldQueue will be passed a nil realRepl. These tests
// know what they're getting into so that's fine.
realRepl, _ := repl.(*Replica)
should, priority := bq.impl.shouldQueue(ctx, now, realRepl, confReader)

labels := pprof.Labels("queue name", bq.name)
var should bool
var priority float64
pprof.Do(ctx, labels, func(ctx context.Context) {
if fn := bq.store.TestingKnobs().MaybeAddInterceptor; fn != nil {
fn(pprof.Label(ctx, "queue name"))
}
should, priority = bq.impl.shouldQueue(ctx, now, realRepl, confReader)
})

if !should {
return
}
Expand Down Expand Up @@ -824,6 +836,9 @@ func (bq *baseQueue) MaybeRemove(rangeID roachpb.RangeID) {
// stopper signals exit.
func (bq *baseQueue) processLoop(stopper *stop.Stopper) {
ctx := bq.AnnotateCtx(context.Background())
ctx, undo := pprofutil.SetProfilerLabelsFromCtxTags(ctx)
defer undo()

done := func() {
bq.mu.Lock()
bq.mu.stopped = true
Expand Down
37 changes: 37 additions & 0 deletions pkg/kv/kvserver/queue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"container/heap"
"context"
"fmt"
"runtime/pprof"
"strconv"
"sync/atomic"
"testing"
Expand Down Expand Up @@ -561,6 +562,42 @@ func TestBaseQueueAddRemove(t *testing.T) {
}
}

// TestBaseQueueLabel verifies that queue name tag does not exist before and after maybeAdd was called but exist while maybeAdd is called.
func TestBaseQueueLabel(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
tc := testContext{}
stopper := stop.NewStopper()
ctx := context.Background()
defer stopper.Stop(ctx)
tc.Start(ctx, t, stopper)

r, err := tc.store.GetReplica(1)
if err != nil {
t.Fatal(err)
}

testQueue := &testQueueImpl{
shouldQueueFn: func(now hlc.ClockTimestamp, r *Replica) (shouldQueue bool, _ float64) {
shouldQueue = true
return
},
}
bq := makeTestBaseQueue("test", testQueue, tc.store, queueConfig{
acceptsUnsplitRanges: true,
})
bq.store.TestingKnobs().MaybeAddInterceptor = func(labelName string, labelDoesExist bool) {
require.True(t, labelDoesExist)
require.Equal(t, "test", labelName)
}
bq.Start(stopper)
bq.maybeAdd(ctx, r, hlc.ClockTimestamp{})

v, ok := pprof.Label(ctx, "queue name")
require.False(t, ok)
require.Equal(t, "", v)
}

// TestNeedsSystemConfig verifies that queues that don't need the system config
// are able to process replicas when the system config isn't available.
func TestNeedsSystemConfig(t *testing.T) {
Expand Down
3 changes: 3 additions & 0 deletions pkg/kv/kvserver/testing_knobs.go
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,9 @@ type StoreTestingKnobs struct {

// RangeLeaseAcquireTimeoutOverride overrides RaftConfig.RangeLeaseAcquireTimeout().
RangeLeaseAcquireTimeoutOverride time.Duration

// MaybeAddInterceptor intercepts calls to MaybeAdd -> passes the function with the label
MaybeAddInterceptor func(labelName string, labelDoesExist bool)
}

// ModuleTestingKnobs is part of the base.ModuleTestingKnobs interface.
Expand Down

0 comments on commit 5cd58f7

Please sign in to comment.