From d768e25adf1bc437e660bf8d2978c32ed98f7fc8 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Wed, 29 Jun 2022 23:36:18 +0100 Subject: [PATCH] tracing: fix panic in Finish for verbose recordings When the children of a finished span won't fit in a parent with a verbose recording type, we don't store the children but still want their structured records. Unfortunately, we did this in the recordFinishedChildredLocked case by falling through from the verbose recording case to the structured recording case. In #81079, we added an assertion in the structured recording case that only applies to structured recordings. The result is Finish() failing with: panic: RecordingStructured has 12 recordings; expected 1 Now, we don't fall through and instead gather the structured records of all children. Release note: None --- pkg/util/tracing/crdbspan.go | 9 +++++++-- pkg/util/tracing/span_test.go | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/util/tracing/crdbspan.go b/pkg/util/tracing/crdbspan.go index a4d810a4fe50..c42c4e0f058f 100644 --- a/pkg/util/tracing/crdbspan.go +++ b/pkg/util/tracing/crdbspan.go @@ -543,8 +543,13 @@ func (s *crdbSpan) recordFinishedChildrenLocked(childRecording tracingpb.Recordi } // We don't have space for this recording. Let's collect just the structured - // records by falling through. - fallthrough + // records. + for ci := range childRecording { + child := &childRecording[ci] + for i := range child.StructuredRecords { + s.recordInternalLocked(&child.StructuredRecords[i], &s.mu.recording.structured) + } + } case tracingpb.RecordingStructured: if len(childRecording) != 1 { panic(fmt.Sprintf("RecordingStructured has %d recordings; expected 1", len(childRecording))) diff --git a/pkg/util/tracing/span_test.go b/pkg/util/tracing/span_test.go index 6f9755e3bde5..da6ee79cbc2d 100644 --- a/pkg/util/tracing/span_test.go +++ b/pkg/util/tracing/span_test.go @@ -405,6 +405,23 @@ func TestRecordingMaxSpans(t *testing.T) { require.Len(t, root.StructuredRecords, extraChildren) } +// TestRecordingFinishWithMaxSpans is a regression test to ensure that a verbose +// recording does not erroneously panic in Finish because it has too many children. +func TestRecordingFinishWithMaxSpans(t *testing.T) { + tr := NewTracer() + root := tr.StartSpan("root", WithRecording(tracingpb.RecordingVerbose)) + defer root.Finish() + + sp := tr.StartSpan("span-under-test", WithParent(root)) + extraChildren := 10 + numChildren := maxRecordedSpansPerTrace + extraChildren + for i := 0; i < numChildren; i++ { + child := tr.StartSpan(fmt.Sprintf("child %d", i), WithParent(sp)) + child.Finish() + } + require.NotPanics(t, sp.Finish) +} + type explodyNetTr struct { trace.Trace }