tracing: introduce a minimal tracing mode and enable in tests #73407
Conversation
|
This change is |
andreimatei
force-pushed
the
tracing.add-tracing-mode
branch
2 times, most recently
from
5e44f7b to
7a5262d
Compare
andreimatei
requested review from
miretskiy and
erikgrinaker
and removed request for
a team
There was a problem hiding this comment.
This feels significant, in a good way, and I'm excited that we're heading in a direction where we think a tracing mode like this is possible from a performance perspective. I just had one question regarding a comment, but otherwise
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15 and @andreimatei)
pkg/util/tracing/tracer.go, line 160 at r12 (raw file):
// ... If no // net.Trace/OpenTelemetry tracer is attached and a span is not explicitly // asked to record, spans do not record events.
This comment is slightly confusing to me. When we say "record events" here are we talking about log events (e.g. Eventf class of functions)?
andreimatei
force-pushed
the
tracing.add-tracing-mode
branch
from
7a5262d to
ffd489f
Compare
There was a problem hiding this comment.
I'm excited that we're heading in a direction where we think a tracing mode like this is possible from a performance perspective
You and me both. I think we're almost there.
Reviewable status:
pkg/util/tracing/tracer.go, line 160 at r12 (raw file):
Previously, abarganier (Alex Barganier) wrote…
// ... If no // net.Trace/OpenTelemetry tracer is attached and a span is not explicitly // asked to record, spans do not record events.This comment is slightly confusing to me. When we say "record events" here are we talking about log events (e.g.
Eventfclass of functions)?
Right. "recording" refers to spans accumulating log messages (log.Info/Eventf() calls which turn into span.Record()), as well as accumulating a history of their finished children.
I've improved the comment some; please see now.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic.go, line 170 at r25 (raw file):
// enabled. This is equivalent to setting COCKROACH_EXPERIMENTAL_SPAN_CONFIGS. // - tracing-off: If specified, tracing defaults to being turned off. This is // used to override the environment, which may ask for ttracing to be on by
ttracing
There was a problem hiding this comment.
Reviewable status:
pkg/util/tracing/tracer.go, line 160 at r12 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Right. "recording" refers to spans accumulating log messages (
log.Info/Eventf()calls which turn intospan.Record()), as well as accumulating a history of their finished children.
I've improved the comment some; please see now.
Thanks for clarifying, lgtm!
This test checks that a job record is not updated with a trace id if tracing the job is not enabled. This test fails if tracing is generally enabled across the cluster. The code under test is this: https://github.com/cockroachdb/cockroach/blob/96a125ade349979f7ff7a42e23f39aa544c5529f/pkg/jobs/adopt.go#L401-L408 And, I'm trying to make it such that tracing is always enabled across the cluster. So, I'm deleting this test, and also morally accepting that there will usually be an extra update to the job record. Hopefully, always getting that trace id in the job is a good thing. Release note: None
This patch makes the Tracer's behavior more configurable. Before, the
Tracer would generally not create spans when there was no explicit
reason to do so, unless a testing knob option was passed to the Tracer
creation (TracerTestingKnobs{ForceRealSpans: true}). This patch elevates
this configuration to a proper tracer option. The Tracer can be
configured to always create spans (useful for having all the in-flight
operations represented in the active spans registry), to not do that
(the old behavior), or to read the configuration from an env var. This
env var is useful while I test various things with the goal of
eventually enabling this tracing mode everywhere. For now the env var
defaults to tracing being off, except in CrdbTestBuild's (i.e. unit
tests), where it's on.
Some tests are changed to explicitly request tracing to be off, so that
they retain the old behavior, as they were relying on it. Among them, a
logic tests needs it, so a new cluster configuration option is added to
logic tests and test clusters.
After this patch, I think we're pretty close to enabling this new
tracing mode in production. Before that though, there's a couple more
performance optimizations to make, and more testing against
span-use-after-Finish.
Release note: None
Tests are using this option when creating spans when they care about tracing. Now that the Tracer can easily be configured to always create spans, that's generally a cleaner options for tests then repeating this option with every span creation. Release note: None
andreimatei
force-pushed
the
tracing.add-tracing-mode
branch
from
ffd489f to
c1c4adf
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic.go, line 170 at r25 (raw file):
Previously, aayushshah15 (Aayush Shah) wrote…
ttracing
Done.
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
|
Build succeeded: |
This patch makes the Tracer's behavior more configurable. Before, the
Tracer would generally not create spans when there was no explicit
reason to do so, unless a testing knob option was passed to the Tracer
creation (TracerTestingKnobs{ForceRealSpans: true}). This patch elevates
this configuration to a proper tracer option. The Tracer can be
configured to always create spans (useful for having all the in-flight
operations represented in the active spans registry), to not do that
(the old behavior), or to read the configuration from an env var. This
env var is useful while I test various things with the goal of
eventually enabling this tracing mode everywhere. For now the env var
defaults to tracing being off, except in CrdbTestBuild's (i.e. unit
tests), where it's on.
Some tests are changed to explicitly request tracing to be off, so that
they retain the old behavior, as they were relying on it. Among them, a
logic tests needs it, so a new cluster configuration option is added to
logic tests and test clusters.
After this patch, I think we're pretty close to enabling this new
tracing mode in production. Before that though, there's a couple more
performance optimizations to make, and more testing against
span-use-after-Finish.
Release note: None