Add edge arrows

[jpellizzari]

Implements #872 and #875:

This deviates from the original design in that the edges do not terminate at the arrow. In order to shorted the edge to match the position of the arrow, we would need to subtract some length from the edge and re-calculate the path of the edge.

[rade]

Does this also show the arrows in the normal view, on mouseover, as discussed?

[jpellizzari]

@rade I had forgotten about that, but it was fairly easy to add:

[rade]

Nice!

I think this is good enough to close #872 and #875, though I suggest filing a follow up issue to adjust node position based on connection direction in the radial view, as per #875 (comment) and #1636 (comment).

[rade]

Just to check, the arrow(s) also appear on mouseover of an edge, right? (right now this just highlights the edge)

[jpellizzari]

They will appear when the edge is in a 'highlighted' state:
const shouldRenderMarker = (focused || highlighted) && (source !== target);

Where marker is the arrow. Mousing over a node will highlight its edges; mousing over an edge will highlight the edge.

[jpellizzari]

adjust node position based on connection direction in the radial view

I had a version of that implemented, where I placed all clients one side and all servers on another. It didn't make a lot of sense with real data, as most nodes either had one or the other. You ended up with a bunch of wasted space on one side.

I do think, however, that it will be possible to keep the nodes in their current radial shape, but place client and server nodes next to each other in the radial shape.

[rade]

I do think, however, that it will be possible to keep the nodes in their current radial shape, but place client and server nodes next to each other in the radial shape.

I am not sure what you mean by that, but the basic idea in #1636 (comment) is to draw the radial shape exactly as now, with exactly as many "slots", but when it comes deciding which node to put in which slot, we bias positioning near the top for clients, near the bottom for servers, and left/right for client/servers.

A further improvement on that would be to maybe add a small gap between the resulting groups.

Anyway, all this should be subject of a follow-up issue. Merge, merge, merge :) (post review, of course)

[jpellizzari]

Hmm, I thought I had this working but it turns out that I got a false positive on the bidirectional connections between the 'consul' containers. There may be an issue with the way the <path /> elements are rendered by D3 that is messing up the behavior of the markerStart attribute.

One thing that would make this implementation much simpler would be if we remove the collapseMultiEdges behavior. Then two edges get rendered for bidirectional connections and the arrows just work.

@fbarl Do you have a sense for how much of a render improvement we get from that optimization? Do you think it would be okay to remove it for a quick win on these edge arrows?

The alternative will be a complex custom solution in which we have to calculate the edge direction to get a polygon to point the way, rather than having the browser SVG engine render it for us.

[rade]

I suggest you gather some stats on what percentage of edges get collapsed. Take something like Weave Cloud Prod, and check the process, container, pod (all namespaces), service (all namespaces) and host views.

I suspect the host view is going to have a very high ratio. And it's already looking like a bowl of spaghetti.

Perhaps don't show any arrows for bidirectional connections?

[fbarl]

Perhaps don't show any arrows for bidirectional connections?

This sounds like a good idea, but I'm worried about two potential downsides:

1. The bidirectional edges will be drawing less attention than the directed ones (while in my opinion, it should be either the same or more)
2. Will it look like a bug, as if the arrow hasn't been drawn by mistake?

Maybe both of these would be resolved by positioning the nodes well in the radial view though...

One thing that would make this implementation much simpler would be if we remove the collapseMultiEdges behavior. Then two edges get rendered for bidirectional connections and the arrows just work.

From the performance perspective, I'd say go ahead and remove it, I think the effects will mostly be negligible. However, even if the two edges would be drawn perfectly on top of one another, I think there would be cases when they would look slightly different - e.g. on hover, since the semi-transparent blue border would be applied twice. Because of these tiny effects, I think it would be really preferable to draw always max one edge between a pair of nodes, but if it's really proving to be hacky and difficult to make it work with collapsed edges, I'd give my vote for the quick win :)

[jpellizzari]

Some stats on the number of edges being collapsed (bidrectionalEdges / totalEdges):

Hosts: .24
Containers (all): .03
Pods (all namespaces): .02
Processes: .03
Services: .02

