render sensible labels for nodes with little/no metadata #2998
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rade
force-pushed
the
node-label-fallbacks
branch
10 times, most recently
from
17cea2e to
dfd2daa
Compare
rade
changed the title
rade
force-pushed
the
node-label-fallbacks
branch
2 times, most recently
from
18f86db to
bdf280e
Compare
rade
force-pushed
the
node-label-fallbacks
branch
4 times, most recently
from
1611af2 to
e7b8d35
Compare
unused for now
unused for now A convenient place to document and deal with the ugliness.
This doesn't make any difference to the outcome - we were simply setting the shape in the NodeSummary to "", which is what it starts out as - but looks less weird in the code.
rade
force-pushed
the
node-label-fallbacks
branch
2 times, most recently
from
15155d0 to
418a1a8
Compare
bboreham
approved these changes
Dec 27, 2017
| base.Shape = report.Circle | ||
| } else { | ||
| // last resort | ||
| base.Label = pseudoID |
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.
render/detailed/summary.go
Outdated
| imageName, _ = n.Latest.Lookup(docker.ImageName) | ||
| imageNameWithoutVersion = docker.ImageNameWithoutVersion(imageName) | ||
| ) | ||
| switch true { |
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.
| // MakeNodeID(ns[row.remoteNodeID]). As we don't need the whole summary. | ||
| summary, _ := MakeNodeSummary(report.RenderContext{Report: r}, ns[row.remoteNodeID]) | ||
| // Use MakeBasicNodeSummary to render the id and label of this node | ||
| summary, _ := MakeBasicNodeSummary(r, ns[row.remoteNodeID]) |
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.
We cope with the absence of the process name and/or container name, and extract the hostID and pid from the node id rather than metadata since that way we are guaranteed to get values for them.
We fall back to the truncated container id when we cannot find a name. NB: this also happens when rendering a container as a parent. We cope with the absence of an image name and/or host name.
The main change here is to to label the node with its pseudoID as the last resort. We also set the rank to the pseudoID instead of the full node id, since including the "pseudo:" prefix is not conducive to good ranking. The rest is just moving from 'if' to 'switch'.
We fall back to image id when we cannot find a name. We extract the image id from the node id rather than the docker.ImageID latest map entry since that way we are guaranteed to get it.
The node id, which we always have, actually contains the hostname.
We fall back to using the object id as the label.
We fall back to using the ARN as the label.
We fall back to using the service ID as the label.
We fall back to using the peerName as the label, which we always have.
The node ID of group nodes is in fact the same value as we get from looking up the metadata key contained in the group topology id. So just use that since a) we always have it, and b) we save a LatestMap lookup.
Now that summary renderers always produce something, they no longer need to indicate whether they did.
Fixes an omission.
This is sufficient for rendering links to nodes, and is cheaper to compute that the full NodeSummary.
instead of the full NodeSummary
We eliminate the custom parent renderer, which was a very partial implementation MakeBasicNodeSummary. We also ensure that parents are always rendered in the same, sensible order. Previously the order was an alphabetic sort of the parent topology IDs. Now lower level topologies come before higher level topologies.
The id and topology are enough to get some basic info for rendering. This isn't just a fall-through for exceptional cases. ContainerwithImageNameRenderer produces parent references using the image name without version as the ID, and containers-by-image topology uses that for grouping. By contrast, the container-image topology in the report is keyed on docker image IDs.
rade
force-pushed
the
node-label-fallbacks
branch
from
418a1a8 to
0b4512b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A report may contain references to nodes which are not contained in the report. For example a short-lived process may have initiated some connections, which Scope detected, but died before Scope got round to obtaining full details for inclusion in the process topology.
More generally, we may be missing some or all metadata for a node - having just a reference (which is the node ID) is the most extreme case.
We encounter this missing info when summarising nodes for rendering in the UI (i.e. determining their shape, labels, etc), and when rendering links to them as parents, children and connection sources/destinations in the detail panel.
This PR ensures that we can summarise nodes in all these cases, ensuring that we can cope with having nothing more than the ID and topology.
Note that there are some cases where our renderers throw away nodes that miss some metadata, when really they should be passing them through. Fixing that entails small changes scattered across a bunch of files, which I'd rather defer to a separate PR.