fix internet connection counts #1843
Conversation
For counting we were using a table keyed on a struct containing Node pointers. For connections between ordinary nodes this works just fine. But for connections to/from the Internet node we want to track individual address/port combinations, which involves an extra lookup. Since our data structures generally contain values, not pointers, this produces aliases. As a result n connections from/to a node to/from a specific Internet IP+port would result in n rows in the count table, each with a count of 1, instead of one row with a count of n. Things wouldn't be so bad if it was actually rendered like that - annoying, but at least accurate - but... Each row has an ID which is computed from the node IDs and ports. Not Node references. The ID must be unique - the frontend will only render *one* thing per ID. Since the row IDs of our n rows are all the same, we see one row with a count of 1 instead of n rows with a count of 1. Furthermore, since the frontend's table row limiting is counting rows, not unique row IDs, a) fewer rows would be rendered than expected, and b) the displayed count of the number of extra rows would be too high. The fix is to replace the Node pointers in the key with Node IDs. This does require an extra table lookup when we come to produce the rows, but that is just a fairly cheap map lookup. Fixes #1495.
|
@paulbellamy this does perhaps deserve a unit test, but I am not familiar with all the data structures involved, so this is a bit daunting. If you do think this should have a test, would you mind helping me write one? |
| @@ -115,7 +110,7 @@ func incomingConnectionsSummary(topologyID string, r report.Report, n report.Nod | |||
| TopologyID: topologyID, | |||
| Label: "Inbound", | |||
| Columns: columnHeaders, | |||
| Connections: connectionRows(r, counts, isInternetNode(n)), | |||
| Connections: connectionRows(r, ns, counts, isInternetNode(n)), | |||
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
yes, please! Edit: There is an example of a "unit test" for the detailed node rendering in: |
and refactor the connection{} construction so we can add some
explanation of what is going on here.
|
I'm now getting the following in the details panel for the Internet: and with +51 (which works as expected). Having multiple rows with all values identical looks confusing. I'm guessing those 3 Y rows, are 3 different containers with the same name. UX remedies:
|
That would break the navigation, i.e. I need to be able to navigate to the 3 individual containers, so need 3 click-able things.
That would be best, but we are really short on horizontal space here.
That might be the only choice left :( Please file an issue. |
|
Filed #1844 for UI issues. |
|
LGTM |
|
I've given up on constructing a test for this. It's just too hard, for me at least, not being familiar with the entire report data structure, and the intricacies of |
Fixes #1495.