Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move isRemote from SpanContext to Span #1086

Closed
anuraaga opened this issue Oct 13, 2020 · 16 comments
Closed

Move isRemote from SpanContext to Span #1086

anuraaga opened this issue Oct 13, 2020 · 16 comments
Assignees
Labels
area:api Cross language API specification issue area:span-relationships Related to span relationships release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory

Comments

@anuraaga
Copy link
Contributor

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?

@anuraaga anuraaga added the spec:trace Related to the specification/trace directory label Oct 13, 2020
@Oberon00
Copy link
Member

Oberon00 commented Oct 13, 2020

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.

@arminru arminru added the area:api Cross language API specification issue label Oct 13, 2020
@Oberon00
Copy link
Member

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
Copy link
Contributor Author

anuraaga commented Oct 13, 2020

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.

open-telemetry/opentelemetry-java#1791

@Oberon00
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

Oberon00 commented Oct 13, 2020

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
Copy link
Contributor Author

anuraaga commented Oct 13, 2020

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
Copy link
Member

How does propagated span present this though?

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

@anuraaga
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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

@andrewhsu andrewhsu added the release:after-ga Not required before GA release, and not going to work on before GA label Oct 13, 2020
@jmacd jmacd self-assigned this Oct 13, 2020
@jmacd
Copy link
Contributor

jmacd commented Oct 13, 2020

@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
Copy link
Member

Oberon00 commented Oct 14, 2020

@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 #366 (comment)

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.

@Oberon00 Oberon00 added the area:span-relationships Related to span relationships label Oct 14, 2020
@jmacd
Copy link
Contributor

jmacd commented Oct 20, 2020

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.

@jmacd jmacd closed this as completed Oct 20, 2020
@trask
Copy link
Member

trask commented Oct 21, 2020

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue area:span-relationships Related to span relationships release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

6 participants