Skip to content

Commit

Permalink
tracing: assert against mixing tracers
Browse files Browse the repository at this point in the history
Creating a child with a different Tracer than the parent is
broken because the child span can leak: if the parent is Finish()ed
first, the child gets inserted into the parent's registry:
https://github.com/cockroachdb/cockroach/blob/d60e17a7047bb16b6aed1585bc5d274bd748d04e/pkg/util/tracing/crdbspan.go#L186

However, when the child gets Finish()ed, we attempt to remove it from
its Tracer's registry.

Mixing Tracers like this seems fundamentally a bad idea, because it is
inherently unclear what the behavior with respect to the registries
should be. This patch adds an assertion at span creation time checking
that the two Tracers involved are one and the same.

Release note: None
  • Loading branch information
andreimatei committed Nov 18, 2021
1 parent 151727a commit ac12ca2
Showing 1 changed file with 10 additions and 0 deletions.
10 changes: 10 additions & 0 deletions pkg/util/tracing/tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,16 @@ func (t *Tracer) startSpanGeneric(
// WithParentAndAutoCollection.
panic("invalid sterile parent")
}
if opts.Parent.Tracer() != t {
// Creating a child with a different Tracer than the parent is not allowed
// because it would become unclear which active span registry the new span
// should belong to. In particular, the child could end up in the parent's
// registry if the parent Finish()es before the child, and then it would
// be leaked because Finish()ing the child would attempt to remove the
// span from the child tracer's registry.
panic(fmt.Sprintf("attempting to start span with parent from different Tracer. parent: %s, child: %s",
opts.Parent.OperationName(), opName))
}
}

// Are we tracing everything, or have a parent, or want a real span, or were
Expand Down

0 comments on commit ac12ca2

Please sign in to comment.