Fix FollowsFrom reference clock skew adjuster #460
Conversation
A span that has a FollowsFrom reference to its parent should not be clock-adjusted, therefore we can consider it as a root span when building the spans subgraph in the clockskew adjuster. Signed-off-by: Adrian Bogatu <bogatu.adrian@gmail.com>
bogatuadrian
requested review from
black-adder,
pavolloffay,
vprithvi and
yurishkuro
as code owners
bogatuadrian
force-pushed
the
fix-follows-from-clock-skew
branch
from
11b4fd7 to
33f03b0
Compare
bogatuadrian
force-pushed
the
fix-follows-from-clock-skew
branch
from
33f03b0 to
6438d73
Compare
Signed-off-by: Adrian Bogatu <bogatu.adrian@gmail.com>
|
|
||
| // Treat spans that have a FOLLOWS_FROM reference to their parent as root spans. | ||
| for _, ref := range n.span.References { | ||
| if ref.RefType == model.FollowsFrom && ref.SpanID == n.span.ParentSpanID { |
There was a problem hiding this comment.
what if the References list has both ChildOf and FollowsFrom?
There was a problem hiding this comment.
Both referencing the parent? Well, first of all this should not happen, but if it does and there is at least one FollowsFrom to the parent I think the time should not be adjusted.
There was a problem hiding this comment.
first of all this should not happen
I don't know about that, there can be all kinds of DAGs people might want to model. My point is that the original implementation of the clock skew was ignoring the FollowsFrom altogether. If we want to support it properly, then the graph may need to be normalized better. I.e. it should be looking at all references, not just jump to a conclusion when we see one specific type. And it should not be relying on the parentId anywhere, only on references.
Parent ID is actually an optimization detail for the cases where there's a single ChildOf reference to that parent - the pattern occurs in the majority of spans, and representing it with a single ID is more efficient than with a ChildOf reference. But the processing should really be based on the references (which might require an enrichment step where add a ChildOf reference if we see parentId != 0 and len(references) == 0 - this is what we're doing before returning JSON to the UI, we could probably refactor the enrichment chain to do that before the clockskew).
There was a problem hiding this comment.
what if we consider the span as a root iff all of its references are FOLLOWS_FROM? Otherwise, we only add it as a child for parents where there is a CHILD_OF relationship?
As stated by @black-adder in #448, the clockskew adjuster does not handle spans that reference the parent span using FollowsFrom reference. This PR tries to fix that by considering spans that have that reference as root spans. This way, the span will not be adjusted (specifically will be adjusted with a skew with
delta = 0).Note: if you find the use of labels ugly, please comment and I will change it to the alternative approach.