Fixing filtered counts on containers topology

Tricky, because we need the filters to be silent sometimes (when they're
in the middle), but not when they're at the top, so we take the "top"
filter's stats. However, this means we have to compose all
user-specified filters into a single Filter layer, so we can get all
stats.

There are no more Silent filters, as all filters are silent (unless they
are at the top).

Additionally, I clarified some of the filters as their usage/terminology
was inconsistent and confused. Now Filter(IsFoo, ...) *keeps* only nodes
where IsFoo is true.

app/api_topologies.go

@@ -26,9 +26,9 @@ func init() {
 			ID:      "system",
 			Default: "application",
 			Options: []APITopologyOption{
-				{"system", "System services", render.FilterApplication},
-				{"application", "Application services", render.FilterSystem},
-				{"both", "Both", render.FilterNoop},
+				{"system", "System services", render.IsApplication},
+				{"application", "Application services", render.IsSystem},
+				{"both", "Both", render.Noop},
 			},
 		},
 	}
@@ -38,9 +38,9 @@ func init() {
 			ID:      "system",
 			Default: "application",
 			Options: []APITopologyOption{
-				{"system", "System pods", render.FilterApplication},
-				{"application", "Application pods", render.FilterSystem},
-				{"both", "Both", render.FilterNoop},
+				{"system", "System pods", render.IsApplication},
+				{"application", "Application pods", render.IsSystem},
+				{"both", "Both", render.Noop},
 			},
 		},
 	}
@@ -50,18 +50,18 @@ func init() {
 			ID:      "system",
 			Default: "application",
 			Options: []APITopologyOption{
-				{"system", "System containers", render.FilterApplication},
-				{"application", "Application containers", render.FilterSystem},
-				{"both", "Both", render.FilterNoop},
+				{"system", "System containers", render.IsSystem},
+				{"application", "Application containers", render.IsApplication},
+				{"both", "Both", render.Noop},
 			},
 		},
 		{
 			ID:      "stopped",
 			Default: "running",
 			Options: []APITopologyOption{
-				{"stopped", "Stopped containers", render.FilterRunning},
-				{"running", "Running containers", render.FilterStopped},
-				{"both", "Both", render.FilterNoop},
+				{"stopped", "Stopped containers", render.IsStopped},
+				{"running", "Running containers", render.IsRunning},
+				{"both", "Both", render.Noop},
 			},
 		},
 	}
@@ -73,7 +73,7 @@ func init() {
 			Options: []APITopologyOption{
 				// Show the user why there are filtered nodes in this view.
 				// Don't give them the option to show those nodes.
-				{"hide", "Unconnected nodes hidden", render.FilterNoop},
+				{"hide", "Unconnected nodes hidden", render.Noop},
 			},
 		},
 	}
