proposal: expose NoopSpan to enable nil-object pattern

[marcind]

I find myself writing the following code:

func Method(ctx context.Contenxt) {
  span, ctx := startSpanConditionally(ctx)
  // Do some work
  if span != nil {
    span.Finish()
  }
}

func startSpanConditionally(ctx context.Context) (tracer.Span, context.Context) {
  if enableTracing {
    return tracer.StartSpanFromContext(ctx, "OpName")
  } else {
    return nil, ctx
  }
}

It would be great if NoopSpan was exposed so that I could return it from the helper and avoid the if span != nil check.

[marcind]

Note that I can obtain a NoopContext by calling tracer.SpanFromContext(nil) but then I have to discard the bool, which makes the code more verbose than it needs to be:

func startSpanConditionally(ctx context.Context) (tracer.Span, context.Context) {
  if enableTracing {
    return tracer.StartSpanFromContext(ctx, "OpName")
  } else {
    noopSpan, _ := tracer.SpanFromContext(nil)
    return noopSpan, ctx
  }
}

[gbbr]

I think you can avoid that... Why do you need the additional function? You could just have:

func Method(ctx context.Contenxt) {
    if enableTracing {
        span, ctx := tracer.StartSpanFromContext(ctx, "OpName")
        defer span.Finish()
    }
    // ...
}

Furthermore, do you know that if you skip calling tracer.Start, the entire API still works but causes no traces? For example:

if enableTracing {
    tracer.Start()
    defer tracer.Stop()
}
// ...
tracer.StartSpan("op") // this has no effect when the tracer is not started

[marcind]

The code I posted is a simplified example. The startSpanConditionally helper does a lot more work (including Extracting/Injecting metadata from a carrier, etc) so I was attempting to avoid allocations in case tracing is not enabled. Here's an example of a benchmark that shows the cost, even if the tracer was never started:

func Benchmark(b *testing.B) {
    for i := 0; i < b.N; i++ {
        tracer.StartSpanFromContext(context.Background(), "opName",
            tracer.ServiceName("the-service"),
            tracer.ResourceName("Foo"),
            tracer.SpanType(ext.SpanTypeMessageConsumer),
            tracer.Tag("A", "ABCD"),
            tracer.Tag("B", 1234),
            tracer.Measured(),
            tracer.Tag(ext.EventSampleRate, 1.0),
        )
    }
}

And the output is

goos: darwin
goarch: amd64
pkg: example.com/pkg
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
Benchmark-8      2224834           575.7 ns/op       632 B/op         14 allocs/op

Returning a Noop span causes no allocations (since it's a struct with no fields)

[marcind]

And to further clarify the conditionality is on a per-subsystem basis (so the global tracer might be Started, but I want to control whether tracing is on in a particular part of the code, for example to measure perf impact)

[gbbr]

I understand. I think it should be fine in that case to expose the NoopSpan. It should probably be part of the ddtrace package, in a noop.go file together with the NoopTracer. Any other placement could result in circular dependencies.

Would you be interested to submit a PR or should we mark this internally as a feature request?

[marcind]

@gbbr I'll see how I do time-wise. The tracer.SpanFromContext(nil) trick works well enough for now, so feel free to track this internally.

[ajgajg1134]

Hello! We're triaging issues currently, is this still something you're looking for?

[marcind]

@ajgajg1134 this has turned out not to be a priority for me, so if this does not fit your bandwidth feel free to close. I do still think it would be nice to look at perf/allocations in general, if you have an opportunity to work on it.

[darccio]

@marcind We are implementing it in #2498. It'll be released in our next major v2 version during the current quarter.