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.

Sign up for GitHub

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

Fix FollowsFrom reference clock skew adjuster #460

Closed
wants to merge 2 commits into from
Closed

Fix FollowsFrom reference clock skew adjuster #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9e2a9e
Fix clock skew for spans with FollowsFrom reference.
bogatuadrian Oct 6, 2017
6438d73
Add test case for FollowsFrom clockskew fix
bogatuadrian Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion model/adjuster/clockskew.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,21 @@ func (a *clockSkewAdjuster) buildNodesMap() {
// or points to an ID for which there is no span.
func (a *clockSkewAdjuster) buildSubGraphs() {
a.roots = make(map[model.SpanID]*node)
IterateSpans:
for _, n := range a.spans {
// TODO handle FOLLOWS_FROM references
if n.span.ParentSpanID == 0 {
a.roots[n.span.SpanID] = n
continue
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the References list has both ChildOf and FollowsFrom?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

a.roots[n.span.SpanID] = n
continue IterateSpans
}
}

if p, ok := a.spans[n.span.ParentSpanID]; ok {
p.children = append(p.children, n)
} else {
Expand Down
9 changes: 9 additions & 0 deletions model/adjuster/clockskew_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func TestClockSkewAdjuster(t *testing.T) {
host string
adjusted int // start time after adjustment
adjustedLogs []int // adjusted log timestamps
refs []model.SpanRef
}

toTime := func(t int) time.Time {
Expand Down Expand Up @@ -66,6 +67,7 @@ func TestClockSkewAdjuster(t *testing.T) {
model.String("ip", spanProto.host),
},
},
References: spanProto.refs,
}
trace.Spans = append(trace.Spans, span)
}
Expand Down Expand Up @@ -112,6 +114,13 @@ func TestClockSkewAdjuster(t *testing.T) {
{id: 2, parent: 1, startTime: 0, duration: 50, host: "a", adjusted: 0},
},
},
{
description: "do not adjust child that follows from parent",
trace: []spanProto{
{id: 1, parent: 0, startTime: 10, duration: 100, host: "a", adjusted: 10},
{id: 2, parent: 1, startTime: 110, duration: 100, host: "a", adjusted: 110, refs: []model.SpanRef{{RefType: model.FollowsFrom, SpanID: 1}}},
},
},
{
description: "do not adjust child that fits inside parent",
trace: []spanProto{
Expand Down