OpenTracing breaking API change: SpanContext

[basvanbeek]

OpenTracing changed its API and deprecated the Inject method in favor of the new Extract method.

@bensigelman: can you comment if this conforms to an idiomatic OpenTracing solution

[bhs]

I guess (?) we could include the rpc server tag here even if there's no client SpanContext to reference...

[bhs]

LGTM, thanks!

[basvanbeek]

@bensigelman it looked very consumer unfriendly to add this:

opentracing.Tags{string(ext.SpanKind): ext.SpanKindRPCServer}

if we update the Apply function of RPCServerOption to this:

func (r rpcServerOption) Apply(o *opentracing.StartSpanOptions) {
    if r.clientContext != nil {
        opentracing.ChildOf(r.clientContext).Apply(o)
    }
    (opentracing.Tags{string(SpanKind): SpanKindRPCServer}).Apply(o)
}

we can change from this in Go kit and other consumers:

if wireContext != nil {
    span = tracer.StartSpan(operationName, ext.RPCServerOption(wireContext))
} else {
    span = tracer.StartSpan(operationName, opentracing.Tags{string(ext.SpanKind): ext.SpanKindRPCServer})
}

to simply this:

span = tracer.StartSpan(operationName, ext.RPCServerOption(wireContext))

I think it is a very common scenario for servers especially at the edge (API gateways and such) to start the trace with a RPC Server span without an upstream Client as the client often is not instrumented.

[bhs]

@basvanbeek I think that makes sense, yeah. Let's do it.

[peterbourgon]

We are still waiting for changes to be merged in openzipkin/zipkin-go-opentracing, is that right?

[basvanbeek]

correct... that will be ready today

[peterbourgon]

LGTM! You can merge, right?