Make edge direction flow from client->server.

[tomwilkie]

So it turns out the layout engine we use is already trying to layout the graph from top-to-bottom: https://github.com/cpettitt/dagre/wiki

This change uses a simple heuristic (client port is the higher port) to orient the edges in the graph such that they flow from client to server.

It somewhat improves the graph layout. Before:

After:

[peterbourgon]

I don't think you can just swap which end of the connection is used as the key in the adjacency map. We assume that every key in the NodeMetadata map has representation in the adjacency map, too. I'm surprised this doesn't break rendering.

What about just using this change in isolation?

[tomwilkie]

That change you refer to is a noop (its the default value). I didn't realise I'd committed it.

[tomwilkie]

This next change reverses the assumption that keys in the Adjacency map always exist in the NodeMetadata map. It makes the pseudo node generator take a direction flag, and updates the tests to reflect reality.

One side effect of this PR is previously, when you have a probe running on two different machines, communicating processes on those machines would have two edges in the UI.

A negative side effect is that now, in the details panel, one only sees outgoing connections from the selected node. I intend to address this in this PR, next.

[peterbourgon]

edges only exist in the endpoint topology

This isn't a constraint of the data model. It's just an accident of the implementation. You can't treat it as an invariant and program against it.

But I was thinking — rather than restructuring the adjacency list, why don't you just indicate the client-to-server directionality as a boolean field in the EdgeMetadata?

[tomwilkie]

This isn't a constraint of the data model. It's just an accident of the implementation. You can't treat it as an invariant and program against it.

I'm not; I was pointing out that whilst Paul had spotted a bug, the reality was it was never going to be hit. I fixed it anyway.

But I was thinking — rather than restructuring the adjacency list, why don't you just indicate the client-to-server directionality as a boolean field in the EdgeMetadata?

I didn't consider this at the time; what would it buy us?

There is an inherent directionality in the adjacency map; previously it was from local->remote. This approach changed the assumption without changing the datastructure. With this approach there is no duplication of information (previously two edges would have existed between communicating nodes, now only one). Also no UI changes are required, as the UI already assumes the edges are directional, although it was probably just a coincidence.

[peterbourgon]

I'm not; I was pointing out that whilst Paul had spotted a bug, the reality was it was never going to be hit. I fixed it anyway.

Okeedokey.

I didn't consider this at the time; what would it buy us?

We would be able to keep the invariant that NodeMetadatas[id] —implies→ Adjacency[id], which I think is a nice property. But...

There is an inherent directionality in the adjacency map; previously it was from local->remote. This approach changed the assumption without changing the datastructure. With this approach there is no duplication of information (previously two edges would have existed between communicating nodes, now only one). Also no UI changes are required, as the UI already assumes the edges are directional, although it was probably just a coincidence.

I agree with everything here. I'm thinking out loud what "directionality" actually means for us. The edges have both ingress and egress counters so they actually represent both directions of data flow. Therefore, we don't actually have a directed graph, we just have a graph. And the directionality encoded by the adjacency list is kind of meaningless. Well, it has the meaning we assign to it: previously that was (reliably) local-to-remote, now it is (heuristically) client-to-server.

It seems to me that we are trading the property I listed above for more code in rendering and a potentially nicer graph layout. Is that right?

[tomwilkie]

The graph is still directed; previous I would say it was undirected, as in most situations having a (src, dst) edge meant you also had a (dst, src) edge. Now you'll only get a single (highest port number, lowest port number) edge - which looks much nicer in the UI, and is factually more correct. The edge represents a tcp connection, not a dataflow. The directionality is super meaningful, but the edgemetadata needs to be updated to deal with it (ingress and egress swapped when appropriate).

I'd phrase this change more as adding more semantics to the topology, which in turns makes the rendering slightly nicer. I don't see how the property of each node metadata key being a valid key in adjacencies was particularly useful - we pretty much only relied on it in one place (the leaf map code). And we were mistakenly relying on both directions of an edge to exist in detailed node rendering, so this change improves that.

[tomwilkie]

@peterbourgon I'd like to get this merged today; whats left to do?

[peterbourgon]

Nothing left to do, I was just giving it some brain time to see if there were potentially other problems, but I couldn't come up with any. 👍

[tomwilkie]

Cool, no problem. I'll file a follow up ticket to ensure we handle the directionality correcting in EdgeMetadata; currently the only populated field is tcp connection count, which isn't directional, so its fine. But when we get the bandwidth stuff enabled by default, it will bite us.