StartSpanFromContext() to use parentSpan.Tracer()

[Loofort]

Hello

I have a question.
Is there any reason why StartSpanFromContext() func uses GlobalTracer() instead parentSpan.Tracer() ?
Isn't it more logical to use the second option?

[yurishkuro]

If you look at it differently, when there is no parent span, then global tracer is the only option. So isn't it more logical to use the same tracer in both scenarios? Global tracer is an implicit dependency of that function.

[Loofort]

Well, my goal is to create child span that implements the same type (behavior) as a parent.
Probably you could suggest me a proper way to do that.

Let me describe a use case:
My service imports library that utilizes opentracing. Internally it may spawn goroutines with child spans.

The service should write logs records associated with a transaction. So I've made log wrapper for the global tracer.
This tracer also produces spans that encapsulate transaction info required for correct logging.

However, it seems there is no way to pass the transaction object to child spans. So the logger is unable to find the transaction object associated with parent span.

As a workaround I have an idea to encapsulate the transaction object to Tracer that could be obtained by span.Tracer()
This way we also get more flexible way in span creation. That could be suitable for big monolithic app - custom log/metrics workflow for particular routes (code branches)

The other idea is to use SpanContext , but it doesn't have a suitable method, so I'd have to make type assertion. and in general, it doesn't seem like proper way.

[yurishkuro]

You lost me a bit with "to pass the transaction object to child spans". If "transaction object" is something specific to your application and you want to propagate it similar to how the spans are propagated, you may want to store it explicitly in the Context.

As for StartSpanFromContext(), I personally never used this function, exactly because it requires a global tracer for at least one of its code paths (when there is no parent). I find it better to build instrumentation functions to expect the tracer to be passed explicitly, and StartSpanFromContext() is really simple to worry about repeating that code manually in the instrumentation.

[Loofort]

The "transaction object" indeed is something specific, that my custom tracer-logger wants to use.
Indeed this object is stored in context, however, the Tracer doesn't consider the context.Context while creating a span ( tracer.StartSpan() doesn't accept context )

StartSpanFromContext() is simple indeed, but it is part of public API that is used by third party packages.
And when you use those packages and pass a span created by your custom tracer (not global), you could face unexpected behavior.

I understand StartSpanFromContext should use GlobalTracer at least for cases when there is no parent span, and I'm not excited about it, but it looks to me like an acceptable trade-off.
Probably there is a better way (make another StartSpanFromContextByParentTracer func ? ).
My Point is that StartSpanFromContext produces not-that-you-may-expect.
I wonder is there any other reason except empty-parent case?

[bhs]

We could also export https://github.com/opentracing/opentracing-go/blob/master/gocontext.go#L48 ... though at that point one might be better off just calling tracer.StartSpan(...) directly.

[devoxel]

I think exporting that function would be a good call. I've had to re-implement it, which is fine, but there's no reason to leave it unexported, especially when this package allows for non-global tracers.

[cstockton]

@yurishkuro I don't think the function in #152 should be implemented, the behavior should be the default here. If a span is derived from a parent span, it should use the parent tracer. It's otherwise impossible for contextual tracing if using opentracing, since GlobalTracer is not thread safe and would affect all in-flight traces.

[yurishkuro]

There's this consideration:

it requires a global tracer for at least one of its code paths (when there is no parent).

[cstockton]

@yurishkuro I don't understand how that is relevant to this topic, can you clarify what I should consider? I can't see a reason for the Tracer() method on the Span if it is not used derive spans, it is impossible to create your own root span from a Tracer implementation. Is this something the library authors feel is correct & desired behavior?

[yurishkuro]

it is impossible to create your own root span from a Tracer implementation

why? this is exactly what happens, and there's no other tracer to use at that point except the global one.

opentracing-go/gocontext.go

Line 52 in 8b5a441

span := tracer.StartSpan(operationName, opts...)

[cstockton]

Well the code I audited is below, which as you can see always uses the global tracer:

func StartSpanFromContext(ctx context.Context, operationName string, opts ...StartSpanOption) (Span, context.Context) {
	return startSpanFromContextWithTracer(ctx, GlobalTracer(), operationName, opts...)
}

// startSpanFromContextWithTracer is factored out for testing purposes.
func startSpanFromContextWithTracer(ctx context.Context, tracer Tracer, operationName string, opts ...StartSpanOption) (Span, context.Context) {
	var span Span
	if parentSpan := SpanFromContext(ctx); parentSpan != nil {
		opts = append(opts, ChildOf(parentSpan.Context()))
		span = tracer.StartSpan(operationName, opts...)
	} else {
		span = tracer.StartSpan(operationName, opts...)
	}
	return span, ContextWithSpan(ctx, span)
}

