Jaeger UI visualizing span with multiple parents

[rubenvp8510]

Which problem is this PR solving?

• See description on Issue Issue with Jaeger UI visualizing span with multiple parents #467

Short description of the changes

• Virtualize of the spaces in multiple parents (repeating the span in each parent), using an icon to indicate which spans has more than one parent, also were able to navigate to those parents (or highlight it)

[rubenvp8510]

@everett980 I attached some screenshots of this, Could you please provide me some feedback? Thanks!

[yurishkuro]

Are there changes to the main timeline, or is the addition only the new icon on the expanded spans?

When the span is expanded I would rather show an additional section References, after Tags and Logs, where refs can be shown in a table, with links. That seems more usable than a mouse over pop up.

[rubenvp8510]

Are there changes to the main timeline, or is the addition only the new icon on the expanded spans?

There are one change to the timeline, before this PR the "poll" span was only displayed on the first parent, now is showing twice (per each parent)

Also an icon to indicate that span has more parents

When the span is expanded I would rather show an additional section References, after Tags and Logs, where refs can be shown in a table, with links. That seems more usable than a mouse over pop up.

That was my second choice, and something I was about to ask, if the accordion section is better I can do the change.

[yurishkuro]

There are one change to the timeline, before this PR the "poll" span was only displayed on the first parent, now is showing twice (per each parent)

Unless there are some explicit visual indications of a duplicate, I think showing the span twice is very misleading. A span represents some work, this work happened once.

I think accordion fits more consistently with the existing design of span details.

[rubenvp8510]

There are one change to the timeline, before this PR the "poll" span was only displayed on the first parent, now is showing twice (per each parent)

Unless there are some explicit visual indications of a duplicate, I think showing the span twice is very misleading. A span represents some work, this work happened once.

Exactly, that is why I added an icon, any other suggestion on how to handle this?

I think accordion fits more consistently with the existing design of span details.

[yurishkuro]

But the icon is only visible when you expand the span. The visual difference needs to exist in the gantt chart itself. Maybe showing the span bar hollow, I don't know, I am not a UX designer. Even hollow bar is not enough imo (also, when the span is visually very short, hollow wouldn't even work).

[everett980]

Should descendants of spans with multiple references also be duplicated?
As is, it looks like the diff makes descendants of spans with multiple references become* descendants of the root span instead.

[rubenvp8510]

I agree we should show something in the Gantt char (still not sure how ) , also in the left view of the spam, May be something after the operation name?

But I think showing the "span" per parent is not too bad, the span preserves his ID, so it is clear that is the same span. If we don't show it duplicated , showing only on the first parent, we are missing the fact that there are other references for that span.

[yurishkuro]

I definitely think the children should not be repeated. Maybe the multi-parent span should only have full details in one place in the chart, and all other copies of that span (a) shown visually different, e.g. a hollow box, and (b) when expanded just display a link to the first occurrence where full details can be seen.

[objectiser]

(b) when expanded just display a link to the first occurrence where full details can be seen

Instead of two steps (i.e. displaying a link) - when the duplicate is expanded, it just actually expands the first occurrence and focus on that span?

[yurishkuro]

Instead of two steps (i.e. displaying a link) - when the duplicate is expanded, it just actually expands the first occurrence and focus on that span?

it might lead to poor user experience, as the first instance of the span could be far away, so the screen would need to scroll, etc., making the user lose the context. An explicit link avoids that.

[rubenvp8510]

Should descendants of spans with multiple references also be duplicated?

Well good question, this PR does that, but reading the discussion I'm not sure that is a correct way to proceed. The thing is, an span with multiple parents represent more than one path in the trace.

As is, it looks like the diff makes descendants of spans with multiple references descendants of the root span instead.

[rubenvp8510]

I definitely think the children should not be repeated. Maybe the multi-parent span should only have full details in one place in the chart, and all other copies of that span (a) shown visually different, e.g. a hollow box, and (b) when expanded just display a link to the first occurrence where full details can be seen.

I agree with a), for b) Is not less usable need to go to another place to see the full details?

[yurishkuro]

Is not less usable need to go to another place to see the full details?

if you follow that argument, then why not show all children of that repeated span as well. I don't think it plays well. For all practical purposes that span is not really there, as each span has only one true parent (e.g. in OpenTelemetry the parent is enforced, whereas in Jaeger we infer the parent from one or more refs).

