Skip to content

Commit

Permalink
tracing: remove WithSeparateRecording
Browse files Browse the repository at this point in the history
`WithSeparateRecording` set up a Span to *not* share its recording
with its parent. This was necessary before cockroachdb#50914 as parents would also
show up in children's recording, and SQL SpanStats want to set up spans
only for the purpose of populating them with the stats (and want to
avoid pulling a possibly giant recording of the parent out with it).

PR cockroachdb#50914 introduced the unidirectional transitivity we have today: a
parent's recording will recurse into the child, but not vice versa.
This means that the existing uses of `WithSeparateRecording` were no
longer useful; all they were doing was to hide information from the
parent.
In fact, they are counterproductive, as they prevent the full recording
to become available at the root span, which is not an issue today since
the only thing missing are SpanStats, but with always-on tracing we'd
possibly silently drop metadata.

Release note: None
  • Loading branch information
tbg committed Nov 4, 2020
1 parent 25edeb8 commit 86bc2e3
Show file tree
Hide file tree
Showing 6 changed files with 9 additions and 34 deletions.
5 changes: 2 additions & 3 deletions pkg/sql/colflow/vectorized_flow.go
Original file line number Diff line number Diff line change
Expand Up @@ -912,9 +912,8 @@ func (s *vectorizedFlowCreator) setupOutput(
metadataSourcesQueue,
execinfrapb.CallbackMetadataSource{
DrainMetaCb: func(ctx context.Context) []execinfrapb.ProducerMetadata {
// Start a separate recording so that GetRecording will return
// the recordings for only the child spans containing stats.
ctx, span := tracing.ChildSpanSeparateRecording(ctx, "")
// Start a child span whose recording will only contain SpanStats.
ctx, span := tracing.ChildSpan(ctx, "stats")
finishVectorizedStatsCollectors(
ctx, flowCtx.ID, flowCtx.Cfg.TestingKnobs.DeterministicStats, vscs,
)
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/execinfra/processorsbase.go
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,7 @@ func (pb *ProcessorBase) AppendTrailingMeta(meta execinfrapb.ProducerMetadata) {
// ProcessorSpan creates a child span for a processor (if we are doing any
// tracing). The returned span needs to be finished using tracing.FinishSpan.
func ProcessorSpan(ctx context.Context, name string) (context.Context, *tracing.Span) {
return tracing.ChildSpanSeparateRecording(ctx, name)
return tracing.ChildSpan(ctx, name)
}

// StartInternal prepares the ProcessorBase for execution. It returns the
Expand Down
10 changes: 3 additions & 7 deletions pkg/util/tracing/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,16 +210,12 @@ func (s *Span) IsRecording() bool {
//
// If parent != nil, the Span will be registered as a child of the respective
// parent.
// If separate recording is specified, the child is not registered with the
// parent. Thus, the parent's recording will not include this child.
func (s *crdbSpan) enableRecording(
parent *crdbSpan, recType RecordingType, separateRecording bool,
) {
func (s *crdbSpan) enableRecording(parent *crdbSpan, recType RecordingType) {
s.mu.Lock()
defer s.mu.Unlock()
atomic.StoreInt32(&s.recording, 1)
s.mu.recording.recordingType = recType
if parent != nil && !separateRecording {
if parent != nil {
parent.addChild(s)
}
if recType == SnowballRecording {
Expand Down Expand Up @@ -256,7 +252,7 @@ func (s *Span) StartRecording(recType RecordingType) {
// If we're already recording (perhaps because the parent was recording when
// this Span was created), there's nothing to do.
if !s.crdb.isRecording() {
s.crdb.enableRecording(nil /* parent */, recType, false /* separateRecording */)
s.crdb.enableRecording(nil /* parent */, recType)
}
}

Expand Down
15 changes: 0 additions & 15 deletions pkg/util/tracing/span_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,18 +183,3 @@ func (forceRealSpanOption) apply(opts spanOptions) spanOptions {
opts.ForceRealSpan = true
return opts
}

type withSeparateRecordingOpt struct{}

// WithSeparateRecording instructs StartSpan to configure any child span
// started via WithParent to *not* share the recording with that parent.
//
// See WithParent and WithRemoteParent for details about recording inheritance.
func WithSeparateRecording() SpanOption {
return withSeparateRecordingOpt{}
}

func (o withSeparateRecordingOpt) apply(opts spanOptions) spanOptions {
opts.SeparateRecording = true
return opts
}
8 changes: 1 addition & 7 deletions pkg/util/tracing/tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ func (t *Tracer) startSpanGeneric(opName string, opts spanOptions) *Span {
if opts.Parent != nil {
p = &opts.Parent.crdb
}
s.crdb.enableRecording(p, recordingType, opts.SeparateRecording)
s.crdb.enableRecording(p, recordingType)
}

if t.useNetTrace() {
Expand Down Expand Up @@ -503,12 +503,6 @@ func ChildSpan(ctx context.Context, opName string) (context.Context, *Span) {
return childSpan(ctx, opName, spanOptions{})
}

// ChildSpanSeparateRecording is like ChildSpan but the new Span has separate
// recording (see StartChildSpan).
func ChildSpanSeparateRecording(ctx context.Context, opName string) (context.Context, *Span) {
return childSpan(ctx, opName, spanOptions{SeparateRecording: true})
}

func childSpan(ctx context.Context, opName string, opts spanOptions) (context.Context, *Span) {
sp := SpanFromContext(ctx)
if sp == nil || sp.isNoop() {
Expand Down
3 changes: 2 additions & 1 deletion pkg/util/tracing/tracer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,12 @@ func TestStartChildSpan(t *testing.T) {

sp1 = tr.StartSpan("parent", WithForceRealSpan())
sp1.StartRecording(SingleNodeRecording)
sp2 = tr.StartSpan("child", WithParent(sp1), WithSeparateRecording())
sp2 = tr.StartSpan("child", WithParent(sp1))
sp2.Finish()
sp1.Finish()
if err := TestingCheckRecordedSpans(sp1.GetRecording(), `
Span parent:
Span child:
`); err != nil {
t.Fatal(err)
}
Expand Down

0 comments on commit 86bc2e3

Please sign in to comment.