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

Fix orphaned spans bug #2956

Merged
merged 2 commits into from Jan 15, 2020
Merged

Fix orphaned spans bug #2956

merged 2 commits into from Jan 15, 2020

Conversation

@tacigar
Copy link
Member

tacigar commented Jan 8, 2020

Fix an obvious bug pointed out in #2922 .

Mainly change the calculation method of shownTree.
Previously, span ID was used to calculate the span's parent-child relationship and Expand/Collapse buttons were implemented using that parent-child relationship.
This created the orphaned span problem pointed out in #2922 because orphaned spans don't have parent-child relationship using span IDs.

Please see here. Orphan spans are simply appended to the root span:

if (!parent) { // Handle headless by attaching spans missing parents to root
this._rootSpan.addChild(child);
} else {
parent.addChild(child);
}

This PR uses span indices to calculate parent-child relationship instead of span IDs.

p.s. TraceSummary component is fairly complex, so I wanted to create custom hooks and improve readability, but I'm not doing it now to minimize PR.

Before

before-orphaned

After

after-orphaned

@tacigar tacigar added ui bug labels Jan 8, 2020
@tacigar tacigar requested review from jcchavezs and anuraaga Jan 8, 2020
@tacigar

This comment has been minimized.

Copy link
Member Author

tacigar commented Jan 8, 2020

let skip = false;

return rerootedTree.reduce((acc, cur) => {
if (cur.depth > depth && skip) {

This comment has been minimized.

Copy link
@anuraaga

anuraaga Jan 8, 2020

Contributor

This logic is pretty complex, can you add some comments? In particular, this propagation of skip is confusing me.

This comment has been minimized.

Copy link
@tacigar

tacigar Jan 9, 2020

Author Member

I simplified this logic.
As you said, skip is unnecessary... 😓

let depth = 0;
let skip = false;

return rerootedTree.reduce((acc, cur) => {

This comment has been minimized.

Copy link
@anuraaga

anuraaga Jan 8, 2020

Contributor

Let's use semantic names instead of acc, cur - I think it's shownTraces, trace?

@tacigar

This comment has been minimized.

Copy link
Member Author

tacigar commented Jan 14, 2020

PTAL 🙇

@adriancole

This comment has been minimized.

Copy link
Contributor

adriancole commented Jan 14, 2020

🙇 on behalf of all

@tacigar tacigar merged commit d772581 into openzipkin:master Jan 15, 2020
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@tacigar tacigar deleted the tacigar:tacigar/orphaned-span branch Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.