Details panel redesign #752

davkal · 2015-12-11T17:10:08Z

This PR is to redesign the details panel that appears when clicking on a node. Fixes #382, fixes #301.

tomwilkie · 2016-01-04T17:04:04Z

tomwilkie · 2016-01-05T14:33:47Z

Some feedback on go code (high level, not line by line):

Pls don't commit app/scope-app
In the probe, instead of adding ID to the Node.Metadata, we could make a field for it (RenderableNode has it, so I'd move it from RenderableNode to Node). Constructors would also need updating, and Topology.AddNode could just take one argument.
I like render/metadata.go. Perhaps the detailed node rendering deservers its own package now?
I'm not convinced by the use of Rank to decide what order to merge nodes in. Merge should be commutative. Why was this needed again?
I think we should promote the various 'parent' foreign keys out of Node.Metadata into a Node.Parent field, of type []struct{topology, nodeid string}, with set-like merge semantics. Doing this will allow us to efficiently gather all the parents recursively, and potentially simplify all the foreign key joins in the rendering code, and might even solve the commutative merge problem.
please make changes to tools/test in tools.git

paulbellamy · 2016-01-06T11:24:50Z

Responses inline:

Pls don't commit app/scope-app

d'oh!

In the probe, instead of adding ID to the Node.Metadata, we could make a field for it (RenderableNode has it, so I'd move it from RenderableNode to Node). Constructors would also need updating, and Topology.AddNode could just take one argument.

Sounds nicer, will have a look into how much shared code there is

I like render/metadata.go. Perhaps the detailed node rendering deservers its own package now?

Agree!

I'm not convinced by the use of Rank to decide what order to merge nodes in. Merge should be commutative. Why was this needed again?

Without the Rank change, merge of Node.Metadata is not commutative, which specifically results in the wrong value for Node.Metadata["topology"] (among others, I'm sure). We could possibly pull Topology and ID into fields, giving them better merge semantics.

I think we should promote the various 'parent' foreign keys out of Node.Metadata into a Node.Parent field, of type []struct{topology, nodeid string}, with set-like merge semantics. Doing this will allow us to efficiently gather all the parents recursively, and potentially simplify all the foreign key joins in the rendering code, and might even solve the commutative merge problem.

A better structure for parents sounds nice. Unfortunately, gathering parents recursively is no good. For a container, it would grab all the parents of the container image which will include all hosts running that image.

please make changes to tools/test in tools.git

ack

paulbellamy · 2016-01-06T13:43:19Z

f2f discussion with @tomwilkie brought up that we should maybe try to set parents on the Node, not on RenderableNode, then aggregate them when merging. Doing it on the Probe would be nice, but not possible in all cases (k8s).

Also, that possibly Node.Metadata[probe.Topology] is not really used, so may be possible to swap it into a field, and add a "up-levelling" function in the MapX2Y funcs where we explicitly change it and the ID.

tomwilkie · 2016-01-08T21:00:41Z

Some more feedback (its looking awesome, but...)

~~clicking on a node in the detail panel thats already open in the stack of details panels doesn't work or do anything (david)~~ works, pops the stack
we used to see the pid of the selected process in the details panel in the title of the table. (david + paul)
containers should have their images as a parent / tag (paul)
container state is missing, as is IP (paul)
container psuedo nodes are showing jibberish in the details panel (looks like process details panel) (paul)
containers-by-hostname nodes should list the containers grouped under that (paul)
container image details seem broken - "Uncaught TypeError: Cannot read property '_currentElement' of null" (david)
Theresmore, but it'll have to wait

tomwilkie · 2016-01-11T13:14:18Z

More feedback:

for nodes in the applications-by-name view, applications in the table can't be clicked. (they are marked as not linkable, paul?)
~~it would be nice if the docker labels fields were a table for container nodes (for a future PR, Docker Labels should be a separate table in the details panel for containers #802)~~
nodes in containers-by-hostname have a container as a parent (should be a child). Also need to ensure they can have multiple hosts and images as parents. (paul)
I'd like to see a container images table for nodes in the hosts view, with a column for #containers. (future PR. Container images in the host view should have a column of the number of containers #821)

This one might be hard:

when looking at a host node, clicking on a container that is hidden by default (say, weavescope) takes you to the containers view but doesn't show weavescope (as its hidden). We should make this work... all we came up with was inferior, so we decided to live with it (david)

Going to look at code now.

probe/topology_tagger.go

-		for id, node := range topology.Nodes {
-			topology.AddNode(id, node.WithMetadata(metadata))
+		for id, node := range t.Nodes {
+			t.AddNode(id, node.WithID(id).WithTopology(name))


davkal · 2016-01-11T13:38:57Z

we used to see the pid of the selected process in the details panel in the title of the table.

@tomwilkie you can see the parent PID on hover, good enough? Is that the same as "PID" in that context (if so, maybe we drop the "Parent")?

render/detailed/metrics.go

+	return nil
+}
+
+func processNodeMetrics(nmd report.Node) []MetricRow {


paulbellamy · 2016-01-13T17:34:34Z

Last MakeDetailedHostNode failure is stuck on #815

foot · 2016-01-13T17:46:37Z

Wow looks awesome!

Encountered two children with the same key after clicking show more in ~~Host > Containers, and~~ Host > Container Images (screenshot), [paul, using the label for node id is not unique, how about image ID?]
~~Host > Container list has lots of room, doesn't need ellipsising so early. (screenshot),~~ [truncate and dynamic expansion not possible]
Text-size/text-color is just readable. Upping the base font from 13px to 14px is easier on my eyes (though breaks the layout). 13px does look more aesthetic.. 14px "feels easier".
CPU always has its sorting arrow, that mimics the primary sorting arrow (e.g. If you click PID a couple of times, both its sorting arrow and that of CPU Usage changes).
Clicking away from the stack (on the canvas) I expected the entire stack to be dismissed rather than just the top card. (Though I still like esc to pop just the top card...)
~~As the stack gets higher it gets fuzzier (??) (see screenshot).~~ I was at 110% zoom, still strange though!
Sometimes I can click on an app (it has a link), other times I cannot, though maybe they are different apps with the same name (?). Makes the UI feel a bit broken sometimes though =/. What controls whether you can click on an app? [a flag called linkable controls it, maybe we add a sentence to the hover, paul what do you think?]
Edge case: If you are being silly and make a stack of 8 cards, it starts DoSing the server, the top card sometimes has to wait for other requests to complete before loading, making it feel a smidge laggy. Perhaps only poll top card.

Stuff thats already been mentioned:

Host > Container > Host doesn't work (Not sure what cycle behaviour should be though, summon card to top?) (Tom).
Terminal/attach not working (Tom)
I get container-image <none>:<none> appearing sometimes? (see screenshot) [I reckon those are untagged containers]

Screenshot:

paulbellamy · 2016-01-14T09:51:28Z

Also PID sorting seems to be doing it alphabetically, which is a bit strange.

paulbellamy · 2016-01-14T09:53:24Z

Sometimes I can click on an app (it has a link), other times I cannot, though maybe they are different apps with the same name (?). Makes the UI feel a bit broken sometimes though =/. What controls whether you can click on an app?

Whether it is a link or not is determined by the process having connections. Processes without any connections don't show up in the process view, so there's not really anything to link to. We could link them to a process details panel, but hide the link to "go to topology" (or just show the process topology with a details panel for a non-existing node).

paulbellamy · 2016-01-18T15:50:44Z

I think the numbers for host cpu usage are wrong: my cpu is close to 100%, and scope it reporting...

I hypothesise that it may be this bug: #815

We merge all the processes and containers into the host (to get the children), but that brings the metrics with it, and since some of those keys happen to overlap, they all get merged in, and a random number is chosen.

tomwilkie · 2016-01-18T16:25:52Z

Could refactor it to a struct (instead of a fn), so the "type" goes at the end.

Roger, please file a ticket against 0.13 and we'll do that then.

Started on this, but you have to change all the MakeNode and MakeNodeWiths, got about 726 lines in, and stashed it.

Ditto, we can fix in 0.13

I hypothesise that it may be this bug: #815

Okay we need to fix this before the merge - I think dropping the metrics when we create new "Derived" nodes in the rendering will be enough for now. I'll take a look today.

paulbellamy · 2016-01-18T16:40:27Z

Uncontained (in the containers view), is rendering the metadata/metrics from one of the processes, not just a list of processes.

Megasquish: [app] remove unused edge endpoint [WIP] refactoring node details api endpoint [WIP] plumbing the children through the rendering process adding IDList.Remove and StringSet.Remove [WIP] working on adding parents to detailed node renderings WIP UI components with mock backend data for new details grouping children by type UI components for node details health and info metric formatters for details panel Column headers and links for details table [WIP] started on rendering node metadata and metrics in the detail view DetailedNode.LabelMajor -> DetailedNode.Label rendering decent labels for parents of detailed nodes render metrics onto the top-level detailed node removing dead code Links to relatives metrics have a Format not Unit Show more/less actions for tables and relatives adjusted metric formatter TopologyTagger should tag k8s topology nodes make renderablenode ids more consistent, e.g. container:abcd1234 working on rendering correct summaries for each node adding report.Node.Rank, so that merging is independent of order rendering children and parents correctly output child renderableNode ids, so we can link to them add group field to metrics, so they can be grouped Refactored details health items to prepare for grouping add metrics to processNodeSummaries hide summary section if there is no data for it fixing up tests moving detailed node rendering into a separate package Node ID/Topology are fields not metadata - This way I think we don't have to care about Metadata being non-commutative. - ID and topology are still non-commutative, as I'm not sure how to sanely merge them, but it's possible we don't care. host memory usage is a filesize, not a percent working on fixing some tests adding children to hosts detail panel - Had to redo how parents are calculated, so that children wouldn't interfere with it - have to have the host at the end because it is non-commutative only render links for linkable children (i.e. not unconnected processes) resolving TODOs fixing up lint errors make nil a valid value for render.Children so tests are cleaner working on backend tests make client handle missing metrics property Stop rendering container image nodes with process summaries/parents fix parent link to container images Calculate parents as a set on report.Node (except k8s) refactoring detailed.NodeSummary stuff removing RenderableNode.Summary*, we already track it on report.Node working on tests add Columns field to NodeSummaryGroup fixing up render/topologies_test fix children links to container images get children of hosts rendering right working on host renderer tests Change container report.Node.ID to a1b2c3;<container> The id should be globally unique, so we don't need the host id. This lets the kubernetes probe return a container node with the pod id, which will get merged into the real containers with other reports. The catch is that the kubernetes api doesn't tell us which hostname the container is running on, so we can't populate the old-style node ids. change terminology of system pods and services Fix kubernetes services with no selector Fixes handling of kubernetes service, which has no pods fix parent links for pods/services refactor detailed metadata to include sets and latest data fixing up host rendering tests fleshing out tests for node metadata and metrics don't render container pseudo-nodes as processes Update test for id format change.

Refactored nodedetails to support multiple data sets, probably broke some tests Allow api requests to out-of-view topologies Fix ESC behavior with details panel Stack details panel like cards Details pain side-by-side Details panel piles Fix node details table header styles Render load and not-found captions like relatives Fix topology click action Make node detail tables sortable Grouped metrics for details health Group metrics in same style Link node details children Fix scroll issues on double-details Fix DESC sort order for node details table Save selected node labels in state - allows rendering of node labels from other topologies before details are loaded Change detail card UX, newest one at top, pile at bottom Details panel one pile w/ animation Sort details table nodes by metadata too Animate sidepanel from children too Fix radial layout Dont set origin if a node was already selected, suppresses animation stack effect: shift top cards to the left, shrink lower cards vertically Clear details card stack if sibling is selected Check if node is still selected on API response Make detail table sorters robust against non-uniform metadata Dont show scrollbar all the time, fix sort icon issue Button to show topology for relative Overflow metrics for details panel health Fix JS error when no metrics are available for container image details Column-based rendering of node details table Fix JS tests Review feedback (UI)

They stopped working because of the change to container node IDs (and rendered nodes IDs for containers). The UI was comparing the IDs, which was never safe. I have just removed that code. This does leave the possibility of us having the pipe control operation take a long time, the user navigate to a different node, and then the terminal pop up, but I think thats better that teaching the UI to understand the format of the node IDs.

- Treat control objects that come back from the server as little black boxes. - Pass our local client nodeId around more instead, use that for comparisons etc, (vs. inspecting the control object and doing brittle magic w/ the ids).

paulbellamy · 2016-01-19T16:57:26Z

We've LGTMed and agreed in chat to merge this, then continue working, as rebasing is becoming painful.

Details panel redesign

paulbellamy force-pushed the 301-details-pain branch 4 times, most recently from ab97e1f to 5e26db9 Compare December 17, 2015 13:11

davkal mentioned this pull request Jan 6, 2016

Animate sparklines #795

Merged

paulbellamy force-pushed the 301-details-pain branch from 239a83d to 77e6dd9 Compare January 8, 2016 14:31

paulbellamy mentioned this pull request Jan 11, 2016

Docker Labels should be a separate table in the details panel for containers #802

Closed

tomwilkie reviewed Jan 11, 2016
View reviewed changes

probe/topology_tagger.go

for id, node := range topology.Nodes {

topology.AddNode(id, node.WithMetadata(metadata))

for id, node := range t.Nodes {

t.AddNode(id, node.WithID(id).WithTopology(name))

This comment was marked as abuse.

Sign in to view

paulbellamy mentioned this pull request Jan 11, 2016

Add container images as children of hosts in details panel #803

Closed

tomwilkie reviewed Jan 11, 2016
View reviewed changes

render/detailed/metrics.go

return nil

}

func processNodeMetrics(nmd report.Node) []MetricRow {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

paulbellamy force-pushed the 301-details-pain branch 3 times, most recently from 778852c to 7ae4491 Compare January 18, 2016 15:22

This was referenced Jan 18, 2016

Refactor render/detailed/metadata.go #835

Closed

Remove usage of report.Node.WithID and report.Node.WithTopology #836

Closed

foot added this to the 0.12.0 milestone Jan 19, 2016

paulbellamy and others added 10 commits January 19, 2016 16:39

remove hostID from container image node ids

3cda328

use constants for the topology names in Parents/metadata/metrics

e30c9dc

templatize metric rendering and shorten column names

54d0db0

Review feedback refactoring

cb9d558

render detail status section for metadata or metrics

bf13a82

adding a benchmark for the various render topologies

e7c9d4b

Fixes pipes again

0785d53

- Treat control objects that come back from the server as little black boxes. - Pass our local client nodeId around more instead, use that for comparisons etc, (vs. inspecting the control object and doing brittle magic w/ the ids).

foot force-pushed the 301-details-pain branch from af5f2e4 to 27300af Compare January 19, 2016 16:10

paulbellamy force-pushed the 301-details-pain branch from 27300af to e877f3d Compare January 19, 2016 16:29

paulbellamy and others added 2 commits January 19, 2016 16:31

fixing up some performance issues in NodeSet

3d32d10

Reconcile some details-panel stuff after the rebase.

c43abd6

paulbellamy force-pushed the 301-details-pain branch from e877f3d to c43abd6 Compare January 19, 2016 16:31

paulbellamy changed the title ~~[WIP] Details panel redesign~~ Details panel redesign Jan 19, 2016

paulbellamy added a commit that referenced this pull request Jan 19, 2016

Merge pull request #752 from weaveworks/301-details-pain

0afd151

Details panel redesign

paulbellamy merged commit 0afd151 into master Jan 19, 2016

paulbellamy deleted the 301-details-pain branch January 19, 2016 16:57

paulbellamy mentioned this pull request Jan 20, 2016

Use ps.Map for Counters and Sets, remove Metadata in favour of Latest. #838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Details panel redesign #752

Details panel redesign #752

davkal commented Dec 11, 2015

tomwilkie commented Jan 4, 2016

tomwilkie commented Jan 5, 2016

paulbellamy commented Jan 6, 2016

paulbellamy commented Jan 6, 2016

tomwilkie commented Jan 8, 2016

tomwilkie commented Jan 11, 2016

This comment was marked as abuse.

davkal commented Jan 11, 2016

This comment was marked as abuse.

This comment was marked as abuse.

paulbellamy commented Jan 13, 2016

foot commented Jan 13, 2016

paulbellamy commented Jan 14, 2016

paulbellamy commented Jan 14, 2016

paulbellamy commented Jan 18, 2016

tomwilkie commented Jan 18, 2016

paulbellamy commented Jan 18, 2016

paulbellamy commented Jan 19, 2016

Details panel redesign #752

Details panel redesign #752

Conversation

davkal commented Dec 11, 2015

tomwilkie commented Jan 4, 2016

tomwilkie commented Jan 5, 2016

paulbellamy commented Jan 6, 2016

paulbellamy commented Jan 6, 2016

tomwilkie commented Jan 8, 2016

tomwilkie commented Jan 11, 2016

This comment was marked as abuse.

davkal commented Jan 11, 2016

This comment was marked as abuse.

This comment was marked as abuse.

paulbellamy commented Jan 13, 2016

foot commented Jan 13, 2016

paulbellamy commented Jan 14, 2016

paulbellamy commented Jan 14, 2016

paulbellamy commented Jan 18, 2016

tomwilkie commented Jan 18, 2016

paulbellamy commented Jan 18, 2016

paulbellamy commented Jan 19, 2016