Replace span relationship with a potentially remote parent context #451

krnowak · 2020-01-29T12:03:37Z

This PR removes the non-compliant ChildOf and FollowsFrom interfaces
and the Relation type, which were inherited from OpenTracing via the
initial prototype. The logic I propose is here, which says to use the
remote span context if its TraceID differs from the current Span.

The opentracing bridge will likely need to see more work, but I'd like to punt it to the separate PR later.

This PR was a part of #381.

MrAlias

Looks good overall. Only comment is on adding a guard against a nil pointer dereference.

MrAlias · 2020-01-29T23:53:31Z

api/trace/context.go

+
+// RemoteContext gets the current core.SpanContext from a Context.
+func RemoteContext(ctx context.Context) core.SpanContext {
+	if sc, ok := ctx.Value(remoteContextKey).(core.SpanContext); ok {


~~This will panic if ctx.Value(remoteContextKey) returns a nil interface.~~

My assertion this will cause a panic is incorrect. Forget I made this suggestion.

MrAlias

I was incorrect about needing the guard.

Everything looks good.

rghetia

LGTM except for one comment regarding possible missing test-case.

rghetia · 2020-01-30T00:53:44Z

api/trace/api.go

+//     a. If a current span is set with the same trace_id, use the span's
+//        SpanContext as parent.
+//     b. If the current span belongs to a different trace_id, use the remote
+//        context as parent.


Is condition 1.b tested?
I looked at the tests but didn't find one. Maybe it is there and I just missed it.

I had also proposed that the current trace_id be used to decide whether to take the current span or the remote context, and in that discussion was convinced (by @mwear) that we should do the simpler thing. Always use the span as parent if there is one, then fall back to the remote. It means that, in some cases, we'll have to clear the current span in order to use the remote context, which I think is acceptable. See here: open-telemetry/opentelemetry-specification#423

krnowak · 2020-01-30T17:06:25Z

The longer I look at this PR, the more convinced I become that WithParent is not a way to go. I feel uneasy when I see that the tracer may need to somehow deal with two contexts (one passed as a first parameter to the Start function, and one potentially passed with the WithParent option). As it is in the PR currently, when WithParent is used, we completely ignore the context passed explicitly to the Start function. This may be confusing if the Otel user had code like:

func doStuff(ctx context.Context, …) {
	someStuff := getSomeStuff()
	ctx = context.WithValue(ctx, myStuffKey, someStuff)
	ctx, span := getTracer().Start(ctx, WithParent(WithRemoteParent(context.Background(), conjureSomeSpanContext())))
	…
	// much, much later, somewhere deeper in the call stack
	// panic, ctx is a background one with some span context
	someStuff := ctx.Value(myStuffKey).(stuffType)
	…
}

So far, we avoided that pitfall in examples and tests, because we usually passed the same context as a first parameter to Start and to the WithParent option, or we passed a throw-away context to Start and a useful context to WithParent, like:

ctx, span := tracer.Start(ctx, WithParent(WithRemoteParent(ctx, someSC)))
ctx2, span2 := tracer.Start(context.Background(), WithParent(WithRemoteParent(ctx, someSC)))

This is just lame, IMO.

I'd propose the following:

drop WithParent option
add functions for clearing current span and remote span context from context (just in case someone really wants a new root)

I'll try to implement that and also see how opentracing bridge can cope with it.

rghetia · 2020-01-30T17:51:04Z

The clearing of remote span context can be implicit with option WithNewRoot.

	ctx, span := getTracer().Start(ctx, WithNewRoot())

This option would create a new span and add link to remote context and existing span context.
If this option is not present then
use SpanContext as parent.
use RemoteSpanContext as parent if SpanContext is not present
use new root if both SpanContext and RemoteSpanContext are not present.

jmacd · 2020-01-30T19:58:51Z

Although I wrote the WithParent() option in the earlier, I think I agree with @krnowak that we shouldn't keep it. Independent of the WithNewRoot() proposal here, the user can just pass one context, either the extracted context to use the remote parent, or some other context to use the current span.

krnowak · 2020-01-31T20:30:19Z

Updated. No more WithParent, got the WithNewRoot option. Fixed opentracing bridge, but otel spans created with the opentracing bridge are always going to have a remote parent. Tough luck.

rghetia · 2020-02-03T17:25:06Z

internal/trace/parent/parent.go

+	return append(links, trace.Link{
+		SpanContext: sc,
+		Attributes: []core.KeyValue{
+			key.String("ignored-on-demand", kind),


I am not sure about ignored-on-demand. kind (key/value) should be defined in sdk-spec.
How about 'link-type'?

Yeah, wasn't sure about the name, but I clearly wanted to preserve some information that we are not making some span context a parent of a span that's about to be created because of the WithNewRoot() option. So I wanted some information that tells that this remote/local span context could be a parent, but isn't, because the code creating the span decided so. Not sure if link-type conveys that. possible-remote-parent or possible-local-parent? There are quite a mouthful already.

For now just open an issue.

Good idea, will do.

This PR removes the non-compliant ChildOf and FollowsFrom interfaces and the Relation type, which were inherited from OpenTracing via the initial prototype. Instead allow adding a span context to the go context as a remote span context and use a simple algorithm for figuring out an actual parent of the new span, which was proposed for the OpenTelemetry specification. Also add a way to ignore current span and remote span context in go context, so we can force the tracer to create a new root span - a span with a new trace ID. That required some moderate changes in the opentracing bridge - first reference with ChildOfRef reference type becomes a local parent, the rest become links. This also fixes links handling in the meantime. The downside of the approach proposed here is that we can only set the remote parent when creating a span through the opentracing API.

krnowak · 2020-02-03T21:16:00Z

Rebased.

krnowak requested review from iredelmeier, jmacd, lizthegrey, paivagustavo, rghetia and tedsuo as code owners January 29, 2020 12:03

MrAlias reviewed Jan 30, 2020

View reviewed changes

MrAlias approved these changes Jan 30, 2020

View reviewed changes

rghetia reviewed Jan 30, 2020

View reviewed changes

krnowak force-pushed the krnowak/span-relationship branch 2 times, most recently from 5821f91 to 2806145 Compare January 31, 2020 20:27

rghetia reviewed Feb 3, 2020

View reviewed changes

rghetia approved these changes Feb 3, 2020

View reviewed changes

krnowak mentioned this pull request Feb 3, 2020

Better attributes for ignored parents #461

Closed

jmacd approved these changes Feb 3, 2020

View reviewed changes

MrAlias approved these changes Feb 3, 2020

View reviewed changes

krnowak force-pushed the krnowak/span-relationship branch from 2806145 to 24f6505 Compare February 3, 2020 21:15

krnowak mentioned this pull request Feb 4, 2020

Make tracer to add ignored parents as links as an option, add initial semantic conventions for links open-telemetry/opentelemetry-specification#436

Closed

rghetia merged commit 942713a into open-telemetry:master Feb 4, 2020

krnowak deleted the krnowak/span-relationship branch February 4, 2020 20:01

MrAlias mentioned this pull request Feb 11, 2020

Add @MrAlias as an approver #471

Merged

6 tasks

rghetia mentioned this pull request Feb 13, 2020

Remove "FollowsFrom" relationships #193

Closed

lizthegrey mentioned this pull request Mar 4, 2020

use NewRoot() standard code. #513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace span relationship with a potentially remote parent context #451

Replace span relationship with a potentially remote parent context #451

krnowak commented Jan 29, 2020

MrAlias left a comment

MrAlias Jan 29, 2020 •

edited

Loading

MrAlias Jan 30, 2020

MrAlias left a comment

rghetia left a comment

rghetia Jan 30, 2020

jmacd Jan 30, 2020

krnowak commented Jan 30, 2020

rghetia commented Jan 30, 2020

jmacd commented Jan 30, 2020

krnowak commented Jan 31, 2020

rghetia Feb 3, 2020

krnowak Feb 3, 2020

rghetia Feb 3, 2020

krnowak Feb 3, 2020

krnowak Feb 3, 2020

krnowak commented Feb 3, 2020

Replace span relationship with a potentially remote parent context #451

Replace span relationship with a potentially remote parent context #451

Conversation

krnowak commented Jan 29, 2020

MrAlias left a comment

Choose a reason for hiding this comment

MrAlias Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrAlias left a comment

Choose a reason for hiding this comment

rghetia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak commented Jan 30, 2020

rghetia commented Jan 30, 2020

jmacd commented Jan 30, 2020

krnowak commented Jan 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnowak commented Feb 3, 2020

MrAlias Jan 29, 2020 •

edited

Loading