[everett980]

To avoid duplicating spans, could we add two interact-able icons?

The first next to the name of the span with multiple references. If there is one other reference, clicking the icon would navigate to its other reference. If there are more than two references, all references but the first would be in a dropdown. Clicking a ref in the dropdown would similarly navigate to the that reference.

The second icon would be on any span that is referenced but is rendered with a different parent span. Clicking this icon would navigate to that reference. Likewise this would need to have a dropdown view if it has been referenced by multiple other spans rendered elsewhere.

[rubenvp8510]

To avoid duplicating spans, could we add two interact-able icons?

The first next to the name of the span with multiple references. If there is one other reference, clicking the icon would navigate to its other reference. If there are more than two references, all references but the first would be in a dropdown. Clicking a ref in the dropdown would similarly navigate to the that reference.

The second icon would be on any span that is referenced but is rendered with a different parent span. Clicking this icon would navigate to that reference. Likewise this would need to have a dropdown view if it has been referenced by multiple other spans rendered elsewhere.

I like this idea, I can do the modifications to this PR.

[codecov]

Codecov Report

Merging #477 into master will increase coverage by 0.08%.
The diff coverage is 98.36%.

@@            Coverage Diff             @@
##           master     #477      +/-   ##
==========================================
+ Coverage   92.79%   92.87%   +0.08%     
==========================================
  Files         194      197       +3     
  Lines        4702     4757      +55     
  Branches     1132     1151      +19     
==========================================
+ Hits         4363     4418      +55     
+ Misses        299      298       -1     
- Partials       40       41       +1

Impacted Files	Coverage Δ
...ts/TracePage/TraceTimelineViewer/SpanDetailRow.tsx	100% <ø> (ø)	⬆️
...s/jaeger-ui/src/components/TracePage/url/index.tsx	100% <ø> (ø)
...age/TraceTimelineViewer/SpanDetail/DetailState.tsx	96.42% <100%> (+0.59%)	⬆️
...kages/jaeger-ui/src/model/transform-trace-data.tsx	88.5% <100%> (+0.41%)	⬆️
.../components/TracePage/TraceTimelineViewer/duck.tsx	98.47% <100%> (+0.07%)	⬆️
...TracePage/TraceTimelineViewer/ReferencesButton.tsx	100% <100%> (ø)
...ePage/TraceTimelineViewer/VirtualizedTraceView.tsx	92.62% <100%> (+0.25%)	⬆️
...nents/TracePage/TraceTimelineViewer/SpanBarRow.tsx	64.28% <100%> (+4.28%)	⬆️
...TracePage/TraceTimelineViewer/SpanDetail/index.tsx	100% <100%> (ø)	⬆️
...-ui/src/components/TracePage/url/ReferenceLink.tsx	100% <100%> (ø)
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f2a05d...24a3759. Read the comment docs.

[rubenvp8510]

I attached some screenshots with the suggested changes, I can change the icons if you have a better idea of what icons I should use, I can also add a references accordion in the details if that helps.

[yurishkuro]

@everett980 is this good to merge?

[rubenvp8510]

@everett980 @yurishkuro What is the status of this? Any other comment that needs to be addressed?

Thanks!

[everett980]

Sorry this took so long to get to. I left a lot of feedback but each item should be pretty small (except for possible refactoring AccordianReferences)

Big picture it looks good, thanks for the PR @rubenvp8510!

[rubenvp8510]

@everett980 Thanks for the review!

I think I covered almost all (if not all) the comments/requested changes, except the accordion stuff which I think would be in another PR.

[everett980]

Thanks for the quick turn around! Tomorrow morning I'll re-review (I started to and what I saw so far looks good) and will create the follow up issue for Accordian consolidation.

[everett980]

Currently it seems safe to treat Boolean(ref.span) as an indication of a local span reference. transform-trace-data only populates ref.span if it is in data.spans (source)

[everett980]

I don't recall hitting submit feedback (hence the lack of any top-level message to go with it)... but anyway:
@rubenvp8510 The PR looks good. I left four little things that would be good to tweak but if you don't have time before a target UI release of tomorrow I can probably just make the changes myself.

[everett980]

LGTM 👍I'll make that follow up ticket now