This repository has been archived by the owner on Aug 30, 2019. It is now read-only.
Compute sublayers for non root toplevel spans #384
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bmermet
force-pushed
the
bmemermet/sublayers
branch
from
b5d51fb to
466906c
Compare
bmermet
changed the title
bmermet
force-pushed
the
bmemermet/sublayers
branch
from
0de20f4 to
0765f1c
Compare
bmermet
changed the title
LotharSee
reviewed
Feb 28, 2018
Gopkg.lock
Outdated
| @@ -25,7 +25,8 @@ | |||
| [[projects]] | |||
gbbr
reviewed
Feb 28, 2018
model/trace.go
Outdated
| @@ -122,3 +122,87 @@ func (t Trace) APITrace() *APITrace { | |||
| EndTime: end, | |||
| } | |||
| } | |||
|
|
|||
| // Subtrace represents the combination of a root and the trace consisting of all its descendant spans | |||
There was a problem hiding this comment.
You missed a word here.
Subtrace represents the combination of a root span and the trace consisting of all its descendant spans.
model/trace.go
Outdated
| Trace Trace | ||
| } | ||
|
|
||
| // spaAndAncestors is an internal type used by ExtractTopLevelSubtraces |
There was a problem hiding this comment.
Here there is also a typo. It would be nice to explain (for devs) what this type is, in the comment, as opposed to writing that it is internal, which is obvious from the fact that it's unexported.
model/trace.go
Outdated
| return value | ||
| } | ||
|
|
||
| // ExtractTopLevelSubtraces extracts all subtraces rooted in a toplevel span |
There was a problem hiding this comment.
Please add a comma at the end of this line and a period at the end of the second.
model/trace.go
Outdated
|
|
||
| visited := make(map[*Span]bool, len(t)) | ||
| subtracesMap := make(map[*Span][]*Span) | ||
| next := stack{} |
There was a problem hiding this comment.
I don't feel very strongly about this but in Go it's generally nicer as:
var next stack
Which has the same outcome.
bmermet
force-pushed
the
bmemermet/sublayers
branch
from
0765f1c to
5409140
Compare
LotharSee
approved these changes
Feb 28, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Until now sublayers were only computed for the root span of each trace. We now compute it for all non leaf top level spans of the trace. The purpose being to fix some gaps in the data currently shown in the UI.
This is implemented by extracting all relevant subtraces in each trace and computing the sublayers for each of theses subtraces individually.
One consequence of this change is that now the complexity of computing sublayers is no longer
O(s)withsthe number of spans, butO(s * t)withsthe number of spans andtthe number of top level spans. Which may have a very significant performance impact on some pathological traces.It has been deployed on staging and I haven't noticed any significant change on the CPU footprint from trace-agent.