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

Add connection tables to details panel #1017

Merged
merged 6 commits into from
Mar 4, 2016
Merged

Conversation

tomwilkie
Copy link
Contributor

Part of #680, Fixes #378, Fixes #1111

This change also:

TODO:

  • make it sort by count, by default
  • change table ID as per davkal's request
  • make connection work to/from the internet node
  • figure out why the memoisation causes the test to fail
  • testing / coverage
  • add real nodes to the table
  • figure out why inbound count != outbound count for self-connections
  • better table for the internet node
  • use reverse dns info (Use reverse DNS info #1121)

remotes[key] = remotes[key] + 1
}
}
}

This comment was marked as abuse.

@davkal
Copy link
Contributor

davkal commented Feb 24, 2016

How about making connections a toplevel node field:

{
metadata,
metrics,
connections: [
  {
    id: "out",
    label: "Outgoing Connections",
    columns: [{id: "label", label: "Remote"}, {id: "port", label: "Port"}, {id: "count", label: "Count"}}],
    items:[{id: <nodeId>, label: <nodeLabel>, port: <port>, count: <count>, topologyId: <topologyId>}, ...]
  },
  {id: in, ...}
]}

@tomwilkie tomwilkie changed the title Add connection tables to details panel [WIP] Add connection tables to details panel Feb 24, 2016
@tomwilkie tomwilkie self-assigned this Feb 24, 2016
@tomwilkie
Copy link
Contributor Author

@davkal I eventually want the rows to link to other objects in the topology (ie link to the other container talking to this one) like the other children tables. Links will always be within the current view. Can you update your datastructure for this? I'm hoping links will always be possible, but you might want to make them optional.

@davkal
Copy link
Contributor

davkal commented Feb 24, 2016

Alternatively we can model connections as a nodes table, i.e. wrapping ports as metadata (typed as int to make it sortable as a number) and counts wrapped as a metric:

{
metadata,
metrics,
children,
connections: [{
    id: "out",
    label: "Outgoing Connections",
    columns: [{id: "label", label: "Remote"}, {id: "port", label: "Port"}, {id: "count", label: "Count"}}],
    nodes: [{
      id: <nodeId>,
      label: <nodeLabel>,
      topologyId: <topologyId>,
      metadata: [{port: <port>, type: "number"}]
      metrics: [{count: <count>}]
    }, ...]
  },
  {id: in, ...}

@davkal
Copy link
Contributor

davkal commented Feb 24, 2016

topologyId: "endpoint-incoming" needs to be id: "endpoint-incoming". topologyId is something like processes that will be used together with a node's id to build a deeplink.

@tomwilkie tomwilkie force-pushed the 680-connections-table branch 2 times, most recently from 0495ffb to 7b98683 Compare February 25, 2016 15:24
@@ -54,27 +54,6 @@ func TestAll(t *testing.T) {
}
}

func TestAPITopologyContainers(t *testing.T) {
ts := topologyServer()

This comment was marked as abuse.

@tomwilkie tomwilkie changed the title [WIP] Add connection tables to details panel Add connection tables to details panel Feb 25, 2016
@tomwilkie tomwilkie removed their assignment Feb 25, 2016
remotePortKey = "Remote Port"
localPortKey = "Local Port"
countKey = "Count"
numberWang = "number"

This comment was marked as abuse.

This comment was marked as abuse.

@paulbellamy
Copy link
Contributor

still quite a bit of tricky string-munging code shared between the incoming connections and outgoing connection aggregators.

@paulbellamy
Copy link
Contributor

Connections Fr... is not a great table name.
screen shot 2016-02-25 at 16 17 40

Edit: When you sort by that column, both just become Connections ...

@paulbellamy
Copy link
Contributor

I guess no luck on converting the endpoints to their respective nodes in the relevant topology?

@tomwilkie
Copy link
Contributor Author

I guess no luck on converting the endpoints to their respective nodes in the relevant topology?

Time constraint I'm afraid. Will do it next sprint.

@paulbellamy
Copy link
Contributor

Aside from a few naming things and refactoring (if possible), this LGTM.

@tomwilkie
Copy link
Contributor Author

@paulbellamy I think I've addressed all the feedback, and got it working for the internet node. Its a bit of a hack. I think we should punt this form 0.13 and finish it properly. Thoughts?

@tomwilkie
Copy link
Contributor Author

Also think the internet is going to need a custom table (for outgoing connections):

screen shot 2016-02-25 at 19 10 27

@tomwilkie
Copy link
Contributor Author

Going to hold this until #566 is merged. Also need @pidster input on the table for the internet node(s).

@tomwilkie tomwilkie force-pushed the 680-connections-table branch 2 times, most recently from 7a96ce3 to be0784f Compare March 3, 2016 10:34
Also:
- Make metadata fields sortable as numbers
- Sort by selected metadata column
// MakePseudoEndpointID makes a pseudo endpoint node ID for rendered nodes.
func MakePseudoEndpointID(hostID, addr, port string) string {
return makeID("pseudo-endpoint", hostID, addr, port)
}

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@paulbellamy
Copy link
Contributor

General approach seems fine. A couple naming and tidiness things.

paulbellamy added a commit that referenced this pull request Mar 4, 2016
Add connection tables to details panel
@paulbellamy paulbellamy merged commit c74cefb into master Mar 4, 2016
@paulbellamy paulbellamy deleted the 680-connections-table branch March 4, 2016 16:42
ID: "foo",
Value: row.localAddr,
Datatype: number,
})

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants