Allow follows-from reference as a parent span id

[kubarydz]

Which problem is this PR solving?

Resolves #4380

Short description of the changes

handle FOLLOWS_FROM as a parent span ID

[yurishkuro]

if no unit tests break I think it's reasonable. But we always treated ChildOf as higher priority, so a better fix would be (a) search for ChildOf ref, (b) if not found, search for FollowsFrom() (this can be optimized to be done in one loop)

[kubarydz]

that makes sense, I will make adjustments

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5d6a2e8) 97.08% compared to head (6a76597) 97.08%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4382   +/-   ##
=======================================
  Coverage   97.08%   97.08%           
=======================================
  Files         300      300           
  Lines       17721    17729    +8     
=======================================
+ Hits        17205    17213    +8     
  Misses        415      415           
  Partials      101      101           

Flag	Coverage Δ
unittests	97.08% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
model/adjuster/clockskew.go	100.00% <ø> (ø)
model/span.go	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[yurishkuro]

please make sure all commits are signed (the DCO check), see CONTRIBUTING

[kubarydz]

right, sorry about that, should be ok now

[yurishkuro]

how would this change affect clock skew? #448

[kubarydz]

Well they would not be considered root spans, which they are now. Tbh now I'm not sure anymore if a span with follows-from reference but no child-of reference should be considered a root span.

[yurishkuro]

Conceptually the parentSpanID field represents an ancestor span, that happened-before the current span and is the cause of the current span. In that regard there is no strong difference between child-of or follows-from reference types, they actually tell more about the parent than about the child. I thought about this when OpenTelemetry model was designed and I was ok with only having a parent-span-id in OTEL, without explicit references like in OpenTracing. However, there is a difference between them for the clock alignment purpose. Namely, and naively, that the child-of span is supposed to finish before the parent is finished, which means we have both lower and upper boundary of how far the child span's timestamps can be moved. The same restriction does not exist with follows-from span because it can legitimately finished after the parent. But the reason I say "naively" is because even with child-of span there is actually no guarantee that it finishes before the parent. All we know is that child-of span sends a response to the parent before parent finishes, whereas finishing the child span may happen any time after response is sent (sometimes long after, due to async behavior). Which means that even for child-of spans the clock alignment can realistically only be done on their startTime.

Long story short, this just highlights the fact that our clock alignment algorithm is pretty simplistic at the moment. But even that simple algorithm could be broken if we treat the two ref types identically (namely, as child-of spans are treated), which is what I think will happen with this change because unfortunately the algorithm uses parentSpanID field instead of inspecting the references explicitly. Hence my concern with this change.

[kubarydz]

Thanks for the exhaustive answer.
It is now my understanding that follows-from can be used to determine parentSpanID everywhere except in the clock skew adjuster. If that's ok I would propose additional change in building sub graphs:

jaeger/model/adjuster/clockskew.go

Lines 123 to 126 in 5d6a2e8

	if n.span.ParentSpanID() == 0 {
	a.roots[n.span.SpanID] = n
	continue
	}

specifically here, since adjuster just wants to know if it should adjust the clock. So we could just ask the span if it should be adjusted (isChild(), shouldBeClockAdjusted(), isClockskewAdjustable() or something similar). Then the logic of clockskew adjuster would not be dependent on how we determine if span is root span.
I might still be missing something, but it seems to me like clockskew adjuster has a different idea of what a root span is, which creates a problem.

[yurishkuro]

I had a second look at the current logic in clockSkew adjuster was incorrect anyway, so let's leave it