As expected the hosts view has the most bidrectional edges. The rest have only a few.

[rade]

That's from Weave Cloud Prod?

Here's another idea: Could you "uncollapse" edges when you want to show arrows on them? Or is that either hard or does not help solve the problem?

[jpellizzari]

I thinks this is ready for review now. @fbarl would you mind taking a look?

My thoughts on this is that its an 80/20 solution that's not perfect, but I think the upside is worth it. The graph is a lot more informative now. I removed the opacity on the edges so that two edges laying on top of each other are indistinguishable from single edges. The hover background is slightly more opaque on bidirectional edges, but I think I only noticed because I knew it was there.

Some samples:

I would like this to be more extensible, but the SVG <marker /> element is fairly restrictive. Some alternatives would be adding waypoints to our path elements and trying to use markerMid, but that might involve digging into the dagre library.

[rade]

so you got bidirectional edges working properly after all?

[jpellizzari]

That's from Weave Cloud Prod?

@rade Yes thats a report from prod

"uncollapse" edges when you want to show arrows on them

Since the arrows appear/disappear on mouseover, we would need to re-render on each those events, so I don't think it would be worth it.

[jpellizzari]

@rade yes, but I cheated by just rendering two edges for bidirectional connections

[rade]

Oh ok. Do you have a before/after of the Weave Cloud Prod hosts spaghetti bowl? I am curious how much worse it looks.

[jpellizzari]

@rade The edges themselves haven't changed. You won't even notice the bidirectional edges because they follow the same path and lay on top of each other. We removed them to save cycles on renders because you can't see them.

Prod:

This branch:

[rade]

You won't even notice the bidirectional edges because they follow the same path and lay on top of each other

Aha! So once again then... Merge, merge, merge :) (post review, of course)

[fbarl]

Looks and behaves great! Here's some thoughts/comments about the UI:

• When you hover on a node in big graphs, some of its outgoing nodes might be outside of the viewport in which case you don't see the arrow at all. Have you thought about showing both incoming & outgoing arrows close to the selected/hovered node instead of close to the nodes they point at? I think it might also look nicer in the circular view.
• Currently the selected node label can obscure some arrows in the circular view, which is especially true in the contrast mode where there the labels are fully opaque. Not sure what can be done there, but maybe I'd increase the label transparency in that case as edge arrows should be fully visible in the circular view.
• I'm not sure Make it clear which nodes have incoming connections and which nodes have outgoing connections when you have a node selected #875 should be closed before the nodes in the circular view are reordered so that the incoming ones are always above the outgoing ones. Or did we decide that we're not implementing this in another story?

[jpellizzari]

@fbarl

When you hover on a node in big graphs, some of its outgoing nodes might be outside of the viewport in which case you don't see the arrow at all. Have you thought about showing both incoming & outgoing arrows close to the selected/hovered node instead of close to the nodes they point at? I think it might also look nicer in the circular view.

This was brought up during the design phase. On big topologies, I think it is better to see the arrow closest to the target node. It allows for a faster understanding of the way connectivity is working. I think the highlighted edge that disappears off screen is enough to indicate that another connection is happening at an unseen node.

Currently the selected node label can obscure some arrows in the circular view, which is especially true in the contrast mode where there the labels are fully opaque. Not sure what can be done there, but maybe I'd increase the label transparency in that case as edge arrows should be fully visible in the circular view.

Increasing the label transparency would make the text clash with the arrow. Ideally, we would be able to detect if the two were on top of one another and act accordingly. Since the edges and nodes are rendered separately, we can't get the bondingRect to know if a collision is happening. Also, we would have to save each node and edge as a ref and interact with the DOM directly ☹️ .

That said, I would like to defer this to a future iteration and try to fade out the selected node label on edge hover maybe.

I'm not sure #875 should be closed before the nodes in the circular view are reordered so that the incoming ones are always above the outgoing ones. Or did we decide that we're not implementing this in another story?

I think that's fair. Updated the PR description.

[fbarl]

That said, I would like to defer this to a future iteration and try to fade out the selected node label on edge hover maybe.

That solution also crossed my mind, but yeah, let's keep it in mind for some future PR. LGTM! :)