SkyAPM / go2sky

Distributed tracing and monitor SDK in Go for Apache SkyWalking APM
https://skywalking.apache.org/
Apache License 2.0
448 stars 123 forks source link

Span context is needed after CreateExitSpan #125

Closed kagaya85 closed 3 years ago

kagaya85 commented 3 years ago

In some scenarios (such as in middleware chain),span context is needed after doing CreateExitSpan, but the CreateExitSpan method won't return the context that include span info right now, even can't get span info from a span object

For the solution, IMHO, we can change the CreateExitSpan to return this context directly like what CreateEntrySpan & CreateLocalSpan do, but this may make a BC:

// New CreateExitSpan
func (t *Tracer) CreateExitSpan(ctx context.Context, operationName string, peer string, injector propagation.Injector) (Span, context.Context, error)

Another choice I considered is to expand the Span interface methods, add new methods like SpanID, TraceID, Context, etc.

// Context will return a context includes current span
func (ds *defaultSpan) Context(ctx context.Context) context.Context {
    return context.WithValue(ctx, ctxKeyInstance, *ds)
}

we can create a new methods collection interface with the current Span interface embedded, just assert to new interface when using new methods

Please give me some advice, thanks in advance :)

wu-sheng commented 3 years ago

I think both works, but we need to keep API forward compatbility. Does option 1 break nothing?

kagaya85 commented 3 years ago

Does option 1 break nothing?

It returns a new value context.Context, the numbers of return value change from 2 to 3

arugal commented 3 years ago

but we need to keep API forward compatbility.

I'm leaning toward option 2, and we can also add a CreateExitSpanWithContext func.

Like this:

func (t *Tracer) CreateExitSpanWithContext(ctx context.Context, operationName string, peer string, injector propagation.Injector) (Span, context.Context, error)
wu-sheng commented 3 years ago

We should not break API, as we are at 1.x stable iteration stage. I agree with @arugal, a new API makes more sense. CreateExitSpanWithContext is preferred by me.

kagaya85 commented 3 years ago

Okay, I'll get to work on it.