util/tracing: optimize span tags and expand their collection #70993
Conversation
|
This change is |
pkg/util/tracing/tracer.go
Outdated
| @@ -520,6 +520,10 @@ func (t *Tracer) startSpanGeneric( | |||
| helper.crdbSpan.mu.operation = opName | |||
| helper.crdbSpan.mu.recording.logs = newSizeLimitedBuffer(maxLogBytesPerSpan) | |||
| helper.crdbSpan.mu.recording.structured = newSizeLimitedBuffer(maxStructuredBytesPerSpan) | |||
| helper.crdbSpan.mu.tags = helper.crdbSpan.mu.tagsAlloc[:0] | |||
There was a problem hiding this comment.
Instead of this tagsAlloc guy sitting on crdbSpanMu, you could just have it as part of the helper struct above, right?
This patch makes the collection of span tags generally allocation-free
by storing the tags into memory pre-allocated in the span. In tandem
with a previous commit which moved away from a generic (interface{})
interface for Span.SetTag(), this makes span collection very cheap. As
such, this patch also re-enables the collection of tags for
non-verbose (but also not no-op) spans. The collecting of tags on
non-verbose spans was stopped in
eba03c4 because of these allocations,
as well as the allocations performed by the variadic WithTags option
for StartSpan(). I'm not re-introducing WithTags.
The motivation for this change is that I want to make broader use of the
open spans registry, and I want (non-verbose) spans in the registry to
have their tags available for inspection. I'm also planning of
introducing new tags for static info defining a span (e.g. the SQL query
for spans representing query execution).
Note that tags in non-verbose spans continue to not be included in
Recordings (and hence not serialized-deserialized over the network).
This was the case, and the patch does not change it.
I've run BenchmarkTracing and it shows no change.
Release note: None
andreimatei
force-pushed
the
tracing.tags-always
branch
from
d5d82bf
to
a6c3d2c
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15 and @irfansharif)
pkg/util/tracing/tracer.go, line 523 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Instead of this
tagsAllocguy sitting oncrdbSpanMu, you could just have it as part of thehelperstruct above, right?
Yeah. I had oscillated about where to put it since I'm not sure how it'll end up looking when I start pooling the spans, but yeah for now at least I've moved it like you said.
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build failed (retrying...): |
|
Build succeeded: |
This patch makes the collection of span tags generally allocation-free
by storing the tags into memory pre-allocated in the span. In tandem
with a previous commit which moved away from a generic (interface{})
interface for Span.SetTag(), this makes span collection very cheap. As
such, this patch also re-enables the collection of tags for
non-verbose (but also not no-op) spans. The collecting of tags on
non-verbose spans was stopped in
eba03c4 because of these allocations,
as well as the allocations performed by the variadic WithTags option
for StartSpan(). I'm not re-introducing WithTags.
The motivation for this change is that I want to make broader use of the
open spans registry, and I want (non-verbose) spans in the registry to
have their tags available for inspection. I'm also planning of
introducing new tags for static info defining a span (e.g. the SQL query
for spans representing query execution).
Note that tags in non-verbose spans continue to not be included in
Recordings (and hence not serialized-deserialized over the network).
This was the case, and the patch does not change it.
I've run BenchmarkTracing and it shows no change.
Release note: None