From e79dbcfc2108997ab2856c25ee01ee526138ce12 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Wed, 28 Oct 2020 11:28:36 +0100 Subject: [PATCH] tracing: remove (*Tracer).SetForceRealSpans It was a testing crutch that is no longer necessary now that we've moved off the opentracing interfaces. Release note: None --- pkg/util/log/ambient_context_test.go | 21 ++++++++------------- pkg/util/log/trace_test.go | 9 +++------ pkg/util/tracing/tracer.go | 16 +--------------- 3 files changed, 12 insertions(+), 34 deletions(-) diff --git a/pkg/util/log/ambient_context_test.go b/pkg/util/log/ambient_context_test.go index 42f62466ef68..bbae949066df 100644 --- a/pkg/util/log/ambient_context_test.go +++ b/pkg/util/log/ambient_context_test.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/logtags" + "github.com/stretchr/testify/require" ) func TestAnnotateCtxTags(t *testing.T) { @@ -40,14 +41,13 @@ func TestAnnotateCtxTags(t *testing.T) { func TestAnnotateCtxSpan(t *testing.T) { tracer := tracing.NewTracer() - tracer.SetForceRealSpans(true) ac := AmbientContext{Tracer: tracer} ac.AddLogTag("ambient", nil) // Annotate a context that has an open span. - sp1 := tracer.StartSpan("root") + sp1 := tracer.StartRootSpan("root", nil /* logTags */, tracing.RecordableSpan) sp1.StartRecording(tracing.SingleNodeRecording) ctx1 := tracing.ContextWithSpan(context.Background(), sp1) Event(ctx1, "a") @@ -70,20 +70,15 @@ func TestAnnotateCtxSpan(t *testing.T) { t.Fatal(err) } - // Annotate a context that has no span. + // Annotate a context that has no span. The tracer will create a non-recordable + // span. We just check here that AnnotateCtxWithSpan properly returns it to the + // caller. ac.Tracer = tracer ctx, sp := ac.AnnotateCtxWithSpan(context.Background(), "s") - sp.StartRecording(tracing.SingleNodeRecording) - Event(ctx, "a") - sp.Finish() - if err := tracing.TestingCheckRecordedSpans(sp.GetRecording(), ` - Span s: - tags: ambient= - event: [ambient] a - `); err != nil { - t.Fatal(err) - } + require.Equal(t, sp, tracing.SpanFromContext(ctx)) + require.NotNil(t, sp) + require.True(t, sp.IsBlackHole()) } func TestAnnotateCtxNodeStoreReplica(t *testing.T) { diff --git a/pkg/util/log/trace_test.go b/pkg/util/log/trace_test.go index 7c804d56a1aa..d4d2de9b096c 100644 --- a/pkg/util/log/trace_test.go +++ b/pkg/util/log/trace_test.go @@ -65,8 +65,7 @@ func TestTrace(t *testing.T) { Event(ctx, "should-not-show-up") tracer := tracing.NewTracer() - tracer.SetForceRealSpans(true) - sp := tracer.StartSpan("s") + sp := tracer.StartRootSpan("s", nil /* logTags */, tracing.RecordableSpan) sp.StartRecording(tracing.SingleNodeRecording) ctxWithSpan := tracing.ContextWithSpan(ctx, sp) Event(ctxWithSpan, "test1") @@ -95,8 +94,7 @@ func TestTraceWithTags(t *testing.T) { ctx = logtags.AddTag(ctx, "tag", 1) tracer := tracing.NewTracer() - tracer.SetForceRealSpans(true) - sp := tracer.StartSpan("s") + sp := tracer.StartRootSpan("s", nil /* logTags */, tracing.RecordableSpan) ctxWithSpan := tracing.ContextWithSpan(ctx, sp) sp.StartRecording(tracing.SingleNodeRecording) @@ -182,8 +180,7 @@ func TestEventLogAndTrace(t *testing.T) { VErrEvent(ctxWithEventLog, noLogV(), "testerr") tracer := tracing.NewTracer() - tracer.SetForceRealSpans(true) - sp := tracer.StartSpan("s") + sp := tracer.StartRootSpan("s", nil /* logTags */, tracing.RecordableSpan) sp.StartRecording(tracing.SingleNodeRecording) ctxWithBoth := tracing.ContextWithSpan(ctxWithEventLog, sp) // Events should only go to the trace. diff --git a/pkg/util/tracing/tracer.go b/pkg/util/tracing/tracer.go index b2520f53af3b..07f6c3f79254 100644 --- a/pkg/util/tracing/tracer.go +++ b/pkg/util/tracing/tracer.go @@ -88,12 +88,6 @@ type Tracer struct { // x/net/trace or lightstep and we are not recording. noopSpan *Span - // If forceRealSpans is set, this Tracer will always create real spans (never - // noopSpans), regardless of the recording or lightstep configuration. Used - // by tests for situations when they need to indirectly create spans and don't - // have the option of passing the Recordable option to their constructor. - forceRealSpans bool - // True if tracing to the debug/requests endpoint. Accessed via t.useNetTrace(). _useNetTrace int32 // updated atomically @@ -145,14 +139,6 @@ func (t *Tracer) Close() { t.setShadowTracer(nil, nil) } -// SetForceRealSpans sets forceRealSpans option to v and returns the previous -// value. -func (t *Tracer) SetForceRealSpans(v bool) bool { - prevVal := t.forceRealSpans - t.forceRealSpans = v - return prevVal -} - func (t *Tracer) setShadowTracer(manager shadowTracerManager, tr opentracing.Tracer) { var shadow *shadowTracer if manager != nil { @@ -270,7 +256,7 @@ const ( // context. func (t *Tracer) AlwaysTrace() bool { shadowTracer := t.getShadowTracer() - return t.useNetTrace() || shadowTracer != nil || t.forceRealSpans + return t.useNetTrace() || shadowTracer != nil } // StartRootSpan creates a root Span. This is functionally equivalent to: