Merge pull request #2947 from weaveworks/simplify-decoration

Decorators, begone!

app/api_topologies.go

@@ -337,9 +337,8 @@ type APITopologyOptionGroup struct {
 	NoneLabel string `json:"noneLabel,omitempty"`
 }
 
-// Get the render filters to use for this option group as a Decorator, if any.
-// If second arg is false, no decorator was needed.
-func (g APITopologyOptionGroup) getFilterDecorator(value string) (render.Decorator, bool) {
+// Get the render filters to use for this option group, if any, or nil otherwise.
+func (g APITopologyOptionGroup) filter(value string) render.FilterFunc {
 	selectType := g.SelectType
 	if selectType == "" {
 		selectType = "one"
@@ -352,7 +351,7 @@ func (g APITopologyOptionGroup) getFilterDecorator(value string) (render.Decorat
 		values = strings.Split(value, ",")
 	default:
 		log.Errorf("Invalid select type %s for option group %s, ignoring option", selectType, g.ID)
-		return nil, false
+		return nil
 	}
 	filters := []render.FilterFunc{}
 	for _, opt := range g.Options {
@@ -375,11 +374,9 @@ func (g APITopologyOptionGroup) getFilterDecorator(value string) (render.Decorat
 		}
 	}
 	if len(filters) == 0 {
-		return nil, false
+		return nil
 	}
-	// Since we've encoded whether to ignore pseudo topologies into each subfilter,
-	// we want no special behaviour for pseudo topologies here, which corresponds to MakePseudo
-	return render.MakeFilterPseudoDecorator(render.AnyFilterFunc(filters...)), true
+	return render.AnyFilterFunc(filters...)
 }
 
 // APITopologyOption describes a &param=value to a given topology.
@@ -488,24 +485,24 @@ func (r *Registry) renderTopologies(rpt report.Report, req *http.Request) []APIT
 	topologies := []APITopologyDesc{}
 	req.ParseForm()
 	r.walk(func(desc APITopologyDesc) {
-		renderer, decorator, _ := r.RendererForTopology(desc.id, req.Form, rpt)
-		desc.Stats = decorateWithStats(rpt, renderer, decorator)
+		renderer, filter, _ := r.RendererForTopology(desc.id, req.Form, rpt)
+		desc.Stats = computeStats(rpt, renderer, filter)
 		for i, sub := range desc.SubTopologies {
-			renderer, decorator, _ := r.RendererForTopology(sub.id, req.Form, rpt)
-			desc.SubTopologies[i].Stats = decorateWithStats(rpt, renderer, decorator)
+			renderer, filter, _ := r.RendererForTopology(sub.id, req.Form, rpt)
+			desc.SubTopologies[i].Stats = computeStats(rpt, renderer, filter)
 		}
 		topologies = append(topologies, desc)
 	})
 	return updateFilters(rpt, topologies)
 }
 
-func decorateWithStats(rpt report.Report, renderer render.Renderer, decorator render.Decorator) topologyStats {
+func computeStats(rpt report.Report, renderer render.Renderer, filter render.FilterFunc) topologyStats {
 	var (
 		nodes     int
 		realNodes int
 		edges     int
 	)
-	r := renderer.Render(rpt, decorator)
+	r := render.Render(rpt, renderer, filter)
 	for _, n := range r.Nodes {
 		nodes++
 		if n.Topology != render.Pseudo {
@@ -522,7 +519,7 @@ func decorateWithStats(rpt report.Report, renderer render.Renderer, decorator re
 }
 
 // RendererForTopology ..
-func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt report.Report) (render.Renderer, render.Decorator, error) {
+func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt report.Report) (render.Renderer, render.FilterFunc, error) {
 	topology, ok := r.get(topologyID)
 	if !ok {
 		return nil, nil, fmt.Errorf("topology not found: %s", topologyID)
@@ -534,18 +531,15 @@ func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt
 		return topology.renderer, nil, nil
 	}
 
-	var decorators []render.Decorator
+	var filters []render.FilterFunc
 	for _, group := range topology.Options {
 		value := values.Get(group.ID)
-		if decorator, ok := group.getFilterDecorator(value); ok {
-			decorators = append(decorators, decorator)
+		if filter := group.filter(value); filter != nil {
+			filters = append(filters, filter)
 		}
 	}
-	if len(decorators) > 0 {
-		// Here we tell the topology renderer to apply the filtering decorator
-		// that we construct as a composition of all the selected filters.
-		composedFilterDecorator := render.ComposeDecorators(decorators...)
-		return render.ApplyDecorator(topology.renderer), composedFilterDecorator, nil
+	if len(filters) > 0 {
+		return topology.renderer, render.ComposeFilterFuncs(filters...), nil
 	}
 	return topology.renderer, nil, nil
 }
@@ -558,7 +552,7 @@ func captureReporter(rep Reporter, f reporterHandler) CtxHandlerFunc {
 	}
 }
 
-type rendererHandler func(context.Context, render.Renderer, render.Decorator, report.RenderContext, http.ResponseWriter, *http.Request)
+type rendererHandler func(context.Context, render.Renderer, render.FilterFunc, report.RenderContext, http.ResponseWriter, *http.Request)
 
 func (r *Registry) captureRenderer(rep Reporter, f rendererHandler) CtxHandlerFunc {
 	return func(ctx context.Context, w http.ResponseWriter, req *http.Request) {
@@ -576,11 +570,11 @@ func (r *Registry) captureRenderer(rep Reporter, f rendererHandler) CtxHandlerFu
 			return
 		}
 		req.ParseForm()
-		renderer, decorator, err := r.RendererForTopology(topologyID, req.Form, rpt)
+		renderer, filter, err := r.RendererForTopology(topologyID, req.Form, rpt)
 		if err != nil {
 			respondWith(w, http.StatusInternalServerError, err)
 			return
 		}
-		f(ctx, renderer, decorator, RenderContextForReporter(rep, rpt), w, req)
+		f(ctx, renderer, filter, RenderContextForReporter(rep, rpt), w, req)
 	}
 }

app/api_topologies_test.go

@@ -109,7 +109,7 @@ func TestRendererForTopologyWithFiltering(t *testing.T) {
 	urlvalues.Set(systemGroupID, customAPITopologyOptionFilterID)
 	urlvalues.Set("stopped", "running")
 	urlvalues.Set("pseudo", "hide")
-	renderer, decorator, err := topologyRegistry.RendererForTopology("containers", urlvalues, fixture.Report)
+	renderer, filter, err := topologyRegistry.RendererForTopology("containers", urlvalues, fixture.Report)
 	if err != nil {
 		t.Fatalf("Topology Registry Report error: %s", err)
 	}
@@ -118,7 +118,7 @@ func TestRendererForTopologyWithFiltering(t *testing.T) {
 	input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{
 		docker.LabelPrefix + "works.weave.role": "system",
 	})
-	have := utils.Prune(renderer.Render(input, decorator).Nodes)
+	have := utils.Prune(render.Render(input, renderer, filter).Nodes)
 	want := utils.Prune(expected.RenderedContainers.Copy())
 	delete(want, fixture.ClientContainerNodeID)
 	delete(want, render.MakePseudoNodeID(render.UncontainedID, fixture.ServerHostID))
@@ -140,7 +140,7 @@ func TestRendererForTopologyNoFiltering(t *testing.T) {
 	urlvalues.Set(systemGroupID, customAPITopologyOptionFilterID)
 	urlvalues.Set("stopped", "running")
 	urlvalues.Set("pseudo", "hide")
-	renderer, decorator, err := topologyRegistry.RendererForTopology("containers", urlvalues, fixture.Report)
+	renderer, filter, err := topologyRegistry.RendererForTopology("containers", urlvalues, fixture.Report)
 	if err != nil {
 		t.Fatalf("Topology Registry Report error: %s", err)
 	}
@@ -149,7 +149,7 @@ func TestRendererForTopologyNoFiltering(t *testing.T) {
 	input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{
 		docker.LabelPrefix + "works.weave.role": "system",
 	})
-	have := utils.Prune(renderer.Render(input, decorator).Nodes)
+	have := utils.Prune(render.Render(input, renderer, filter).Nodes)
 	want := utils.Prune(expected.RenderedContainers.Copy())
 	delete(want, render.MakePseudoNodeID(render.UncontainedID, fixture.ServerHostID))
 	delete(want, render.OutgoingInternetID)
@@ -178,12 +178,12 @@ func getTestContainerLabelFilterTopologySummary(t *testing.T, exclude bool) (det
 	urlvalues.Set(systemGroupID, customAPITopologyOptionFilterID)
 	urlvalues.Set("stopped", "running")
 	urlvalues.Set("pseudo", "hide")
-	renderer, decorator, err := topologyRegistry.RendererForTopology("containers", urlvalues, fixture.Report)
+	renderer, filter, err := topologyRegistry.RendererForTopology("containers", urlvalues, fixture.Report)
 	if err != nil {
 		return nil, err
 	}
 
-	return detailed.Summaries(report.RenderContext{Report: fixture.Report}, renderer.Render(fixture.Report, decorator).Nodes), nil
+	return detailed.Summaries(report.RenderContext{Report: fixture.Report}, render.Render(fixture.Report, renderer, filter).Nodes), nil
 }
 
 func TestAPITopologyAddsKubernetes(t *testing.T) {

app/api_topology.go

@@ -29,27 +29,40 @@ type APINode struct {
 }
 
 // Full topology.
-func handleTopology(ctx context.Context, renderer render.Renderer, decorator render.Decorator, rc report.RenderContext, w http.ResponseWriter, r *http.Request) {
+func handleTopology(ctx context.Context, renderer render.Renderer, filter render.FilterFunc, rc report.RenderContext, w http.ResponseWriter, r *http.Request) {
 	respondWith(w, http.StatusOK, APITopology{
-		Nodes: detailed.Summaries(rc, renderer.Render(rc.Report, decorator).Nodes),
+		Nodes: detailed.Summaries(rc, render.Render(rc.Report, renderer, filter).Nodes),
 	})
 }
 
 // Individual nodes.
-func handleNode(ctx context.Context, renderer render.Renderer, decorator render.Decorator, rc report.RenderContext, w http.ResponseWriter, r *http.Request) {
+func handleNode(ctx context.Context, renderer render.Renderer, filter render.FilterFunc, rc report.RenderContext, w http.ResponseWriter, r *http.Request) {
 	var (
-		vars             = mux.Vars(r)
-		topologyID       = vars["topology"]
-		nodeID           = vars["id"]
-		preciousRenderer = render.PreciousNodeRenderer{PreciousNodeID: nodeID, Renderer: renderer}
-		rendered         = preciousRenderer.Render(rc.Report, decorator).Nodes
-		node, ok         = rendered[nodeID]
+		vars       = mux.Vars(r)
+		topologyID = vars["topology"]
+		nodeID     = vars["id"]
 	)
+	// We must not lose the node during filtering. We achieve that by
+	// (1) rendering the report with the base renderer, without
+	// filtering, which gives us the node (if it exists at all), and
+	// then (2) applying the filter separately to that result.  If the
+	// node is lost in the second step, we simply put it back.
+	nodes := renderer.Render(rc.Report)
+	node, ok := nodes.Nodes[nodeID]
 	if !ok {
 		http.NotFound(w, r)
 		return
 	}
-	respondWith(w, http.StatusOK, APINode{Node: detailed.MakeNode(topologyID, rc, rendered, node)})
+	if filter != nil {
+		nodes = filter.Apply(nodes)
+		if filteredNode, ok := nodes.Nodes[nodeID]; ok {
+			node = filteredNode
+		} else { // we've lost the node during filtering; put it back
+			nodes.Nodes[nodeID] = node
+			nodes.Filtered--
+		}
+	}
+	respondWith(w, http.StatusOK, APINode{Node: detailed.MakeNode(topologyID, rc, nodes.Nodes, node)})
 }
 
 // Websocket for the full topology.
@@ -118,12 +131,12 @@ func handleWebsocket(
 			log.Errorf("Error generating report: %v", err)
 			return
 		}
-		renderer, decorator, err := topologyRegistry.RendererForTopology(topologyID, r.Form, re)
+		renderer, filter, err := topologyRegistry.RendererForTopology(topologyID, r.Form, re)
 		if err != nil {
 			log.Errorf("Error generating report: %v", err)
 			return
 		}
-		newTopo := detailed.Summaries(RenderContextForReporter(rep, re), renderer.Render(re, decorator).Nodes)
+		newTopo := detailed.Summaries(RenderContextForReporter(rep, re), render.Render(re, renderer, filter).Nodes)
 		diff := detailed.TopoDiff(previousTopo, newTopo)
 		previousTopo = newTopo
 

app/benchmark_internal_test.go

@@ -65,10 +65,10 @@ func BenchmarkTopologyContainers(b *testing.B) {
 
 func benchmarkOneTopology(b *testing.B, topologyID string) {
 	benchmarkRender(b, func(report report.Report) {
-		renderer, decorator, err := topologyRegistry.RendererForTopology(topologyID, url.Values{}, report)
+		renderer, filter, err := topologyRegistry.RendererForTopology(topologyID, url.Values{}, report)
 		if err != nil {
 			b.Fatal(err)
 		}
-		renderer.Render(report, decorator)
+		render.Render(report, renderer, filter)
 	})
 }

render/benchmark_test.go

@@ -52,7 +52,7 @@ func benchmarkRender(b *testing.B, r render.Renderer) {
 		b.StopTimer()
 		render.ResetCache()
 		b.StartTimer()
-		benchmarkRenderResult = r.Render(report, FilterNoop)
+		benchmarkRenderResult = r.Render(report)
 		if len(benchmarkRenderResult.Nodes) == 0 {
 			b.Errorf("Rendered topology contained no nodes")
 		}

render/container.go

@@ -53,10 +53,10 @@ type connectionJoin struct {
 	r     Renderer
 }
 
-func (c connectionJoin) Render(rpt report.Report, dct Decorator) Nodes {
+func (c connectionJoin) Render(rpt report.Report) Nodes {
 	local := LocalNetworks(rpt)
-	inputNodes := c.r.Render(rpt, dct)
-	endpoints := SelectEndpoint.Render(rpt, dct)
+	inputNodes := c.r.Render(rpt)
+	endpoints := SelectEndpoint.Render(rpt)
 
 	// Collect all the IPs we are trying to map to, and which ID they map from
 	var ipNodes = map[string]string{}
@@ -131,9 +131,9 @@ type containerWithImageNameRenderer struct {
 
 // Render produces a container graph where the the latest metadata contains the
 // container image name, if found.
-func (r containerWithImageNameRenderer) Render(rpt report.Report, dct Decorator) Nodes {
-	containers := r.Renderer.Render(rpt, dct)
-	images := SelectContainerImage.Render(rpt, dct)
+func (r containerWithImageNameRenderer) Render(rpt report.Report) Nodes {
+	containers := r.Renderer.Render(rpt)
+	images := SelectContainerImage.Render(rpt)
 
 	outputs := report.Nodes{}
 	for id, c := range containers.Nodes {
@@ -285,7 +285,7 @@ func MapProcess2Container(n report.Node, _ report.Networks) report.Nodes {
 		id = MakePseudoNodeID(UncontainedID, report.ExtractHostID(n))
 		node = NewDerivedPseudoNode(id, n)
 		node = propagateLatest(report.HostNodeID, n, node)
-		node = propagateLatest(IsConnected, n, node)
+		node = propagateLatest(IsConnectedMark, n, node)
 	}
 	return report.Nodes{id: node}
 }

render/container_test.go

@@ -15,18 +15,10 @@ import (
 	"github.com/weaveworks/scope/test/utils"
 )
 
-// FilterApplication is a Renderer which filters out application nodes.
-func FilterApplication(r render.Renderer) render.Renderer {
-	return render.MakeFilter(render.IsApplication, r)
-}
-
-// FilterSystem is a Renderer which filters out system nodes.
-func FilterSystem(r render.Renderer) render.Renderer {
-	return render.MakeFilter(render.IsSystem, r)
-}
-
-// FilterNoop does nothing.
-func FilterNoop(r render.Renderer) render.Renderer { return r }
+var (
+	filterApplication = render.AnyFilterFunc(render.IsPseudoTopology, render.IsApplication)
+	filterSystem      = render.AnyFilterFunc(render.IsPseudoTopology, render.IsSystem)
+)
 
 func TestMapProcess2Container(t *testing.T) {
 	for _, input := range []testcase{
@@ -59,7 +51,7 @@ func testMap(t *testing.T, f render.MapFunc, input testcase) {
 }
 
 func TestContainerRenderer(t *testing.T) {
-	have := utils.Prune(render.ContainerWithImageNameRenderer.Render(fixture.Report, FilterNoop).Nodes)
+	have := utils.Prune(render.ContainerWithImageNameRenderer.Render(fixture.Report).Nodes)
 	want := utils.Prune(expected.RenderedContainers)
 	if !reflect.DeepEqual(want, have) {
 		t.Error(test.Diff(want, have))
@@ -70,13 +62,10 @@ func TestContainerFilterRenderer(t *testing.T) {
 	// tag on of the containers in the topology and ensure
 	// it is filtered out correctly.
 	input := fixture.Report.Copy()
-	renderer := render.ApplyDecorator(render.ContainerWithImageNameRenderer)
-
 	input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{
 		docker.LabelPrefix + "works.weave.role": "system",
 	})
-
-	have := utils.Prune(renderer.Render(input, FilterApplication).Nodes)
+	have := utils.Prune(render.Render(input, render.ContainerWithImageNameRenderer, filterApplication).Nodes)
 	want := utils.Prune(expected.RenderedContainers.Copy())
 	delete(want, fixture.ClientContainerNodeID)
 	if !reflect.DeepEqual(want, have) {
@@ -85,17 +74,15 @@ func TestContainerFilterRenderer(t *testing.T) {
 }
 
 func TestContainerHostnameRenderer(t *testing.T) {
-	renderer := render.ApplyDecorator(render.ContainerHostnameRenderer)
-	have := utils.Prune(renderer.Render(fixture.Report, FilterNoop).Nodes)
+	have := utils.Prune(render.Render(fixture.Report, render.ContainerHostnameRenderer, nil).Nodes)
 	want := utils.Prune(expected.RenderedContainerHostnames)
 	if !reflect.DeepEqual(want, have) {
 		t.Error(test.Diff(want, have))
 	}
 }
 
 func TestContainerHostnameFilterRenderer(t *testing.T) {
-	renderer := render.ApplyDecorator(render.ContainerHostnameRenderer)
-	have := utils.Prune(renderer.Render(fixture.Report, FilterSystem).Nodes)
+	have := utils.Prune(render.Render(fixture.Report, render.ContainerHostnameRenderer, filterSystem).Nodes)
 	want := utils.Prune(expected.RenderedContainerHostnames.Copy())
 	delete(want, fixture.ClientContainerHostname)
 	delete(want, fixture.ServerContainerHostname)
@@ -106,17 +93,15 @@ func TestContainerHostnameFilterRenderer(t *testing.T) {
 }
 
 func TestContainerImageRenderer(t *testing.T) {
-	renderer := render.ApplyDecorator(render.ContainerImageRenderer)
-	have := utils.Prune(renderer.Render(fixture.Report, FilterNoop).Nodes)
+	have := utils.Prune(render.Render(fixture.Report, render.ContainerImageRenderer, nil).Nodes)
 	want := utils.Prune(expected.RenderedContainerImages)
 	if !reflect.DeepEqual(want, have) {
 		t.Error(test.Diff(want, have))
 	}
 }
 
 func TestContainerImageFilterRenderer(t *testing.T) {
-	renderer := render.ApplyDecorator(render.ContainerImageRenderer)
-	have := utils.Prune(renderer.Render(fixture.Report, FilterSystem).Nodes)
+	have := utils.Prune(render.Render(fixture.Report, render.ContainerImageRenderer, filterSystem).Nodes)
 	want := utils.Prune(expected.RenderedContainerHostnames.Copy())
 	delete(want, fixture.ClientContainerHostname)
 	delete(want, fixture.ServerContainerHostname)