Skip to content

Commit

Permalink
Merge #59370
Browse files Browse the repository at this point in the history
59370: tracing: guardrail against memory leaks r=knz a=tbg

tracing: limit number of direct children

In #59347 we saw an OOM due to more and more children being registered
with a long-running trace Span (held open by a raft scheduler worker
goroutine). This commit adds a simple guardrail: no more than 1000 spans
can be registered directly with a single parent; any in excess are
simply not registered at all.

Fixes #59347.
Fixes #59355.
Fixes #59344.

Release note: None


Co-authored-by: Tobias Grieger <[email protected]>
  • Loading branch information
craig[bot] and tbg committed Jan 25, 2021
2 parents 1aa232c + 64d6d87 commit bc21f3a
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 3 deletions.
5 changes: 4 additions & 1 deletion pkg/util/tracing/crdbspan.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,10 @@ func (s *crdbSpan) getRecordingLocked(m mode) tracingpb.RecordedSpan {

func (s *crdbSpan) addChild(child *crdbSpan) {
s.mu.Lock()
s.mu.recording.children = append(s.mu.recording.children, child)
// Only record the child if the parent still has room.
if len(s.mu.recording.children) < maxChildrenPerSpan {
s.mu.recording.children = append(s.mu.recording.children, child)
}
s.mu.Unlock()
}

Expand Down
18 changes: 18 additions & 0 deletions pkg/util/tracing/span_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
package tracing

import (
"fmt"
"regexp"
"strings"
"testing"
Expand Down Expand Up @@ -212,3 +213,20 @@ func TestNonVerboseChildSpanRegisteredWithParent(t *testing.T) {
require.Len(t, rec, 2)
require.Len(t, rec[1].InternalStructured, 1)
}

// TestSpanMaxChildren verifies that a Span can
// track at most maxChildrenPerSpan direct children.
func TestSpanMaxChildren(t *testing.T) {
tr := NewTracer()
sp := tr.StartSpan("root", WithForceRealSpan())
defer sp.Finish()
for i := 0; i < maxChildrenPerSpan+123; i++ {
ch := tr.StartSpan(fmt.Sprintf("child %d", i), WithParentAndAutoCollection(sp), WithForceRealSpan())
ch.Finish()
exp := i + 1
if exp > maxChildrenPerSpan {
exp = maxChildrenPerSpan
}
require.Len(t, sp.crdb.mu.recording.children, exp)
}
}
8 changes: 6 additions & 2 deletions pkg/util/tracing/tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,12 @@ import (
// and since this string goes on the wire, it's a hassle to change it now.
const verboseTracingBaggageKey = "sb"

// maxLogsPerSpan limits the number of logs in a Span; use a comfortable limit.
const maxLogsPerSpan = 1000
const (
// maxLogsPerSpan limits the number of logs in a Span; use a comfortable limit.
maxLogsPerSpan = 1000
// maxChildrenPerSpan limits the number of (direct) child spans in a Span.
maxChildrenPerSpan = 1000
)

// These constants are used to form keys to represent tracing context
// information in carriers supporting opentracing.HTTPHeaders format.
Expand Down

0 comments on commit bc21f3a

Please sign in to comment.