@@ -181,7 +181,7 @@ type APITopologyOption struct {
 	Value string `json:"value"`
 	Label string `json:"label"`
 
-	decorator render.Decorator
+	filter render.FilterFunc
 }
 
 type topologyStats struct {
@@ -281,16 +281,18 @@ func decorateWithStats(rpt report.Report, renderer render.Renderer, decorator re
 }
 
 func renderedForRequest(r *http.Request, topology APITopologyDesc) (render.Renderer, render.Decorator) {
-	var filters []render.Decorator
+	var filters []render.FilterFunc
 	for _, group := range topology.Options {
 		value := r.FormValue(group.ID)
 		for _, opt := range group.Options {
 			if (value == "" && group.Default == opt.Value) || (opt.Value != "" && opt.Value == value) {
-				filters = append(filters, opt.decorator)
+				filters = append(filters, opt.filter)
 			}
 		}
 	}
-	return topology.renderer, render.ComposeDecorators(filters...)
+	return topology.renderer, func(renderer render.Renderer) render.Renderer {
+		return render.MakeFilter(render.ComposeFilterFuncs(filters...), renderer)
+	}
 }
 
 type reportRenderHandler func(

render/container.go

@@ -26,14 +26,14 @@ const (
 // NB We only want processes in container _or_ processes with network connections
 // but we need to be careful to ensure we only include each edge once, by only
 // including the ProcessRenderer once.
-var ContainerRenderer = MakeSilentFilter(
+var ContainerRenderer = MakeFilter(
 	func(n report.Node) bool {
 		// Drop deleted containers
 		state, ok := n.Latest.Lookup(docker.ContainerState)
 		return !ok || state != docker.StateDeleted
 	},
 	MakeReduce(
-		MakeSilentFilter(
+		MakeFilter(
 			func(n report.Node) bool {
 				// Drop unconnected pseudo nodes (could appear due to filtering)
 				_, isConnected := n.Latest.Lookup(IsConnected)
@@ -49,7 +49,7 @@ var ContainerRenderer = MakeSilentFilter(
 		// We need to be careful to ensure we only include each edge once.  Edges brought in
 		// by the above renders will have a pid, so its enough to filter out any nodes with
 		// pids.
-		SilentFilterUnconnected(MakeMap(
+		FilterUnconnected(MakeMap(
 			MapIP2Container,
 			MakeReduce(
 				MakeMap(

render/container_test.go

@@ -61,7 +61,7 @@ func TestContainerFilterRenderer(t *testing.T) {
 	input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{
 		docker.LabelPrefix + "works.weave.role": "system",
 	})
-	have := Prune(render.ContainerWithImageNameRenderer.Render(input, render.FilterSystem))
+	have := Prune(render.ContainerWithImageNameRenderer.Render(input, render.FilterApplication))
 	want := Prune(expected.RenderedContainers.Copy())
 	delete(want, fixture.ClientContainerNodeID)
 	if !reflect.DeepEqual(want, have) {
@@ -115,7 +115,7 @@ func TestContainerHostnameFilterRenderer(t *testing.T) {
 			Add("host", report.MakeStringSet(fixture.ClientHostNodeID)),
 		).WithTopology(report.Container))
 
-	have := Prune(render.ContainerHostnameRenderer.Render(input, render.FilterSystem))
+	have := Prune(render.ContainerHostnameRenderer.Render(input, render.FilterApplication))
 	want := Prune(expected.RenderedContainerHostnames)
 	// Test works by virtue of the RenderedContainerHostname only having a container
 	// counter == 1
@@ -151,7 +151,7 @@ func TestContainerImageFilterRenderer(t *testing.T) {
 			Add("host", report.MakeStringSet(fixture.ClientHostNodeID)),
 		).WithTopology(report.ContainerImage))
 
-	have := Prune(render.ContainerImageRenderer.Render(input, render.FilterSystem))
+	have := Prune(render.ContainerImageRenderer.Render(input, render.FilterApplication))
 	want := Prune(expected.RenderedContainerImages.Copy())
 	// Test works by virtue of the RenderedContainerImage only having a container
 	// counter == 1

render/filters.go

@@ -55,27 +55,32 @@ func ColorConnected(r Renderer) Renderer {
 	}
 }
 
+// FilterFunc is the function type used by Filters
+type FilterFunc func(report.Node) bool
+
+// ComposeFilterFuncs composes filterfuncs into a single FilterFunc checking all.
+func ComposeFilterFuncs(fs ...FilterFunc) FilterFunc {
+	return func(n report.Node) bool {
+		for _, f := range fs {
+			if !f(n) {
+				return false
+			}
+		}
+		return true
+	}
+}
+
 // Filter removes nodes from a view based on a predicate.
 type Filter struct {
 	Renderer
-	FilterFunc func(report.Node) bool
-	Silent     bool // true means we don't report stats for how many are filtered
+	FilterFunc FilterFunc
 }
 
 // MakeFilter makes a new Filter.
-func MakeFilter(f func(report.Node) bool, r Renderer) Renderer {
-	return Memoise(&Filter{
-		Renderer:   r,
-		FilterFunc: f,
-	})
-}
-
-// MakeSilentFilter makes a new Filter which does not report how many nodes it filters in Stats.
-func MakeSilentFilter(f func(report.Node) bool, r Renderer) Renderer {
+func MakeFilter(f FilterFunc, r Renderer) Renderer {
 	return Memoise(&Filter{
 		Renderer:   r,
 		FilterFunc: f,
-		Silent:     true,
 	})
 }
 
@@ -126,14 +131,13 @@ func (f *Filter) render(rpt report.Report, dct Decorator) (report.Nodes, int) {
 	return output, filtered
 }
 
-// Stats implements Renderer
+// Stats implements Renderer. General logic is to take the first (i.e.
+// highest-level) stats we find, so upstream stats are ignored. This means that
+// if we want to count the stats from multiple filters we need to compose their
+// FilterFuncs, into a single Filter.
 func (f Filter) Stats(rpt report.Report, dct Decorator) Stats {
-	var upstream = f.Renderer.Stats(rpt, dct)
-	if !f.Silent {
-		_, filtered := f.render(rpt, dct)
-		upstream.FilteredNodes += filtered
-	}
-	return upstream
+	_, filtered := f.render(rpt, dct)
+	return Stats{FilteredNodes: filtered}
 }
 
 // IsConnected is the key added to Node.Metadata by ColorConnected
@@ -142,7 +146,7 @@ const IsConnected = "is_connected"
 
 // Complement takes a FilterFunc f and returns a FilterFunc that has the same
 // effects, if any, and returns the opposite truth value.
-func Complement(f func(report.Node) bool) func(report.Node) bool {
+func Complement(f FilterFunc) FilterFunc {
 	return func(node report.Node) bool { return !f(node) }
 }
 
@@ -169,42 +173,34 @@ func FilterUnconnected(r Renderer) Renderer {
 	)
 }
 
-// SilentFilterUnconnected produces a renderer that filters unconnected nodes
-// from the given renderer; nodes filtered by this are not reported in stats.
-func SilentFilterUnconnected(r Renderer) Renderer {
-	return MakeSilentFilter(
-		func(node report.Node) bool {
-			_, ok := node.Latest.Lookup(IsConnected)
-			return ok
-		},
-		ColorConnected(r),
-	)
-}
+// Noop allows all nodes through
+func Noop(_ report.Node) bool { return true }
 
 // FilterNoop does nothing.
-func FilterNoop(in Renderer) Renderer {
-	return in
-}
-
-// FilterStopped filters out stopped containers.
-func FilterStopped(r Renderer) Renderer {
-	return MakeSilentFilter(IsRunning, r)
-}
+func FilterNoop(r Renderer) Renderer { return r }
 
 // IsRunning checks if the node is a running docker container
 func IsRunning(n report.Node) bool {
 	state, ok := n.Latest.Lookup(docker.ContainerState)
 	return !ok || (state == docker.StateRunning || state == docker.StateRestarting || state == docker.StatePaused)
 }
 
+// IsStopped checks if the node is *not* a running docker container
+var IsStopped = Complement(IsRunning)
+
+// FilterStopped filters out stopped containers.
+func FilterStopped(r Renderer) Renderer {
+	return MakeFilter(IsStopped, r)
+}
+
 // FilterRunning filters out running containers.
 func FilterRunning(r Renderer) Renderer {
-	return MakeSilentFilter(Complement(IsRunning), r)
+	return MakeFilter(IsRunning, r)
 }
 
 // FilterNonProcspied removes endpoints which were not found in procspy.
 func FilterNonProcspied(r Renderer) Renderer {
-	return MakeSilentFilter(
+	return MakeFilter(
 		func(node report.Node) bool {
 			_, ok := node.Latest.Lookup(endpoint.Procspied)
 			return ok
@@ -213,8 +209,8 @@ func FilterNonProcspied(r Renderer) Renderer {
 	)
 }
 
-// IsSystem checks if the node is a "system" node
-func IsSystem(n report.Node) bool {
+// IsApplication checks if the node is an "application" node
+func IsApplication(n report.Node) bool {
 	containerName, _ := n.Latest.Lookup(docker.ContainerName)
 	if _, ok := systemContainerNames[containerName]; ok {
 		return false
@@ -239,14 +235,17 @@ func IsSystem(n report.Node) bool {
 	return true
 }
 
+// IsSystem checks if the node is a "system" node
+var IsSystem = Complement(IsApplication)
+
 // FilterSystem is a Renderer which filters out system nodes.
 func FilterSystem(r Renderer) Renderer {
-	return MakeSilentFilter(IsSystem, r)
+	return MakeFilter(IsSystem, r)
 }
 
 // FilterApplication is a Renderer which filters out application nodes.
 func FilterApplication(r Renderer) Renderer {
-	return MakeSilentFilter(Complement(IsSystem), r)
+	return MakeFilter(IsApplication, r)
 }
 
 // FilterEmpty is a Renderer which filters out nodes which have no children
@@ -257,7 +256,7 @@ func FilterEmpty(topology string, r Renderer) Renderer {
 
 // HasChildren returns true if the node has no children from the specified
 // topology.
-func HasChildren(topology string) func(report.Node) bool {
+func HasChildren(topology string) FilterFunc {
 	return func(n report.Node) bool {
 		if n.Topology == Pseudo {
 			return true

render/pod.go

@@ -17,7 +17,7 @@ const (
 // graph by merging the container graph and the pods topology.
 var PodRenderer = FilterEmpty(report.Container,
 	MakeReduce(
-		MakeSilentFilter(
+		MakeFilter(
 			func(n report.Node) bool {
 				// Drop unconnected pseudo nodes (could appear due to filtering)
 				_, isConnected := n.Latest.Lookup(IsConnected)

render/pod_test.go

@@ -33,7 +33,7 @@ func TestPodFilterRenderer(t *testing.T) {
 	input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{
 		docker.LabelPrefix + "io.kubernetes.pod.name": "kube-system/foo",
 	})
-	have := Prune(render.PodRenderer.Render(input, render.FilterSystem))
+	have := Prune(render.PodRenderer.Render(input, render.FilterApplication))
 	want := Prune(expected.RenderedPods.Copy())
 	delete(want, fixture.ClientPodNodeID)
 	delete(want, fixture.ClientContainerNodeID)
@@ -62,7 +62,7 @@ func TestPodServiceFilterRenderer(t *testing.T) {
 	input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{
 		docker.LabelPrefix + "io.kubernetes.pod.name": "kube-system/foo",
 	})
-	have := Prune(render.PodServiceRenderer.Render(input, render.FilterSystem))
+	have := Prune(render.PodServiceRenderer.Render(input, render.FilterApplication))
 	want := Prune(expected.RenderedPodServices.Copy())
 	wantNode := want[fixture.ServiceNodeID]
 	wantNode.Adjacency = nil