should StartSpanOptions implement StartSpanOption?

[RaduBerinde]

I found it a little strange that I couldn't pass a StartSpanOptions directly to StartSpan. I had to create a new type that implements Apply (https://github.com/cockroachdb/cockroach/blob/develop/util/tracing/tee_tracer.go#L42).

[yurishkuro]

You only pass individual StartSpanOption instances to StartSpan(), as shown here, and they populate the struct StartSpanOptions. The latter never needs to be passed directly to StartSpan.

Without referring to specific API details, can you describe what you're trying to do?

[RaduBerinde]

"Never" is a strong word - in my case I did already have a populated StartSpanOptions, and it was strange that I couldn't use it directly without jumping through some hoops.

The application here was a "Tee Tracer" - a tracer that keeps track of multiple tracers, and creates spans and "tees off" events to all of them. The StartSpan function creates a suitable StartSpanOptions for each "inner" tracer (link).

We are using this Tracer to push traces to both a Lightstep tracer and a "local" tracer which uses net/trace (we need them both because lightstep is not useful for stuck operations, where the interesting spans are stuck in open state).

[yurishkuro]

I would argue that since the actual API the user code must interact with can only provide individual options ...StartSpanOption, your wrapper tracer can simply keep that slice and pass it down to the underlying tracers.

Pre-populating the struct in this case is a minor performance optimization, and you've been able to do it without changing the public API.

Anyway, I don't have a strong objection, but a minor one is that it makes the API more confusing. For example what happens in this scenario?

tracer.StartSpan("name", 
  opentracing.Tags(map[string]interface{} {"key1": "value1"}),
  &opentracing.StartSpanOptions {
    Tags: map[string]interface{} {"key2": "value2"},
  }
)

In your current (private) implementation the struct as option overrides everything else.

[yurishkuro]

On unrelated note, people have asked before about a tee-like tracer, and tracer as a wrapper around net/trace is also an interesting idea. Maybe you would consider migrating these into opentracing-contrib?

[bhs]

Agree re a tee Tracer... the only tricky decision would be the treatment of Inject/Extract (where the "correct" / clean impl might have to duplicate a bunch of baggage data), though there are ways around that problem.

[RaduBerinde]

I would argue that since the actual API the user code must interact with can only provide individual options ...StartSpanOption, your wrapper tracer can simply keep that slice and pass it down to the underlying tracers.

Pre-populating the struct in this case is a minor performance optimization, and you've been able to do it without changing the public API.

This is not true. I have to convert References to tee tracer contexts to References to corresponding contexts for underlying tracers.

That said, I don't feel strongly about changing anything, it's more of a question. I see the argument against it and I'm totally ok keeping the code as is.

On unrelated note, people have asked before about a tee-like tracer, and tracer as a wrapper around net/trace is also an interesting idea. Maybe you would consider migrating these into opentracing-contrib?

@tschottdorf already contributed the NetTraceIntegrator to basictracer-go. The tee tracer implementation we have right now is kind of hacky; it assumes all tracers are instances of basictracer and it only Inject/Extracts the first tracer's context . This is because we're using basictracer.Delegator and wanted to keep that as is, and we don't care about correlating spans between hosts for the second (net/trace) tracer. If we make it more general we will consider migrating it.

[yurishkuro]

StartSpanOptions should be a private type. This will be superseded by #174