It is from the latest tag v1.0.2, which I now see is over 2 years old. When will the bug fix (though it's labeled as a "complexity removal") 8013ba5 be ready for a release?

Edit:
I thought you were linking me a revision that fixes the behavior but it doesn't. I realize now you are talking about not having a root span, I still don't see how this is relevant. May we please focus on why the code below is written this way:

func StartSpanFromContext(ctx context.Context, operationName string, opts ...StartSpanOption) (Span, context.Context) {
	return startSpanFromContextWithTracer(ctx, GlobalTracer(), operationName, opts...)
}

// startSpanFromContextWithTracer is factored out for testing purposes.
func startSpanFromContextWithTracer(ctx context.Context, tracer Tracer, operationName string, opts ...StartSpanOption) (Span, context.Context) {
	if parentSpan := SpanFromContext(ctx); parentSpan != nil {
		opts = append(opts, ChildOf(parentSpan.Context()))
	}
	span := tracer.StartSpan(operationName, opts...)
	return span, ContextWithSpan(ctx, span)
}

Instead of:

func StartSpanFromContext(ctx context.Context, operationName string, opts ...StartSpanOption) (Span, context.Context) {
	return startSpanFromContextWithTracer(ctx, GlobalTracer(), operationName, opts...)
}

// startSpanFromContextWithTracer is factored out for testing purposes.
func startSpanFromContextWithTracer(ctx context.Context, tracer Tracer, operationName string, opts ...StartSpanOption) (Span, context.Context) {
	if parentSpan := SpanFromContext(ctx); parentSpan != nil {
		opts = append(opts, ChildOf(parentSpan.Context()))
		// when the parentSpan is non-nil, use that tracer.
		tracer = parentSpan.Tracer() 
	}
	span := tracer.StartSpan(operationName, opts...)
	return span, ContextWithSpan(ctx, span)
}

If you disagree with the above change, then my argument is this: just add Tracer() to the ever growing list of deprecated interfaces on Span as it provides no benefit because tracer is always GlobalTracer.

Now- if your counter argument here is that GlobalTracer is an ideal design I vehemently disagree with you. If you feel it is not ideal but driven by necessity based on the use case when StartSpanFromContext is called as the root span I would argue it was the wrong solution. Here is how I would have designed this:

rm -rf https://github.com/opentracing/opentracing-go/blob/master/globaltracer.go

The above adds 4 functions to this libraries API surface, all to simply assign and read from a global variable. It gives the impression that changing it at runtime is safe- because why else would a global variable be wrapped like this? An example is zap.L(). The below code would have been just fine and made it clear to API users that it was unsafe to change after the first concurrent read:

// DefaultTracer is the tracer used when none is found in the current context.
// 
// Users are encouraged to begin their root span via their tracer implementations
// StartSpan method.
var DefaultTracer = new(tracer.NoopTracer{})

If the goal was to allow changing the GlobalTracer then the underlying container should be an atomic value. SetGlobalTracer() and GlobalTracer() would have been more than enough here. While a contention free atomic load is cheap, it should only be the fallback for when a context has no Span.

But this API is already in wide spread use, so there is little we can do but march forward. I'm okay with that, with this "GlobalTracer" discussion out of the way I would like to focus on using the current spans tracer, because:

1. The current design makes it impossible to have more than 1 tracer within a process
2. The current design makes it impossible to prove a given package is correct and race free. You must audit every single package you import in your program carefully, to ensure under no circumstance they will call SetGlobalTracer() once the program has started.

When I first set out to use this library and saw API consumers are encouraged to set a global variable at initialization, experience told me: don't do that. Create your initial root span from a tracer to remove any question from your mind that a potential support package may inadvertently call SetGlobalTracer.

This turned out to be impossible,I feel I've made a valid argument for why it shouldn't be- and am unable to see a reason why it should remain this way moving forward. Can you point me to any documentation, rational or material of any kind to pivot my position?

[yurishkuro]

I have no love for global tracer or global variables in general. I'd perfectly happy if we put "not recommended" disclaimers on everything in globaltrace.go and on StartSpanFromContext. And I would leave it at that, rather than change the semantics of the current function by adding tracer = parentSpan.Tracer() line that you're proposing.

As for your later points "The current design makes it impossible ...", my recommendation is: don't use those "not recommended" helper methods. This whole discussion is about a helper function that was explicitly (rightly or not) designed to work with the global tracer, and that is 4 lines long. The only value this function provides is adding a parent if it exists, and it is forced to do it an inefficient manner by extending the options slice and potentially causing a memory allocation. Rather than fight about it, you can simply copy the 4 lines into your instrumentation and treat the tracer any way you want.

[cstockton]

I have no love for global tracer or global variables in general. I'd perfectly happy if we put "not recommended" disclaimers on everything in globaltrace.go and on StartSpanFromContext. And I would leave it at that, rather than change the semantics of the current function by adding tracer = parentSpan.Tracer() line that you're proposing.

Would you mind summarizing the technical reason for the bold portion of your quote? The only reason that comes to mind is backwards-compat, but I can't think of any use case or even accidental code that would rely on such behavior.

As for your later points "The current design makes it impossible ...", my recommendation is: don't use those "not recommended" helper methods. This whole discussion is about a helper function that was explicitly (rightly or not) designed to work with the global tracer, and that is 4 lines long. The only value this function provides is adding a parent if it exists, and it is forced to do it an inefficient manner by extending the options slice and potentially causing a memory allocation.

I think I am failing to convey my issue properly, sorry for that. Can you tell me how no longer using the function fixes my problem under this circumstance?

1. Call InitGlobalTracer to a log-only tracer.Tracer at the start of your program

2. Start N services within the process

3. Service X creates a jaeger tracer and root span via jagertracer.StartSpan(...) as suggested:

   my recommendation is: don't use those "not recommended" helper methods.

4. Service X calls into one of the many libraries that use opentracing.StartSpanFromContext

5. Observe: trace data is lost - libraries that I have no control over will use GlobalTracer resulting in a loss of trace data.

Rather than fight about it, you can simply copy the 4 lines into your instrumentation and treat the tracer any way you want.

I'm sorry if it comes across that way, that is not my intent. But I can't resolve this on my own as I've shown you here. I would really appreciate a technical discussion around the behavior because I believe it should be changed. I may concede to the experience of the package maintainer, or at least agree to disagree if you shared your technical perspective. Thanks.

[cstockton]

@yurishkuro Do you have any argument in favor of the existing behavior? As it stands I see this as a bug for the reasons I enumerated above.

[yurishkuro]

I think I already stated the arguments. The function has two code paths, it would be (a) backwards-incompatible, and (b) inconsistent if those two code paths would use different tracers. That was not the intention of that function, which was explicitly designed to work with global tracer. I don't see it as a bug in the function, it's part of the contract.

Your example is very specific and very non-standard for a normal application that is meant to work with a singleton tracer. I don't think that example is fundamentally compatible with any instrumentation that has a dependency on global tracer. Out of curiosity, which "many libraries" are you referring to that depend on global tracer? I just did a random spot-check among these https://opentracing.io/registry/?s=go, and a handful I looked at didn't use StartSpanFromContext.

[cstockton]

(a) backwards-incompatible,

Please explain an example which backwards compatible break is made in a correct Go program. The only way this could impact another application is if that program set a global tracer at the start of the program, then changed it later during a trace which had a context that carried a new tracer. This would be a race condition and against the documented best-practices to set it at the start of the program.

(b) inconsistent if those two code paths would use different tracers.

The entire API is contextual- Span https://godoc.org/github.com/opentracing/opentracing-go#Span comes from context, Span holds a Tracer. The function StartSpanFromContext makes no mention of "GlobalTracer":

https://godoc.org/github.com/opentracing/opentracing-go#StartSpanFromContext

// StartSpanFromContext starts and returns a Span with `operationName`, using any Span found
// within `ctx` as a ChildOfRef. If no such parent could be found, StartSpanFromContext creates a
// root (parentless) Span.
//
// The second return value is a context.Context object built around the returned Span.
//
// Example usage:
func SomeFunction(ctx context.Context, ...) {
    sp, ctx := opentracing.StartSpanFromContext(ctx, "SomeFunction")
    defer sp.Finish()
    ...
}

But it does mention using the Span from the current Context:

StartSpanFromContext starts and returns a Span with operationName, using any Span found
within ctx as a ChildOfRef.

I see nothing in this behavior that is intuitive, documented, desired or even safe. I am really trying to see a reason for your reluctance to concede or provide some form of merit, but feel at this point coming across as "upset" has cemented your position.

Your example is very specific and very non-standard for a normal application that is meant to work with a singleton tracer. I don't think that example is fundamentally compatible with any instrumentation that has a dependency on global tracer. Out of curiosity, which "many libraries" are you referring to that depend on global tracer? I just did a random spot-check among these https://opentracing.io/registry/?s=go, and a handful I looked at didn't use StartSpanFromContext.

I am completely lost here, what exactly are you arguing for / against?

Your example is very specific and very non-standard for a normal application that is meant to work with a singleton tracer. I don't think that example is fundamentally compatible with any instrumentation that has a dependency on global tracer.

"Normal applications use global tracer"

Out of curiosity, which "many libraries" are you referring to that depend on global tracer? I just did a random spot-check among these https://opentracing.io/registry/?s=go, and a handful I looked at didn't use StartSpanFromContext.

"I can't find any applications that use global tracer"

😕

To answer your question feel free to dig into the thousand repos here: https://github.com/search?l=Go&p=2&q=StartSpanFromContext&type=Code - even if it was exactly 1(it isn't)- it's a public exported function so I don't think it should stay broken.

Anyways, I didn't expect such a debate over such a tiny and obvious oversight in an otherwise great library, I'll move on if you're truly okay with this behavior. Thanks.

[evanj]

I must admit, while attempting to implement a simple logging tracer, I'm also a little confused about why the Span.Tracer method exists. I was expecting it to be used as suggested in this issue: To create child spans with the parent Tracer.

It seems at the very least we should update the package comments to clarify that within your program, all code needs to either use the global Tracer, or all code needs to always call StartSpanFromContextWithTracer. Mixing the two in a single program seems like a bad idea?