Merge pull request #2998 from weaveworks/node-label-fallbacks

render sensible labels for nodes with little/no metadata

render/detailed/connections.go

@@ -125,10 +125,8 @@ func internetAddr(node report.Node, ep report.Node) (string, bool) {
 func (c *connectionCounters) rows(r report.Report, ns report.Nodes, includeLocal bool) []Connection {
 	output := []Connection{}
 	for row, count := range c.counts {
-		// Use MakeNodeSummary to render the id and label of this node
-		// TODO(paulbellamy): Would be cleaner if we hade just a
-		// 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])
 		connection := Connection{
 			ID:         fmt.Sprintf("%s-%s-%s-%s", row.remoteNodeID, row.remoteAddr, row.localAddr, row.port),
 			NodeID:     summary.ID,

render/detailed/links_test.go

@@ -31,24 +31,33 @@ var (
 	}
 )
 
+func nodeSummaryWithMetrics(label string, metrics []report.MetricRow) detailed.NodeSummary {
+	return detailed.NodeSummary{
+		BasicNodeSummary: detailed.BasicNodeSummary{
+			Label: label,
+		},
+		Metrics: metrics,
+	}
+}
+
 func TestRenderMetricURLs_Disabled(t *testing.T) {
-	s := detailed.NodeSummary{Label: "foo", Metrics: sampleMetrics}
+	s := nodeSummaryWithMetrics("foo", sampleMetrics)
 	result := detailed.RenderMetricURLs(s, samplePodNode, "")
 
 	assert.Empty(t, result.Metrics[0].URL)
 	assert.Empty(t, result.Metrics[1].URL)
 }
 
 func TestRenderMetricURLs_UnknownTopology(t *testing.T) {
-	s := detailed.NodeSummary{Label: "foo", Metrics: sampleMetrics}
+	s := nodeSummaryWithMetrics("foo", sampleMetrics)
 	result := detailed.RenderMetricURLs(s, sampleUnknownNode, sampleMetricsGraphURL)
 
 	assert.Empty(t, result.Metrics[0].URL)
 	assert.Empty(t, result.Metrics[1].URL)
 }
 
 func TestRenderMetricURLs_Pod(t *testing.T) {
-	s := detailed.NodeSummary{Label: "foo", Metrics: sampleMetrics}
+	s := nodeSummaryWithMetrics("foo", sampleMetrics)
 	result := detailed.RenderMetricURLs(s, samplePodNode, sampleMetricsGraphURL)
 
 	checkURL(t, result.Metrics[0].URL, sampleMetricsGraphURL,
@@ -58,7 +67,7 @@ func TestRenderMetricURLs_Pod(t *testing.T) {
 }
 
 func TestRenderMetricURLs_Container(t *testing.T) {
-	s := detailed.NodeSummary{Label: "foo", Metrics: sampleMetrics}
+	s := nodeSummaryWithMetrics("foo", sampleMetrics)
 	result := detailed.RenderMetricURLs(s, sampleContainerNode, sampleMetricsGraphURL)
 
 	checkURL(t, result.Metrics[0].URL, sampleMetricsGraphURL,
@@ -84,10 +93,7 @@ func TestRenderMetricURLs_EmptyMetrics(t *testing.T) {
 }
 
 func TestRenderMetricURLs_CombinedEmptyMetrics(t *testing.T) {
-	s := detailed.NodeSummary{
-		Label:   "foo",
-		Metrics: []report.MetricRow{{ID: docker.MemoryUsage, Priority: 1}},
-	}
+	s := nodeSummaryWithMetrics("foo", []report.MetricRow{{ID: docker.MemoryUsage, Priority: 1}})
 	result := detailed.RenderMetricURLs(s, samplePodNode, sampleMetricsGraphURL)
 
 	assert.NotEmpty(t, result.Metrics[0].URL)
@@ -99,7 +105,7 @@ func TestRenderMetricURLs_CombinedEmptyMetrics(t *testing.T) {
 }
 
 func TestRenderMetricURLs_QueryReplacement(t *testing.T) {
-	s := detailed.NodeSummary{Label: "foo", Metrics: sampleMetrics}
+	s := nodeSummaryWithMetrics("foo", sampleMetrics)
 	result := detailed.RenderMetricURLs(s, samplePodNode, "http://example.test/?q=:query")
 
 	checkURL(t, result.Metrics[0].URL, "http://example.test/?q=",

render/detailed/node_test.go

@@ -43,14 +43,16 @@ func TestMakeDetailedHostNode(t *testing.T) {
 	podNodeSummary := child(t, render.PodRenderer, fixture.ClientPodNodeID)
 	want := detailed.Node{
 		NodeSummary: detailed.NodeSummary{
-			ID:         fixture.ClientHostNodeID,
-			Label:      "client",
-			LabelMinor: "hostname.com",
-			Rank:       "hostname.com",
-			Pseudo:     false,
-			Shape:      "circle",
-			Linkable:   true,
-			Adjacency:  report.MakeIDList(fixture.ServerHostNodeID),
+			BasicNodeSummary: detailed.BasicNodeSummary{
+				ID:         fixture.ClientHostNodeID,
+				Label:      "client",
+				LabelMinor: "hostname.com",
+				Rank:       "hostname.com",
+				Pseudo:     false,
+				Shape:      "circle",
+				Linkable:   true,
+			},
+			Adjacency: report.MakeIDList(fixture.ServerHostNodeID),
 			Metadata: []report.MetadataRow{
 				{
 					ID:       "host_name",
@@ -189,13 +191,15 @@ func TestMakeDetailedContainerNode(t *testing.T) {
 	serverProcessNodeSummary.Linkable = true
 	want := detailed.Node{
 		NodeSummary: detailed.NodeSummary{
-			ID:         id,
-			Label:      "server",
-			LabelMinor: "server.hostname.com",
-			Rank:       fixture.ServerContainerImageName,
-			Shape:      "hexagon",
-			Linkable:   true,
-			Pseudo:     false,
+			BasicNodeSummary: detailed.BasicNodeSummary{
+				ID:         id,
+				Label:      "server",
+				LabelMinor: "server.hostname.com",
+				Rank:       fixture.ServerContainerImageName,
+				Shape:      "hexagon",
+				Linkable:   true,
+				Pseudo:     false,
+			},
 			Metadata: []report.MetadataRow{
 				{ID: "docker_image_name", Label: "Image", Value: fixture.ServerContainerImageName, Priority: 1},
 				{ID: "docker_container_state_human", Label: "State", Value: "running", Priority: 3},
@@ -225,16 +229,16 @@ func TestMakeDetailedContainerNode(t *testing.T) {
 					Label:      fixture.ServerContainerImageName,
 					TopologyID: "containers-by-image",
 				},
-				{
-					ID:         fixture.ServerHostNodeID,
-					Label:      fixture.ServerHostName,
-					TopologyID: "hosts",
-				},
 				{
 					ID:         fixture.ServerPodNodeID,
 					Label:      "pong-b",
 					TopologyID: "pods",
 				},
+				{
+					ID:         fixture.ServerHostNodeID,
+					Label:      "server",
+					TopologyID: "hosts",
+				},
 			},
 		},
 		Controls: []detailed.ControlInstance{},
@@ -320,29 +324,31 @@ func TestMakeDetailedPodNode(t *testing.T) {
 	serverProcessNodeSummary.Linkable = true // Temporary workaround for: https://github.com/weaveworks/scope/issues/1295
 	want := detailed.Node{
 		NodeSummary: detailed.NodeSummary{
-			ID:         id,
-			Label:      "pong-b",
-			LabelMinor: "1 container",
-			Rank:       "ping/pong-b",
-			Shape:      "heptagon",
-			Linkable:   true,
-			Pseudo:     false,
+			BasicNodeSummary: detailed.BasicNodeSummary{
+				ID:         id,
+				Label:      "pong-b",
+				LabelMinor: "1 container",
+				Rank:       "ping/pong-b",
+				Shape:      "heptagon",
+				Linkable:   true,
+				Pseudo:     false,
+			},
 			Metadata: []report.MetadataRow{
 				{ID: "kubernetes_state", Label: "State", Value: "running", Priority: 2},
 				{ID: "container", Label: "# Containers", Value: "1", Priority: 4, Datatype: report.Number},
 				{ID: "kubernetes_namespace", Label: "Namespace", Value: "ping", Priority: 5},
 			},
 			Parents: []detailed.Parent{
-				{
-					ID:         fixture.ServerHostNodeID,
-					Label:      fixture.ServerHostName,
-					TopologyID: "hosts",
-				},
 				{
 					ID:         fixture.ServiceNodeID,
 					Label:      fixture.ServiceName,
 					TopologyID: "services",
 				},
+				{
+					ID:         fixture.ServerHostNodeID,
+					Label:      "server",
+					TopologyID: "hosts",
+				},
 			},
 		},
 		Controls: []detailed.ControlInstance{},

render/detailed/parents.go

@@ -1,12 +1,6 @@
 package detailed
 
 import (
-	"sort"
-
-	"github.com/weaveworks/scope/probe/awsecs"
-	"github.com/weaveworks/scope/probe/docker"
-	"github.com/weaveworks/scope/probe/host"
-	"github.com/weaveworks/scope/probe/kubernetes"
 	"github.com/weaveworks/scope/report"
 )
 
@@ -17,24 +11,21 @@ type Parent struct {
 	TopologyID string `json:"topologyId"`
 }
 
-var (
-	kubernetesParentLabel = latestLookup(kubernetes.Name)
-
-	getLabelForTopology = map[string]func(report.Node) string{
-		report.Container:      getRenderableContainerName,
-		report.Pod:            kubernetesParentLabel,
-		report.Deployment:     kubernetesParentLabel,
-		report.DaemonSet:      kubernetesParentLabel,
-		report.StatefulSet:    kubernetesParentLabel,
-		report.CronJob:        kubernetesParentLabel,
-		report.Service:        kubernetesParentLabel,
-		report.ECSTask:        latestLookup(awsecs.TaskFamily),
-		report.ECSService:     ecsServiceParentLabel,
-		report.SwarmService:   latestLookup(docker.ServiceName),
-		report.ContainerImage: containerImageParentLabel,
-		report.Host:           latestLookup(host.HostName),
-	}
-)
+// parent topologies, in the order we want to show them
+var parentTopologies = []string{
+	report.Container,
+	report.ContainerImage,
+	report.Pod,
+	report.Deployment,
+	report.DaemonSet,
+	report.StatefulSet,
+	report.CronJob,
+	report.Service,
+	report.ECSTask,
+	report.ECSService,
+	report.SwarmService,
+	report.Host,
+}
 
 // Parents renders the parents of this report.Node, which have been aggregated
 // from the probe reports.
@@ -43,66 +34,35 @@ func Parents(r report.Report, n report.Node) []Parent {
 		return nil
 	}
 	result := make([]Parent, 0, n.Parents.Size())
-	topologyIDs := []string{}
-	for topologyID := range getLabelForTopology {
-		topologyIDs = append(topologyIDs, topologyID)
-	}
-	sort.Strings(topologyIDs)
-	for _, topologyID := range topologyIDs {
-		getLabel := getLabelForTopology[topologyID]
+	for _, topologyID := range parentTopologies {
 		topology, ok := r.Topology(topologyID)
 		if !ok {
 			continue
 		}
+		apiTopologyID, ok := primaryAPITopology[topologyID]
+		if !ok {
+			continue
+		}
 		parents, _ := n.Parents.Lookup(topologyID)
 		for _, id := range parents {
 			if topologyID == n.Topology && id == n.ID {
 				continue
 			}
-
-			var parentNode report.Node
-			// Special case: container image parents should be empty nodes for some reason
-			if topologyID == report.ContainerImage {
-				parentNode = report.MakeNode(id)
-			} else {
-				if parent, ok := topology.Nodes[id]; ok {
-					parentNode = parent
-				} else {
-					continue
-				}
-			}
-
-			apiTopologyID, ok := primaryAPITopology[topologyID]
+			parentNode, ok := topology.Nodes[id]
 			if !ok {
-				continue
+				parentNode = report.MakeNode(id).WithTopology(topologyID)
+			}
+			if summary, ok := MakeBasicNodeSummary(r, parentNode); ok {
+				result = append(result, Parent{
+					ID:         summary.ID,
+					Label:      summary.Label,
+					TopologyID: apiTopologyID,
+				})
 			}
-
-			result = append(result, Parent{
-				ID:         id,
-				Label:      getLabel(parentNode),
-				TopologyID: apiTopologyID,
-			})
 		}
 	}
 	if len(result) == 0 {
 		return nil
 	}
 	return result
 }
-
-func latestLookup(key string) func(report.Node) string {
-	return func(n report.Node) string {
-		value, _ := n.Latest.Lookup(key)
-		return value
-	}
-}
-
-func ecsServiceParentLabel(n report.Node) string {
-	_, name, _ := report.ParseECSServiceNodeID(n.ID)
-	return name
-}
-
-func containerImageParentLabel(n report.Node) string {
-	name, _ := report.ParseContainerImageNodeID(n.ID)
-	return name
-}

render/detailed/parents_test.go

@@ -34,23 +34,23 @@ func TestParents(t *testing.T) {
 			name: "Container image",
 			node: render.ContainerImageRenderer.Render(fixture.Report).Nodes[expected.ClientContainerImageNodeID],
 			want: []detailed.Parent{
-				{ID: fixture.ClientHostNodeID, Label: fixture.ClientHostName, TopologyID: "hosts"},
+				{ID: fixture.ClientHostNodeID, Label: "client", TopologyID: "hosts"},
 			},
 		},
 		{
 			name: "Container",
 			node: render.ContainerWithImageNameRenderer.Render(fixture.Report).Nodes[fixture.ClientContainerNodeID],
 			want: []detailed.Parent{
 				{ID: expected.ClientContainerImageNodeID, Label: fixture.ClientContainerImageName, TopologyID: "containers-by-image"},
-				{ID: fixture.ClientHostNodeID, Label: fixture.ClientHostName, TopologyID: "hosts"},
 				{ID: fixture.ClientPodNodeID, Label: "pong-a", TopologyID: "pods"},
+				{ID: fixture.ClientHostNodeID, Label: "client", TopologyID: "hosts"},
 			},
 		},
 		{
 			node: render.ProcessRenderer.Render(fixture.Report).Nodes[fixture.ClientProcess1NodeID],
 			want: []detailed.Parent{
 				{ID: fixture.ClientContainerNodeID, Label: fixture.ClientContainerName, TopologyID: "containers"},
-				{ID: fixture.ClientHostNodeID, Label: fixture.ClientHostName, TopologyID: "hosts"},
+				{ID: fixture.ClientHostNodeID, Label: "client", TopologyID: "hosts"},
 			},
 		},
 	} {