Opentelemetry-specification: Move isRemote from SpanContext to Span

[anuraaga]

Now that we have PropagatedSpan in the spec, SpanContext.isRemote seems redundant. In practice, it would be difficult to prevent creation of a PropagatedSpan with a non-remote SpanContext for example. Does it make sense to move isRemote into Span, where it returns true only for PropagatedSpan?

[Oberon00]

We will lose the information whether a span has a remote parent then (which is currently available to exporters via the parent SpanContext in each span), which is IMHO one of the most important uses of the IsRemote flag.

[Oberon00]

Thinking about it a bit more, I really don't think we should do this: IsRemote does basically say: "Was this span ID generated by this SDK?" and so it should be stored next to the Span ID and not be easily separated from it. E.g. what happens when you put a SpanContext from some span into a propagated span? The IsRemote should be preserved IMHO.

[anuraaga]

I suggested moving it to Span, not removing it. Is there a functional difference?

For reference, I wasn't able to use the new propagated span concept, since currently the concept of propagation is in the SpanContext, not the Span. It makes propagated span awkward - there's nothing special about a propagated span.

https://github.com/open-telemetry/opentelemetry-java/pull/1791

[Oberon00]

I suggested moving it to Span, not removing it. Is there a functional difference?

Yes, I listed some in my comments. 😃

Maybe one more concrete example: Today you can have a propagated span with IsRemote=false by doing PropagatedSpan.create(myLocalSpan.getSpanContext()).

[anuraaga]

@Oberon00 Yeah - what does it mean for a propagated span to have a non-remote span context? I thought by definition that's not a propagated span. That confusion brought up the topic for me.

[Oberon00]

It means that you can rely on the Span ID having been generated by a trusted ID generator, e.g. you can rely on it being an uniform random integer number instead of an incremented integer counter. EDIT: It also means that the tracestate really applies directly to this span(context) as opposed to a parent span of it.

[anuraaga]

How does propagated span present this though? Is the expectation to do instanceof PropagatedSpan if a user needs to know the ID is a trusted ID? A PropagatedSpan is still a Span, so I don't see any way to actually use this piece of information.

If we rename PropagatedSpan to NoopSpan, then things make more sense to me - it is a Span that has no functionality. We use it for propagated spans, unsampled spans, and even invalid spans. Would this make more sense?

[Oberon00]

How does propagated span present this though?

I'm not sure what you mean here? proapgatedSpan.getSpanContext().isRemote()?

[anuraaga]

I'm not sure what you mean here? proapgatedSpan.getSpanContext().isRemote()?

Yeah so that's seeing whether it's remote or not by accessing the SpanContext - but propagated span has no special "propagatedness" involved in this case, it's just a wrapper. My expectation was that a propagated span indicates it's a remote span, but that's not always the case if it can have any SpanContext. If this isn't the intention of propagated span, than noop span seems like a closer definition.

[anuraaga]

I think the crux of my confusion is that PropagatedSpan is a type named after how it's used, not what it does it seems - I was expecting it to have some direct relationship with propagation. We'd usually name a type by what it does, since we don't know how it's used in practice - a span could be created, but maybe not propagated for whatever reason for example.

[Oberon00]

I think the naming is really a bit unfortunate. Maybe PropagateOnlySpan would be better since it can only be used to propagate (a SpanContext) not to record anything. Or NonRecordingSpan.

[andrewhsu]

from the spec sig mtg today, sounds like there could be a separate issue for editorial change on clarification on PropagatedSpan usage. @arminru created an issue for this

[jmacd]

@Oberon00 I was confused by this statement:

It means that you can rely on the Span ID having been generated by a trusted ID generator, e.g. you can rely on it being an uniform random integer number instead of an incremented integer counter. EDIT: It also means that the tracestate really applies directly to this span(context) as opposed to a parent span of it.

I guess the part that confuses me is the "parent span of it." Are you thinking of the PropagatedSpan instance as the "parent" in the sense that it reflects a remote Span? I would expect the tracestate of an isRemote PropagatedSpan to relate to the remote span, which only becomes a parent after a local Span is created. @Oberon00 could you clarify?

I came into this discussion thinking, as I suspect @anuraaga did, that isRemote meant that a SpanContexts had been propagated, so I agree with the original premise, that isRemote() should be true for PropagatedSpan and false otherwise. I would expect a propagated-in-process span to look like it was a remote child, because the context was propagated.

In the discussion above, it looks like some of us have been thinking of a PropagatedSpan as something like NoopSpan with real but non-recorded span identifiers, in that case the isRemote property cannot be derived by instanceof PropagatedSpan, but @Oberon00 if you need to know something for example about uniformity of the random number generator, shouldn't you convey the SDK's identity in tracestate, instead of relying on isRemote?

[Oberon00]

@jmacd

@Oberon00 I was confused by this statement:

... It also means that the tracestate really applies directly to this span(context) as opposed to a parent span of it ...

OK, that is confusing. I guess what I really need is an IsExtracted property (which is currently exactly equivalent to IsRemote).

The use case we currently use IsRemote for: We inject a copy of the span ID into the traestate like TENANT@dt=SPAN_ID, where TENANT is a unique identifier for the tenant/instance the injected span was exported to and SPAN_ID is the same as the one in traceparent. Why do we do this? If a W3C tracecontext participant that does not report to the same (Dynatrace) is somewhere in the request chain, it will break the trace if you only use the traceparent. With the TENANT@dt tracestate entry, we are able to find the closest parent span which was reported to this backend. EDIT: See also https://github.com/open-telemetry/opentelemetry-specification/issues/366#issuecomment-580235961

Without IsRemote, we would not be able to implement this: The tracestate is copied to every child span, but it is only meaningful for the "local root" span, the one with a remote parent, as there is no mechanism in the default SDK to update the tracestate for local child spans (I guess you could use the sampler since #988, but there you also would need to have IsRemote as input to decide what to do).

I don't have a use case for distinguishing otherwise non-propagated from propagated spans.

[jmacd]

from the spec SIG (APAC) we agreed that this issue can be closed. the SpanContext concept is conceptually the part of a Span that you can read, so wherever the span's identifiers go, that is where the "is remote" concept goes.

[trask]

the SpanContext concept is conceptually the part of a Span that you can read

I thought SpanContext is conceptually the part of the span that is needed for propagation?