tracing: make Span.crdbSpan a pointer #56238
Conversation
|
This change is |
tbg
force-pushed
the
tracing-nil-crdbspan
branch
from
3723545 to
29a8aab
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @irfansharif, @knz, @RaduBerinde, and @tbg)
pkg/util/tracing/span.go, line 186 at r1 (raw file):
tracer *Tracer // never nil // Internal trace Span; nil if not tracing to crdb.
[nit] mention that this is allocated together with the Span in a single object so it doesn't mean a separate allocation
tbg
force-pushed
the
tracing-nil-crdbspan
branch
from
29a8aab to
eb7aabb
Compare
A `Span` essentially fans out over any subset of the below:
- CRDB trace span
- net/trace span
- external opentracing-comatible tracer Span
The latter two are already pointers, which means that there is a
convenient check for their absence in tracing-internal code.
This wasn't true for crdbSpan, which has bitten me in the past.
Make things more idiomatic by using a pointer for this one, too.
As currently written, our tracer will always populate the crdbSpan
whenever net/trace or opentracing tracers are active. However,
there is no good reason to do that, and even if there were, it
should not reflect as a fundamental requirement in the code.
One nice outcome of this change is that the noopSpan is now
`&Span{tracer: t}`, and all fields except for the tracer are nil,
meaning that any bug around noop spans will quickly manifest as an NPE.
(One past bug involved erroneously assigning an operation name to the
noopSpan).
Other than a few clarification that resulted from the pointer change and
some cleanup around log tags, this commit also improves
`startSpanGeneric` to avoid populating a struct's internals after the
initial assignment, a pattern which I have found easier to get wrong.
Now, all ingredients are held in local variables, which are assigned to
`crdbSpan` wholesale.
Release note: None
tbg
force-pushed
the
tracing-nil-crdbspan
branch
from
eb7aabb to
8010c90
Compare
There was a problem hiding this comment.
bors r=RaduBerinde,irfansharif
TFTR!
Reviewable status:
pkg/util/tracing/shadow.go, line 100 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
I think we're missing a word here?
Done.
pkg/util/tracing/span.go, line 630 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[nit] Break this line up, too many parens.
Done.
pkg/util/tracing/tags.go, line 52 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[nit] swap the argument order here, will make the call sites look much nicer.
Done.
|
Build succeeded: |
A
Spanessentially fans out over any subset of the below:The latter two are already pointers, which means that there is a
convenient check for their absence in tracing-internal code.
This wasn't true for crdbSpan, which has bitten me in the past.
Make things more idiomatic by using a pointer for this one, too.
As currently written, our tracer will always populate the crdbSpan
whenever net/trace or opentracing tracers are active. However,
there is no good reason to do that, and even if there were, it
should not reflect as a fundamental requirement in the code.
One nice outcome of this change is that the noopSpan is now
&Span{tracer: t}, and all fields except for the tracer are nil,meaning that any bug around noop spans will quickly manifest as an NPE.
(One past bug involved erroneously assigning an operation name to the
noopSpan).
Other than a few clarification that resulted from the pointer change,
this commit also improves
startSpanGenericto avoid populating astruct's internals after the initial assignment, a pattern which I
have found easier to get wrong. Now, all ingredients are held in
local variables, which are assigned to
crdbSpanwholesale.Release note: None