From a82d245e93082a965a12e7ba9e5d3a1af7e7b599 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sat, 18 Nov 2017 15:30:00 +0000 Subject: [PATCH 1/8] simplify render decoration Decoration is in fact quite a simple process that is applied on entry to rendering: we take a base renderer, transform it with a decorator, and then render a report with it. The new render.Decorate() function does exactly that. There is one exception. When rendering an individual node, e.g. for showing its details panel in the UI, we must not lose the node during decoration. That requires some special logic, which previously resided in the PreciousNodeRenderer, and now lives in handleNode. --- app/api_topologies.go | 7 ++----- app/api_topologies_test.go | 6 +++--- app/api_topology.go | 35 +++++++++++++++++++++++++--------- app/benchmark_internal_test.go | 2 +- render/container_test.go | 15 +++++---------- render/filters.go | 18 ----------------- render/pod_test.go | 8 ++------ render/render.go | 24 ++++++++--------------- 8 files changed, 47 insertions(+), 68 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index 78481e3555..0a8ca0a985 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -504,7 +504,7 @@ func decorateWithStats(rpt report.Report, renderer render.Renderer, decorator re realNodes int edges int ) - r := renderer.Render(rpt, decorator) + r := render.Decorate(rpt, renderer, decorator) for _, n := range r.Nodes { nodes++ if n.Topology != render.Pseudo { @@ -541,10 +541,7 @@ func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt } } 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 + return topology.renderer, render.ComposeDecorators(decorators...), nil } return topology.renderer, nil, nil } diff --git a/app/api_topologies_test.go b/app/api_topologies_test.go index 64fda080fa..d468c7947c 100644 --- a/app/api_topologies_test.go +++ b/app/api_topologies_test.go @@ -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.Decorate(input, renderer, decorator).Nodes) want := utils.Prune(expected.RenderedContainers.Copy()) delete(want, fixture.ClientContainerNodeID) delete(want, render.MakePseudoNodeID(render.UncontainedID, fixture.ServerHostID)) @@ -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.Decorate(input, renderer, decorator).Nodes) want := utils.Prune(expected.RenderedContainers.Copy()) delete(want, render.MakePseudoNodeID(render.UncontainedID, fixture.ServerHostID)) delete(want, render.OutgoingInternetID) @@ -183,7 +183,7 @@ func getTestContainerLabelFilterTopologySummary(t *testing.T, exclude bool) (det 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.Decorate(fixture.Report, renderer, decorator).Nodes), nil } func TestAPITopologyAddsKubernetes(t *testing.T) { diff --git a/app/api_topology.go b/app/api_topology.go index 6c95e76425..ffa339e869 100644 --- a/app/api_topology.go +++ b/app/api_topology.go @@ -31,25 +31,42 @@ 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) { respondWith(w, http.StatusOK, APITopology{ - Nodes: detailed.Summaries(rc, renderer.Render(rc.Report, decorator).Nodes), + Nodes: detailed.Summaries(rc, render.Decorate(rc.Report, renderer, decorator).Nodes), }) } // Individual nodes. func handleNode(ctx context.Context, renderer render.Renderer, decorator render.Decorator, 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 decoration. We achieve that by + // (1) rendering the report with the base renderer, without + // decoration, which gives us the node (if it exists at all), then + // (2) performing a normal decorated render of the report. If the + // node is lost in the second step, we simply put it back. + // + // To avoid repeating the work from step (1) in step (2), we + // replace the renderer in the latter with a constant renderer of + // the result obtained in step (1). + nodes := renderer.Render(rc.Report, nil) + 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 decorator != nil { + nodes = render.Decorate(rc.Report, render.ConstantRenderer{Nodes: nodes}, decorator) + if decoratedNode, ok := nodes.Nodes[nodeID]; ok { + node = decoratedNode + } else { // we've lost the node during decoration; 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. @@ -123,7 +140,7 @@ func handleWebsocket( 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.Decorate(re, renderer, decorator).Nodes) diff := detailed.TopoDiff(previousTopo, newTopo) previousTopo = newTopo diff --git a/app/benchmark_internal_test.go b/app/benchmark_internal_test.go index 9eae97bfb7..bbdd87c5d5 100644 --- a/app/benchmark_internal_test.go +++ b/app/benchmark_internal_test.go @@ -69,6 +69,6 @@ func benchmarkOneTopology(b *testing.B, topologyID string) { if err != nil { b.Fatal(err) } - renderer.Render(report, decorator) + render.Decorate(report, renderer, decorator) }) } diff --git a/render/container_test.go b/render/container_test.go index 0d765b21db..156a41ade3 100644 --- a/render/container_test.go +++ b/render/container_test.go @@ -70,13 +70,12 @@ 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.Decorate(input, render.ContainerWithImageNameRenderer, FilterApplication).Nodes) want := utils.Prune(expected.RenderedContainers.Copy()) delete(want, fixture.ClientContainerNodeID) if !reflect.DeepEqual(want, have) { @@ -85,8 +84,7 @@ 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.Decorate(fixture.Report, render.ContainerHostnameRenderer, FilterNoop).Nodes) want := utils.Prune(expected.RenderedContainerHostnames) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) @@ -94,8 +92,7 @@ func TestContainerHostnameRenderer(t *testing.T) { } func TestContainerHostnameFilterRenderer(t *testing.T) { - renderer := render.ApplyDecorator(render.ContainerHostnameRenderer) - have := utils.Prune(renderer.Render(fixture.Report, FilterSystem).Nodes) + have := utils.Prune(render.Decorate(fixture.Report, render.ContainerHostnameRenderer, FilterSystem).Nodes) want := utils.Prune(expected.RenderedContainerHostnames.Copy()) delete(want, fixture.ClientContainerHostname) delete(want, fixture.ServerContainerHostname) @@ -106,8 +103,7 @@ 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.Decorate(fixture.Report, render.ContainerImageRenderer, FilterNoop).Nodes) want := utils.Prune(expected.RenderedContainerImages) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) @@ -115,8 +111,7 @@ func TestContainerImageRenderer(t *testing.T) { } func TestContainerImageFilterRenderer(t *testing.T) { - renderer := render.ApplyDecorator(render.ContainerImageRenderer) - have := utils.Prune(renderer.Render(fixture.Report, FilterSystem).Nodes) + have := utils.Prune(render.Decorate(fixture.Report, render.ContainerImageRenderer, FilterSystem).Nodes) want := utils.Prune(expected.RenderedContainerHostnames.Copy()) delete(want, fixture.ClientContainerHostname) delete(want, fixture.ServerContainerHostname) diff --git a/render/filters.go b/render/filters.go index aa60ebe0b2..6bb888ec43 100644 --- a/render/filters.go +++ b/render/filters.go @@ -14,24 +14,6 @@ const ( swarmNamespaceLabel = "com.docker.stack.namespace" ) -// PreciousNodeRenderer ensures a node is never filtered out by decorators -type PreciousNodeRenderer struct { - PreciousNodeID string - Renderer -} - -// Render implements Renderer -func (p PreciousNodeRenderer) Render(rpt report.Report, dct Decorator) Nodes { - undecoratedNodes := p.Renderer.Render(rpt, nil) - preciousNode, foundBeforeDecoration := undecoratedNodes.Nodes[p.PreciousNodeID] - finalNodes := applyDecorator{ConstantRenderer{undecoratedNodes}}.Render(rpt, dct) - if _, ok := finalNodes.Nodes[p.PreciousNodeID]; !ok && foundBeforeDecoration { - finalNodes.Nodes[p.PreciousNodeID] = preciousNode - finalNodes.Filtered-- - } - return finalNodes -} - // CustomRenderer allow for mapping functions that received the entire topology // in one call - useful for functions that need to consider the entire graph. // We should minimise the use of this renderer type, as it is very inflexible. diff --git a/render/pod_test.go b/render/pod_test.go index 9be35ffb1d..92a7d3a9ca 100644 --- a/render/pod_test.go +++ b/render/pod_test.go @@ -28,13 +28,11 @@ func TestPodFilterRenderer(t *testing.T) { // tag on containers or pod namespace in the topology and ensure // it is filtered out correctly. input := fixture.Report.Copy() - renderer := render.ApplyDecorator(render.PodRenderer) - input.Pod.Nodes[fixture.ClientPodNodeID] = input.Pod.Nodes[fixture.ClientPodNodeID].WithLatests(map[string]string{ kubernetes.Namespace: "kube-system", }) - have := utils.Prune(renderer.Render(input, filterNonKubeSystem).Nodes) + have := utils.Prune(render.Decorate(input, render.PodRenderer, filterNonKubeSystem).Nodes) want := utils.Prune(expected.RenderedPods.Copy()) delete(want, fixture.ClientPodNodeID) if !reflect.DeepEqual(want, have) { @@ -54,13 +52,11 @@ func TestPodServiceFilterRenderer(t *testing.T) { // tag on containers or pod namespace in the topology and ensure // it is filtered out correctly. input := fixture.Report.Copy() - renderer := render.ApplyDecorator(render.PodServiceRenderer) - input.Service.Nodes[fixture.ServiceNodeID] = input.Service.Nodes[fixture.ServiceNodeID].WithLatests(map[string]string{ kubernetes.Namespace: "kube-system", }) - have := utils.Prune(renderer.Render(input, filterNonKubeSystem).Nodes) + have := utils.Prune(render.Decorate(input, render.PodServiceRenderer, filterNonKubeSystem).Nodes) want := utils.Prune(expected.RenderedPodServices.Copy()) delete(want, fixture.ServiceNodeID) delete(want, render.IncomingInternetID) diff --git a/render/render.go b/render/render.go index 3741e89f46..317901d015 100644 --- a/render/render.go +++ b/render/render.go @@ -29,6 +29,14 @@ func (r Nodes) Merge(o Nodes) Nodes { } } +// Decorate renders the report with a decorated renderer +func Decorate(rpt report.Report, renderer Renderer, dct Decorator) Nodes { + if dct != nil { + renderer = dct(renderer) + } + return renderer.Render(rpt, nil) +} + // Reduce renderer is a Renderer which merges together the output of several // other renderers. type Reduce []Renderer @@ -124,22 +132,6 @@ func ComposeDecorators(decorators ...Decorator) Decorator { } } -type applyDecorator struct { - Renderer -} - -func (ad applyDecorator) Render(rpt report.Report, dct Decorator) Nodes { - if dct != nil { - return dct(ad.Renderer).Render(rpt, nil) - } - return ad.Renderer.Render(rpt, nil) -} - -// ApplyDecorator returns a renderer which will apply the given decorator to the child render. -func ApplyDecorator(renderer Renderer) Renderer { - return applyDecorator{renderer} -} - func propagateLatest(key string, from, to report.Node) report.Node { if value, timestamp, ok := from.Latest.LookupEntry(key); ok { to.Latest = to.Latest.Set(key, timestamp, value) From ae153e57f50b6e23fba4e093c770ab45a35a100f Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sat, 18 Nov 2017 16:06:56 +0000 Subject: [PATCH 2/8] refactor: remove Decorator from render.Renderer.Render() signature --- app/api_topology.go | 2 +- render/benchmark_test.go | 2 +- render/container.go | 12 ++++++------ render/container_test.go | 2 +- render/detailed/node_test.go | 8 ++++---- render/detailed/parents_test.go | 10 +++++----- render/detailed/summary_test.go | 4 ++-- render/filters.go | 12 ++++++------ render/filters_test.go | 12 ++++++------ render/host.go | 4 ++-- render/host_test.go | 2 +- render/memoise.go | 10 ++-------- render/memoise_test.go | 10 +++++----- render/pod.go | 2 +- render/pod_test.go | 4 ++-- render/process.go | 12 ++++++------ render/process_test.go | 6 +++--- render/render.go | 18 +++++++++--------- render/render_test.go | 13 +++++-------- render/selectors.go | 2 +- render/short_lived_connections_test.go | 4 ++-- 21 files changed, 71 insertions(+), 80 deletions(-) diff --git a/app/api_topology.go b/app/api_topology.go index ffa339e869..e0efd4e06a 100644 --- a/app/api_topology.go +++ b/app/api_topology.go @@ -51,7 +51,7 @@ func handleNode(ctx context.Context, renderer render.Renderer, decorator render. // To avoid repeating the work from step (1) in step (2), we // replace the renderer in the latter with a constant renderer of // the result obtained in step (1). - nodes := renderer.Render(rc.Report, nil) + nodes := renderer.Render(rc.Report) node, ok := nodes.Nodes[nodeID] if !ok { http.NotFound(w, r) diff --git a/render/benchmark_test.go b/render/benchmark_test.go index 9be78cdd4a..3ce56cb5db 100644 --- a/render/benchmark_test.go +++ b/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") } diff --git a/render/container.go b/render/container.go index 21fa42e0ff..9c8c4424e2 100644 --- a/render/container.go +++ b/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 { diff --git a/render/container_test.go b/render/container_test.go index 156a41ade3..1001f17be1 100644 --- a/render/container_test.go +++ b/render/container_test.go @@ -59,7 +59,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)) diff --git a/render/detailed/node_test.go b/render/detailed/node_test.go index 5d04cf2b9d..7196252416 100644 --- a/render/detailed/node_test.go +++ b/render/detailed/node_test.go @@ -18,7 +18,7 @@ import ( ) func child(t *testing.T, r render.Renderer, id string) detailed.NodeSummary { - s, ok := detailed.MakeNodeSummary(report.RenderContext{Report: fixture.Report}, r.Render(fixture.Report, nil).Nodes[id]) + s, ok := detailed.MakeNodeSummary(report.RenderContext{Report: fixture.Report}, r.Render(fixture.Report).Nodes[id]) if !ok { t.Fatalf("Expected node %s to be summarizable, but wasn't", id) } @@ -30,7 +30,7 @@ func connectionID(nodeID string, addr string) string { } func TestMakeDetailedHostNode(t *testing.T) { - renderableNodes := render.HostRenderer.Render(fixture.Report, nil).Nodes + renderableNodes := render.HostRenderer.Render(fixture.Report).Nodes renderableNode := renderableNodes[fixture.ClientHostNodeID] have := detailed.MakeNode("hosts", report.RenderContext{Report: fixture.Report}, renderableNodes, renderableNode) @@ -178,7 +178,7 @@ func TestMakeDetailedHostNode(t *testing.T) { func TestMakeDetailedContainerNode(t *testing.T) { id := fixture.ServerContainerNodeID - renderableNodes := render.ContainerWithImageNameRenderer.Render(fixture.Report, nil).Nodes + renderableNodes := render.ContainerWithImageNameRenderer.Render(fixture.Report).Nodes renderableNode, ok := renderableNodes[id] if !ok { t.Fatalf("Node not found: %s", id) @@ -308,7 +308,7 @@ func TestMakeDetailedContainerNode(t *testing.T) { func TestMakeDetailedPodNode(t *testing.T) { id := fixture.ServerPodNodeID - renderableNodes := render.PodRenderer.Render(fixture.Report, nil).Nodes + renderableNodes := render.PodRenderer.Render(fixture.Report).Nodes renderableNode, ok := renderableNodes[id] if !ok { t.Fatalf("Node not found: %s", id) diff --git a/render/detailed/parents_test.go b/render/detailed/parents_test.go index 28251d9c14..1a6cdf1d37 100644 --- a/render/detailed/parents_test.go +++ b/render/detailed/parents_test.go @@ -21,25 +21,25 @@ func TestParents(t *testing.T) { }{ { name: "Node accidentally tagged with itself", - node: render.HostRenderer.Render(fixture.Report, nil).Nodes[fixture.ClientHostNodeID].WithParents( + node: render.HostRenderer.Render(fixture.Report).Nodes[fixture.ClientHostNodeID].WithParents( report.MakeSets().Add(report.Host, report.MakeStringSet(fixture.ClientHostNodeID)), ), want: nil, }, { - node: render.HostRenderer.Render(fixture.Report, nil).Nodes[fixture.ClientHostNodeID], + node: render.HostRenderer.Render(fixture.Report).Nodes[fixture.ClientHostNodeID], want: nil, }, { name: "Container image", - node: render.ContainerImageRenderer.Render(fixture.Report, nil).Nodes[expected.ClientContainerImageNodeID], + node: render.ContainerImageRenderer.Render(fixture.Report).Nodes[expected.ClientContainerImageNodeID], want: []detailed.Parent{ {ID: fixture.ClientHostNodeID, Label: fixture.ClientHostName, TopologyID: "hosts"}, }, }, { name: "Container", - node: render.ContainerWithImageNameRenderer.Render(fixture.Report, nil).Nodes[fixture.ClientContainerNodeID], + 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"}, @@ -47,7 +47,7 @@ func TestParents(t *testing.T) { }, }, { - node: render.ProcessRenderer.Render(fixture.Report, nil).Nodes[fixture.ClientProcess1NodeID], + 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"}, diff --git a/render/detailed/summary_test.go b/render/detailed/summary_test.go index e04827f1a6..9d142b4a28 100644 --- a/render/detailed/summary_test.go +++ b/render/detailed/summary_test.go @@ -21,7 +21,7 @@ import ( func TestSummaries(t *testing.T) { { // Just a convenient source of some rendered nodes - have := detailed.Summaries(report.RenderContext{Report: fixture.Report}, render.ProcessRenderer.Render(fixture.Report, nil).Nodes) + have := detailed.Summaries(report.RenderContext{Report: fixture.Report}, render.ProcessRenderer.Render(fixture.Report).Nodes) // The ids of the processes rendered above expectedIDs := []string{ fixture.ClientProcess1NodeID, @@ -51,7 +51,7 @@ func TestSummaries(t *testing.T) { input := fixture.Report.Copy() input.Process.Nodes[fixture.ClientProcess1NodeID].Metrics[process.CPUUsage] = metric - have := detailed.Summaries(report.RenderContext{Report: input}, render.ProcessRenderer.Render(input, nil).Nodes) + have := detailed.Summaries(report.RenderContext{Report: input}, render.ProcessRenderer.Render(input).Nodes) node, ok := have[fixture.ClientProcess1NodeID] if !ok { diff --git a/render/filters.go b/render/filters.go index 6bb888ec43..e09cdf2c51 100644 --- a/render/filters.go +++ b/render/filters.go @@ -23,8 +23,8 @@ type CustomRenderer struct { } // Render implements Renderer -func (c CustomRenderer) Render(rpt report.Report, dct Decorator) Nodes { - return c.RenderFunc(c.Renderer.Render(rpt, dct)) +func (c CustomRenderer) Render(rpt report.Report) Nodes { + return c.RenderFunc(c.Renderer.Render(rpt)) } // ColorConnected colors nodes with the IsConnected key if @@ -123,15 +123,15 @@ func MakeFilterPseudoDecorator(f FilterFunc) Decorator { } // Render implements Renderer -func (f Filter) Render(rpt report.Report, dct Decorator) Nodes { - return f.render(rpt, dct) +func (f Filter) Render(rpt report.Report) Nodes { + return f.render(rpt) } -func (f Filter) render(rpt report.Report, dct Decorator) Nodes { +func (f Filter) render(rpt report.Report) Nodes { output := report.Nodes{} inDegrees := map[string]int{} filtered := 0 - for id, node := range f.Renderer.Render(rpt, dct).Nodes { + for id, node := range f.Renderer.Render(rpt).Nodes { if f.FilterFunc(node) { output[id] = node inDegrees[id] = 0 diff --git a/render/filters_test.go b/render/filters_test.go index 7e2f7434ec..97a2962872 100644 --- a/render/filters_test.go +++ b/render/filters_test.go @@ -16,7 +16,7 @@ func TestFilterRender(t *testing.T) { "baz": report.MakeNode("baz"), }} have := report.MakeIDList() - for id := range renderer.Render(report.MakeReport(), render.FilterUnconnected).Nodes { + for id := range render.Decorate(report.MakeReport(), renderer, render.FilterUnconnected).Nodes { have = have.Add(id) } want := report.MakeIDList("foo", "bar") @@ -41,7 +41,7 @@ func TestFilterRender2(t *testing.T) { "baz": report.MakeNode("baz"), }} - have := renderer.Render(report.MakeReport(), filter).Nodes + have := render.Decorate(report.MakeReport(), renderer, filter).Nodes if have["foo"].Adjacency.Contains("bar") { t.Error("adjacencies for removed nodes should have been removed") } @@ -66,7 +66,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { } } want := nodes - have := renderer.Render(report.MakeReport(), filter).Nodes + have := render.Decorate(report.MakeReport(), renderer, filter).Nodes if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) } @@ -85,7 +85,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("baz"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo), }} - have := renderer.Render(report.MakeReport(), filter).Nodes + have := render.Decorate(report.MakeReport(), renderer, filter).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } @@ -104,7 +104,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("foo"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo).WithAdjacent("bar"), }} - have := renderer.Render(report.MakeReport(), filter).Nodes + have := render.Decorate(report.MakeReport(), renderer, filter).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } @@ -118,7 +118,7 @@ func TestFilterUnconnectedSelf(t *testing.T) { "foo": report.MakeNode("foo").WithAdjacent("foo"), } renderer := mockRenderer{Nodes: nodes} - have := renderer.Render(report.MakeReport(), render.FilterUnconnected).Nodes + have := render.Decorate(report.MakeReport(), renderer, render.FilterUnconnected).Nodes if len(have) > 0 { t.Error("expected node only connected to self to be removed") } diff --git a/render/host.go b/render/host.go index 44e19a36d0..484142260d 100644 --- a/render/host.go +++ b/render/host.go @@ -66,9 +66,9 @@ func MapX2Host(n report.Node, _ report.Networks) report.Nodes { type endpoints2Hosts struct { } -func (e endpoints2Hosts) Render(rpt report.Report, dct Decorator) Nodes { +func (e endpoints2Hosts) Render(rpt report.Report) Nodes { local := LocalNetworks(rpt) - endpoints := SelectEndpoint.Render(rpt, dct) + endpoints := SelectEndpoint.Render(rpt) ret := newJoinResults() for _, n := range endpoints.Nodes { diff --git a/render/host_test.go b/render/host_test.go index 8596793b38..3f30188e61 100644 --- a/render/host_test.go +++ b/render/host_test.go @@ -12,7 +12,7 @@ import ( ) func TestHostRenderer(t *testing.T) { - have := utils.Prune(render.HostRenderer.Render(fixture.Report, FilterNoop).Nodes) + have := utils.Prune(render.HostRenderer.Render(fixture.Report).Nodes) want := utils.Prune(expected.RenderedHosts) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) diff --git a/render/memoise.go b/render/memoise.go index 55cfdab3ef..e4121cea99 100644 --- a/render/memoise.go +++ b/render/memoise.go @@ -37,13 +37,7 @@ func Memoise(r Renderer) Renderer { // retrieves a promise from the cache and returns its value, otherwise // it stores a new promise and fulfils it by calling through to // m.Renderer. -// -// The cache is bypassed when rendering a report with a decorator. -func (m *memoise) Render(rpt report.Report, dct Decorator) Nodes { - if dct != nil { - return m.Renderer.Render(rpt, dct) - } - +func (m *memoise) Render(rpt report.Report) Nodes { key := fmt.Sprintf("%s-%s", rpt.ID, m.id) m.Lock() @@ -56,7 +50,7 @@ func (m *memoise) Render(rpt report.Report, dct Decorator) Nodes { renderCache.Set(key, promise) m.Unlock() - output := m.Renderer.Render(rpt, dct) + output := m.Renderer.Render(rpt) promise.Set(output) diff --git a/render/memoise_test.go b/render/memoise_test.go index ea1acfd5fb..9d7954ae97 100644 --- a/render/memoise_test.go +++ b/render/memoise_test.go @@ -11,7 +11,7 @@ import ( type renderFunc func(r report.Report) render.Nodes -func (f renderFunc) Render(r report.Report, _ render.Decorator) render.Nodes { return f(r) } +func (f renderFunc) Render(r report.Report) render.Nodes { return f(r) } func TestMemoise(t *testing.T) { calls := 0 @@ -22,7 +22,7 @@ func TestMemoise(t *testing.T) { m := render.Memoise(r) rpt1 := report.MakeReport() - result1 := m.Render(rpt1, nil) + result1 := m.Render(rpt1) // it should have rendered it. if _, ok := result1.Nodes[rpt1.ID]; !ok { t.Errorf("Expected rendered report to contain a node, but got: %v", result1) @@ -31,7 +31,7 @@ func TestMemoise(t *testing.T) { t.Errorf("Expected renderer to have been called the first time") } - result2 := m.Render(rpt1, nil) + result2 := m.Render(rpt1) if !reflect.DeepEqual(result1, result2) { t.Errorf("Expected memoised result to be returned: %s", test.Diff(result1, result2)) } @@ -40,7 +40,7 @@ func TestMemoise(t *testing.T) { } rpt2 := report.MakeReport() - result3 := m.Render(rpt2, nil) + result3 := m.Render(rpt2) if reflect.DeepEqual(result1, result3) { t.Errorf("Expected different result for different report, but were the same") } @@ -49,7 +49,7 @@ func TestMemoise(t *testing.T) { } render.ResetCache() - result4 := m.Render(rpt1, nil) + result4 := m.Render(rpt1) if !reflect.DeepEqual(result1, result4) { t.Errorf("Expected original result to be returned: %s", test.Diff(result1, result4)) } diff --git a/render/pod.go b/render/pod.go index ccff7fbaae..a9ee1e73d1 100644 --- a/render/pod.go +++ b/render/pod.go @@ -117,7 +117,7 @@ func renderParents(childTopology string, parentTopologies []string, noParentsPse // to deployments where applicable. type selectPodsWithDeployments struct{} -func (s selectPodsWithDeployments) Render(rpt report.Report, dct Decorator) Nodes { +func (s selectPodsWithDeployments) Render(rpt report.Report) Nodes { result := report.Nodes{} // For each pod, we check for any replica sets, and merge any deployments they point to // into a replacement Parents value. diff --git a/render/pod_test.go b/render/pod_test.go index 92a7d3a9ca..67264fe4bd 100644 --- a/render/pod_test.go +++ b/render/pod_test.go @@ -13,7 +13,7 @@ import ( ) func TestPodRenderer(t *testing.T) { - have := utils.Prune(render.PodRenderer.Render(fixture.Report, nil).Nodes) + have := utils.Prune(render.PodRenderer.Render(fixture.Report).Nodes) want := utils.Prune(expected.RenderedPods) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) @@ -41,7 +41,7 @@ func TestPodFilterRenderer(t *testing.T) { } func TestPodServiceRenderer(t *testing.T) { - have := utils.Prune(render.PodServiceRenderer.Render(fixture.Report, nil).Nodes) + have := utils.Prune(render.PodServiceRenderer.Render(fixture.Report).Nodes) want := utils.Prune(expected.RenderedPodServices) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) diff --git a/render/process.go b/render/process.go index c2415bbc71..ad174a9ad2 100644 --- a/render/process.go +++ b/render/process.go @@ -40,9 +40,9 @@ type processWithContainerNameRenderer struct { Renderer } -func (r processWithContainerNameRenderer) Render(rpt report.Report, dct Decorator) Nodes { - processes := r.Renderer.Render(rpt, dct) - containers := SelectContainer.Render(rpt, dct) +func (r processWithContainerNameRenderer) Render(rpt report.Report) Nodes { + processes := r.Renderer.Render(rpt) + containers := SelectContainer.Render(rpt) outputs := report.Nodes{} for id, p := range processes.Nodes { @@ -86,13 +86,13 @@ var ProcessNameRenderer = ConditionalRenderer(renderProcesses, type endpoints2Processes struct { } -func (e endpoints2Processes) Render(rpt report.Report, dct Decorator) Nodes { +func (e endpoints2Processes) Render(rpt report.Report) Nodes { if len(rpt.Process.Nodes) == 0 { return Nodes{} } local := LocalNetworks(rpt) - processes := SelectProcess.Render(rpt, dct) - endpoints := SelectEndpoint.Render(rpt, dct) + processes := SelectProcess.Render(rpt) + endpoints := SelectEndpoint.Render(rpt) ret := newJoinResults() for _, n := range endpoints.Nodes { diff --git a/render/process_test.go b/render/process_test.go index 0cbbf0c4e8..cdef54a15c 100644 --- a/render/process_test.go +++ b/render/process_test.go @@ -12,7 +12,7 @@ import ( ) func TestEndpointRenderer(t *testing.T) { - have := utils.Prune(render.EndpointRenderer.Render(fixture.Report, FilterNoop).Nodes) + have := utils.Prune(render.EndpointRenderer.Render(fixture.Report).Nodes) want := utils.Prune(expected.RenderedEndpoints) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) @@ -20,7 +20,7 @@ func TestEndpointRenderer(t *testing.T) { } func TestProcessRenderer(t *testing.T) { - have := utils.Prune(render.ProcessRenderer.Render(fixture.Report, FilterNoop).Nodes) + have := utils.Prune(render.ProcessRenderer.Render(fixture.Report).Nodes) want := utils.Prune(expected.RenderedProcesses) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) @@ -28,7 +28,7 @@ func TestProcessRenderer(t *testing.T) { } func TestProcessNameRenderer(t *testing.T) { - have := utils.Prune(render.ProcessNameRenderer.Render(fixture.Report, FilterNoop).Nodes) + have := utils.Prune(render.ProcessNameRenderer.Render(fixture.Report).Nodes) want := utils.Prune(expected.RenderedProcessNames) if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) diff --git a/render/render.go b/render/render.go index 317901d015..4379e829fa 100644 --- a/render/render.go +++ b/render/render.go @@ -12,7 +12,7 @@ type MapFunc func(report.Node, report.Networks) report.Nodes // Renderer is something that can render a report to a set of Nodes. type Renderer interface { - Render(report.Report, Decorator) Nodes + Render(report.Report) Nodes } // Nodes is the result of Rendering @@ -34,7 +34,7 @@ func Decorate(rpt report.Report, renderer Renderer, dct Decorator) Nodes { if dct != nil { renderer = dct(renderer) } - return renderer.Render(rpt, nil) + return renderer.Render(rpt) } // Reduce renderer is a Renderer which merges together the output of several @@ -47,7 +47,7 @@ func MakeReduce(renderers ...Renderer) Renderer { } // Render produces a set of Nodes given a Report. -func (r Reduce) Render(rpt report.Report, dct Decorator) Nodes { +func (r Reduce) Render(rpt report.Report) Nodes { l := len(r) switch l { case 0: @@ -57,7 +57,7 @@ func (r Reduce) Render(rpt report.Report, dct Decorator) Nodes { for _, renderer := range r { renderer := renderer // Pike!! go func() { - c <- renderer.Render(rpt, dct) + c <- renderer.Render(rpt) }() } for ; l > 1; l-- { @@ -83,9 +83,9 @@ func MakeMap(f MapFunc, r Renderer) Renderer { // Render transforms a set of Nodes produces by another Renderer. // using a map function -func (m Map) Render(rpt report.Report, dct Decorator) Nodes { +func (m Map) Render(rpt report.Report) Nodes { var ( - input = m.Renderer.Render(rpt, dct) + input = m.Renderer.Render(rpt) output = report.Nodes{} mapped = map[string]report.IDList{} // input node ID -> output node IDs adjacencies = map[string]report.IDList{} // output node ID -> input node Adjacencies @@ -153,9 +153,9 @@ func ConditionalRenderer(c Condition, r Renderer) Renderer { return conditionalRenderer{c, r} } -func (cr conditionalRenderer) Render(rpt report.Report, dct Decorator) Nodes { +func (cr conditionalRenderer) Render(rpt report.Report) Nodes { if cr.Condition(rpt) { - return cr.Renderer.Render(rpt, dct) + return cr.Renderer.Render(rpt) } return Nodes{} } @@ -166,7 +166,7 @@ type ConstantRenderer struct { } // Render implements Renderer -func (c ConstantRenderer) Render(_ report.Report, _ Decorator) Nodes { +func (c ConstantRenderer) Render(_ report.Report) Nodes { return c.Nodes } diff --git a/render/render_test.go b/render/render_test.go index b671effb24..74c1a25f19 100644 --- a/render/render_test.go +++ b/render/render_test.go @@ -13,10 +13,7 @@ type mockRenderer struct { report.Nodes } -func (m mockRenderer) Render(rpt report.Report, d render.Decorator) render.Nodes { - if d != nil { - return d(mockRenderer{m.Nodes}).Render(rpt, nil) - } +func (m mockRenderer) Render(rpt report.Report) render.Nodes { return render.Nodes{Nodes: m.Nodes} } @@ -30,7 +27,7 @@ func TestReduceRender(t *testing.T) { "foo": report.MakeNode("foo"), "bar": report.MakeNode("bar"), } - have := renderer.Render(report.MakeReport(), FilterNoop).Nodes + have := renderer.Render(report.MakeReport()).Nodes if !reflect.DeepEqual(want, have) { t.Errorf("want %+v, have %+v", want, have) } @@ -47,7 +44,7 @@ func TestMapRender1(t *testing.T) { }}, } want := report.Nodes{} - have := mapper.Render(report.MakeReport(), FilterNoop).Nodes + have := mapper.Render(report.MakeReport()).Nodes if !reflect.DeepEqual(want, have) { t.Errorf("want %+v, have %+v", want, have) } @@ -69,7 +66,7 @@ func TestMapRender2(t *testing.T) { want := report.Nodes{ "bar": report.MakeNode("bar"), } - have := mapper.Render(report.MakeReport(), FilterNoop).Nodes + have := mapper.Render(report.MakeReport()).Nodes if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) } @@ -91,7 +88,7 @@ func TestMapRender3(t *testing.T) { "_foo": report.MakeNode("_foo").WithAdjacent("_baz"), "_baz": report.MakeNode("_baz").WithAdjacent("_foo"), } - have := mapper.Render(report.MakeReport(), FilterNoop).Nodes + have := mapper.Render(report.MakeReport()).Nodes if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) } diff --git a/render/selectors.go b/render/selectors.go index e134aad5b6..6b80e0ac44 100644 --- a/render/selectors.go +++ b/render/selectors.go @@ -9,7 +9,7 @@ import ( type TopologySelector string // Render implements Renderer -func (t TopologySelector) Render(r report.Report, _ Decorator) Nodes { +func (t TopologySelector) Render(r report.Report) Nodes { topology, _ := r.Topology(string(t)) return Nodes{Nodes: topology.Nodes} } diff --git a/render/short_lived_connections_test.go b/render/short_lived_connections_test.go index ebf2fd37a9..214344f3f3 100644 --- a/render/short_lived_connections_test.go +++ b/render/short_lived_connections_test.go @@ -109,7 +109,7 @@ var ( ) func TestShortLivedInternetNodeConnections(t *testing.T) { - have := utils.Prune(render.ContainerWithImageNameRenderer.Render(rpt, FilterNoop).Nodes) + have := utils.Prune(render.ContainerWithImageNameRenderer.Render(rpt).Nodes) // Conntracked-only connections from the internet should be assigned to the internet pseudonode internet, ok := have[render.IncomingInternetID] @@ -123,7 +123,7 @@ func TestShortLivedInternetNodeConnections(t *testing.T) { } func TestPauseContainerDiscarded(t *testing.T) { - have := utils.Prune(render.ContainerWithImageNameRenderer.Render(rpt, FilterNoop).Nodes) + have := utils.Prune(render.ContainerWithImageNameRenderer.Render(rpt).Nodes) // There should only be a connection from container1 and the destination should be container2 container1, ok := have[container1NodeID] if !ok { From 67950985ef81649d52c08bdd9aea4ae3358bfdaa Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sat, 18 Nov 2017 19:51:17 +0000 Subject: [PATCH 3/8] refactor: introduce IsConnected FilterFunc and rename existing IsConnected const to IsConnectedMark --- render/container.go | 2 +- render/detailed/summary.go | 3 +-- render/filters.go | 33 +++++++++++++++++---------------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/render/container.go b/render/container.go index 9c8c4424e2..34760b216a 100644 --- a/render/container.go +++ b/render/container.go @@ -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} } diff --git a/render/detailed/summary.go b/render/detailed/summary.go index 6f0e508ae2..9cb56fbd76 100644 --- a/render/detailed/summary.go +++ b/render/detailed/summary.go @@ -209,8 +209,7 @@ func processNodeSummary(base NodeSummary, n report.Node) (NodeSummary, bool) { base.LabelMinor = fmt.Sprintf("%s (%s)", report.ExtractHostID(n), pid) } - _, isConnected := n.Latest.Lookup(render.IsConnected) - base.Linkable = isConnected + base.Linkable = render.IsConnected(n) return base, true } diff --git a/render/filters.go b/render/filters.go index e09cdf2c51..a88fcff53d 100644 --- a/render/filters.go +++ b/render/filters.go @@ -27,9 +27,9 @@ func (c CustomRenderer) Render(rpt report.Report) Nodes { return c.RenderFunc(c.Renderer.Render(rpt)) } -// ColorConnected colors nodes with the IsConnected key if -// they have edges to or from them. Edges to/from yourself -// are not counted here (see #656). +// ColorConnected colors nodes with the IsConnectedMark key if they +// have edges to or from them. Edges to/from yourself are not counted +// here (see #656). func ColorConnected(r Renderer) Renderer { return CustomRenderer{ Renderer: r, @@ -48,7 +48,7 @@ func ColorConnected(r Renderer) Renderer { output := input.Copy() for id := range connected { - output[id] = output[id].WithLatest(IsConnected, mtime.Now(), "true") + output[id] = output[id].WithLatest(IsConnectedMark, mtime.Now(), "true") } return Nodes{Nodes: output, Filtered: input.Filtered} }, @@ -168,9 +168,10 @@ func (f Filter) render(rpt report.Report) Nodes { return Nodes{Nodes: output, Filtered: filtered} } -// IsConnected is the key added to Node.Metadata by ColorConnected -// to indicate a node has an edge pointing to it or from it -const IsConnected = "is_connected" +// IsConnectedMark is the key added to Node.Metadata by +// ColorConnected to indicate a node has an edge pointing to it or +// from it +const IsConnectedMark = "is_connected" // Complement takes a FilterFunc f and returns a FilterFunc that has the same // effects, if any, and returns the opposite truth value. @@ -178,16 +179,17 @@ func Complement(f FilterFunc) FilterFunc { return func(node report.Node) bool { return !f(node) } } +// IsConnected checks whether the node has been marked with the +// IsConnectedMark. +func IsConnected(node report.Node) bool { + _, ok := node.Latest.Lookup(IsConnectedMark) + return ok +} + // FilterUnconnected produces a renderer that filters unconnected nodes // from the given renderer func FilterUnconnected(r Renderer) Renderer { - return MakeFilterPseudo( - func(node report.Node) bool { - _, ok := node.Latest.Lookup(IsConnected) - return ok - }, - ColorConnected(r), - ) + return MakeFilterPseudo(IsConnected, ColorConnected(r)) } // FilterUnconnectedPseudo produces a renderer that filters @@ -198,8 +200,7 @@ func FilterUnconnectedPseudo(r Renderer) Renderer { if !IsPseudoTopology(node) { return true } - _, ok := node.Latest.Lookup(IsConnected) - return ok + return IsConnected(node) }, ColorConnected(r), ) From 093857f37f29a38d2317cd009a77ee073f8568f2 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Tue, 21 Nov 2017 20:13:14 +0000 Subject: [PATCH 4/8] refactor: extract a little test helper --- render/filters_test.go | 40 ++++++++++++---------------------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/render/filters_test.go b/render/filters_test.go index 97a2962872..2933ad5ea7 100644 --- a/render/filters_test.go +++ b/render/filters_test.go @@ -9,6 +9,15 @@ import ( "github.com/weaveworks/scope/test/reflect" ) +func isNotBar(renderer render.Renderer) render.Renderer { + return &render.Filter{ + FilterFunc: func(node report.Node) bool { + return node.ID != "bar" + }, + Renderer: renderer, + } +} + func TestFilterRender(t *testing.T) { renderer := mockRenderer{Nodes: report.Nodes{ "foo": report.MakeNode("foo").WithAdjacent("bar"), @@ -27,21 +36,12 @@ func TestFilterRender(t *testing.T) { func TestFilterRender2(t *testing.T) { // Test adjacencies are removed for filtered nodes. - filter := func(renderer render.Renderer) render.Renderer { - return &render.Filter{ - FilterFunc: func(node report.Node) bool { - return node.ID != "bar" - }, - Renderer: renderer, - } - } renderer := mockRenderer{Nodes: report.Nodes{ "foo": report.MakeNode("foo").WithAdjacent("bar"), "bar": report.MakeNode("bar").WithAdjacent("foo"), "baz": report.MakeNode("baz"), }} - - have := render.Decorate(report.MakeReport(), renderer, filter).Nodes + have := render.Decorate(report.MakeReport(), renderer, isNotBar).Nodes if have["foo"].Adjacency.Contains("bar") { t.Error("adjacencies for removed nodes should have been removed") } @@ -72,39 +72,23 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { } } { - filter := func(renderer render.Renderer) render.Renderer { - return &render.Filter{ - FilterFunc: func(node report.Node) bool { - return node.ID != "bar" - }, - Renderer: renderer, - } - } renderer := mockRenderer{Nodes: report.Nodes{ "foo": report.MakeNode("foo").WithAdjacent("bar"), "bar": report.MakeNode("bar").WithAdjacent("baz"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo), }} - have := render.Decorate(report.MakeReport(), renderer, filter).Nodes + have := render.Decorate(report.MakeReport(), renderer, isNotBar).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } } { - filter := func(renderer render.Renderer) render.Renderer { - return &render.Filter{ - FilterFunc: func(node report.Node) bool { - return node.ID != "bar" - }, - Renderer: renderer, - } - } renderer := mockRenderer{Nodes: report.Nodes{ "foo": report.MakeNode("foo"), "bar": report.MakeNode("bar").WithAdjacent("foo"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo).WithAdjacent("bar"), }} - have := render.Decorate(report.MakeReport(), renderer, filter).Nodes + have := render.Decorate(report.MakeReport(), renderer, isNotBar).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } From 88e8b52d66e7b3ead2baf97cd239a4f1519646fe Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Tue, 21 Nov 2017 20:16:30 +0000 Subject: [PATCH 5/8] Decorators, begone! Decorators were just a complicated way of constructing filters. --- app/api_topologies.go | 43 ++++++++++++++++------------------ app/api_topologies_test.go | 12 +++++----- app/api_topology.go | 26 ++++++++++---------- app/benchmark_internal_test.go | 4 ++-- render/container_test.go | 28 +++++++--------------- render/filters.go | 16 ------------- render/filters_test.go | 29 +++++++---------------- render/pod_test.go | 8 +++---- render/render.go | 21 ++++------------- 9 files changed, 65 insertions(+), 122 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index 0a8ca0a985..003742bb44 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -336,9 +336,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" @@ -351,7 +350,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 { @@ -374,11 +373,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 ¶m=value to a given topology. @@ -487,24 +484,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 = decorateWithStats(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 = decorateWithStats(rpt, renderer, filter) } topologies = append(topologies, desc) }) return updateFilters(rpt, topologies) } -func decorateWithStats(rpt report.Report, renderer render.Renderer, decorator render.Decorator) topologyStats { +func decorateWithStats(rpt report.Report, renderer render.Renderer, filter render.FilterFunc) topologyStats { var ( nodes int realNodes int edges int ) - r := render.Decorate(rpt, renderer, decorator) + r := render.Render(rpt, renderer, filter) for _, n := range r.Nodes { nodes++ if n.Topology != render.Pseudo { @@ -521,7 +518,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) @@ -533,15 +530,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 { - return topology.renderer, render.ComposeDecorators(decorators...), nil + if len(filters) > 0 { + return topology.renderer, render.ComposeFilterFuncs(filters...), nil } return topology.renderer, nil, nil } @@ -554,7 +551,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) { @@ -572,11 +569,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) } } diff --git a/app/api_topologies_test.go b/app/api_topologies_test.go index d468c7947c..a238adcf41 100644 --- a/app/api_topologies_test.go +++ b/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(render.Decorate(input, renderer, 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(render.Decorate(input, renderer, 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}, render.Decorate(fixture.Report, renderer, decorator).Nodes), nil + return detailed.Summaries(report.RenderContext{Report: fixture.Report}, render.Render(fixture.Report, renderer, filter).Nodes), nil } func TestAPITopologyAddsKubernetes(t *testing.T) { diff --git a/app/api_topology.go b/app/api_topology.go index e0efd4e06a..c418afd14e 100644 --- a/app/api_topology.go +++ b/app/api_topology.go @@ -29,23 +29,23 @@ 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, render.Decorate(rc.Report, renderer, 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"] ) - // We must not lose the node during decoration. We achieve that by + // We must not lose the node during filtering. We achieve that by // (1) rendering the report with the base renderer, without - // decoration, which gives us the node (if it exists at all), then - // (2) performing a normal decorated render of the report. If the + // filtering, which gives us the node (if it exists at all), then + // (2) performing a normal filtered render of the report. If the // node is lost in the second step, we simply put it back. // // To avoid repeating the work from step (1) in step (2), we @@ -57,11 +57,11 @@ func handleNode(ctx context.Context, renderer render.Renderer, decorator render. http.NotFound(w, r) return } - if decorator != nil { - nodes = render.Decorate(rc.Report, render.ConstantRenderer{Nodes: nodes}, decorator) - if decoratedNode, ok := nodes.Nodes[nodeID]; ok { - node = decoratedNode - } else { // we've lost the node during decoration; put it back + if filter != nil { + nodes = render.Render(rc.Report, render.ConstantRenderer{Nodes: nodes}, filter) + 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-- } @@ -135,12 +135,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), render.Decorate(re, renderer, decorator).Nodes) + newTopo := detailed.Summaries(RenderContextForReporter(rep, re), render.Render(re, renderer, filter).Nodes) diff := detailed.TopoDiff(previousTopo, newTopo) previousTopo = newTopo diff --git a/app/benchmark_internal_test.go b/app/benchmark_internal_test.go index bbdd87c5d5..537883b3c0 100644 --- a/app/benchmark_internal_test.go +++ b/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) } - render.Decorate(report, renderer, decorator) + render.Render(report, renderer, filter) }) } diff --git a/render/container_test.go b/render/container_test.go index 1001f17be1..9ec27887ca 100644 --- a/render/container_test.go +++ b/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{ @@ -70,12 +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() - input.Container.Nodes[fixture.ClientContainerNodeID] = input.Container.Nodes[fixture.ClientContainerNodeID].WithLatests(map[string]string{ docker.LabelPrefix + "works.weave.role": "system", }) - - have := utils.Prune(render.Decorate(input, render.ContainerWithImageNameRenderer, 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) { @@ -84,7 +74,7 @@ func TestContainerFilterRenderer(t *testing.T) { } func TestContainerHostnameRenderer(t *testing.T) { - have := utils.Prune(render.Decorate(fixture.Report, render.ContainerHostnameRenderer, 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)) @@ -92,7 +82,7 @@ func TestContainerHostnameRenderer(t *testing.T) { } func TestContainerHostnameFilterRenderer(t *testing.T) { - have := utils.Prune(render.Decorate(fixture.Report, render.ContainerHostnameRenderer, 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) @@ -103,7 +93,7 @@ func TestContainerHostnameFilterRenderer(t *testing.T) { } func TestContainerImageRenderer(t *testing.T) { - have := utils.Prune(render.Decorate(fixture.Report, render.ContainerImageRenderer, 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)) @@ -111,7 +101,7 @@ func TestContainerImageRenderer(t *testing.T) { } func TestContainerImageFilterRenderer(t *testing.T) { - have := utils.Prune(render.Decorate(fixture.Report, render.ContainerImageRenderer, 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) diff --git a/render/filters.go b/render/filters.go index a88fcff53d..e5bdf6278d 100644 --- a/render/filters.go +++ b/render/filters.go @@ -106,22 +106,6 @@ func MakeFilterPseudo(f FilterFunc, r Renderer) Renderer { } } -// MakeFilterDecorator makes a decorator that filters out non-pseudo nodes -// which match the predicate. -func MakeFilterDecorator(f FilterFunc) Decorator { - return func(renderer Renderer) Renderer { - return MakeFilter(f, renderer) - } -} - -// MakeFilterPseudoDecorator makes a decorator that filters out all nodes -// (including pseudo nodes) which match the predicate. -func MakeFilterPseudoDecorator(f FilterFunc) Decorator { - return func(renderer Renderer) Renderer { - return MakeFilterPseudo(f, renderer) - } -} - // Render implements Renderer func (f Filter) Render(rpt report.Report) Nodes { return f.render(rpt) diff --git a/render/filters_test.go b/render/filters_test.go index 2933ad5ea7..5854641fd7 100644 --- a/render/filters_test.go +++ b/render/filters_test.go @@ -9,13 +9,8 @@ import ( "github.com/weaveworks/scope/test/reflect" ) -func isNotBar(renderer render.Renderer) render.Renderer { - return &render.Filter{ - FilterFunc: func(node report.Node) bool { - return node.ID != "bar" - }, - Renderer: renderer, - } +func isNotBar(node report.Node) bool { + return node.ID != "bar" } func TestFilterRender(t *testing.T) { @@ -25,7 +20,7 @@ func TestFilterRender(t *testing.T) { "baz": report.MakeNode("baz"), }} have := report.MakeIDList() - for id := range render.Decorate(report.MakeReport(), renderer, render.FilterUnconnected).Nodes { + for id := range render.Render(report.MakeReport(), render.ColorConnected(renderer), render.IsConnected).Nodes { have = have.Add(id) } want := report.MakeIDList("foo", "bar") @@ -41,7 +36,7 @@ func TestFilterRender2(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("foo"), "baz": report.MakeNode("baz"), }} - have := render.Decorate(report.MakeReport(), renderer, isNotBar).Nodes + have := render.Render(report.MakeReport(), renderer, isNotBar).Nodes if have["foo"].Adjacency.Contains("bar") { t.Error("adjacencies for removed nodes should have been removed") } @@ -57,16 +52,8 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { "baz": report.MakeNode("baz").WithTopology(render.Pseudo), } renderer := mockRenderer{Nodes: nodes} - filter := func(renderer render.Renderer) render.Renderer { - return &render.Filter{ - FilterFunc: func(node report.Node) bool { - return true - }, - Renderer: renderer, - } - } want := nodes - have := render.Decorate(report.MakeReport(), renderer, filter).Nodes + have := render.Render(report.MakeReport(), renderer, nil).Nodes if !reflect.DeepEqual(want, have) { t.Error(test.Diff(want, have)) } @@ -77,7 +64,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("baz"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo), }} - have := render.Decorate(report.MakeReport(), renderer, isNotBar).Nodes + have := render.Render(report.MakeReport(), renderer, isNotBar).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } @@ -88,7 +75,7 @@ func TestFilterUnconnectedPseudoNodes(t *testing.T) { "bar": report.MakeNode("bar").WithAdjacent("foo"), "baz": report.MakeNode("baz").WithTopology(render.Pseudo).WithAdjacent("bar"), }} - have := render.Decorate(report.MakeReport(), renderer, isNotBar).Nodes + have := render.Render(report.MakeReport(), renderer, isNotBar).Nodes if _, ok := have["baz"]; ok { t.Error("expected the unconnected pseudonode baz to have been removed") } @@ -102,7 +89,7 @@ func TestFilterUnconnectedSelf(t *testing.T) { "foo": report.MakeNode("foo").WithAdjacent("foo"), } renderer := mockRenderer{Nodes: nodes} - have := render.Decorate(report.MakeReport(), renderer, render.FilterUnconnected).Nodes + have := render.Render(report.MakeReport(), render.ColorConnected(renderer), render.IsConnected).Nodes if len(have) > 0 { t.Error("expected node only connected to self to be removed") } diff --git a/render/pod_test.go b/render/pod_test.go index 67264fe4bd..01c5983410 100644 --- a/render/pod_test.go +++ b/render/pod_test.go @@ -20,9 +20,7 @@ func TestPodRenderer(t *testing.T) { } } -func filterNonKubeSystem(renderer render.Renderer) render.Renderer { - return render.MakeFilter(render.Complement(render.IsNamespace("kube-system")), renderer) -} +var filterNonKubeSystem = render.Complement(render.IsNamespace("kube-system")) func TestPodFilterRenderer(t *testing.T) { // tag on containers or pod namespace in the topology and ensure @@ -32,7 +30,7 @@ func TestPodFilterRenderer(t *testing.T) { kubernetes.Namespace: "kube-system", }) - have := utils.Prune(render.Decorate(input, render.PodRenderer, filterNonKubeSystem).Nodes) + have := utils.Prune(render.Render(input, render.PodRenderer, filterNonKubeSystem).Nodes) want := utils.Prune(expected.RenderedPods.Copy()) delete(want, fixture.ClientPodNodeID) if !reflect.DeepEqual(want, have) { @@ -56,7 +54,7 @@ func TestPodServiceFilterRenderer(t *testing.T) { kubernetes.Namespace: "kube-system", }) - have := utils.Prune(render.Decorate(input, render.PodServiceRenderer, filterNonKubeSystem).Nodes) + have := utils.Prune(render.Render(input, render.PodServiceRenderer, filterNonKubeSystem).Nodes) want := utils.Prune(expected.RenderedPodServices.Copy()) delete(want, fixture.ServiceNodeID) delete(want, render.IncomingInternetID) diff --git a/render/render.go b/render/render.go index 4379e829fa..502f68715f 100644 --- a/render/render.go +++ b/render/render.go @@ -29,10 +29,10 @@ func (r Nodes) Merge(o Nodes) Nodes { } } -// Decorate renders the report with a decorated renderer -func Decorate(rpt report.Report, renderer Renderer, dct Decorator) Nodes { - if dct != nil { - renderer = dct(renderer) +// Render renders the report and then applies the filter +func Render(rpt report.Report, renderer Renderer, filter FilterFunc) Nodes { + if filter != nil { + renderer = MakeFilterPseudo(filter, renderer) } return renderer.Render(rpt) } @@ -119,19 +119,6 @@ func (m Map) Render(rpt report.Report) Nodes { return Nodes{Nodes: output} } -// Decorator transforms one renderer to another. e.g. Filters. -type Decorator func(Renderer) Renderer - -// ComposeDecorators composes decorators into one. -func ComposeDecorators(decorators ...Decorator) Decorator { - return func(r Renderer) Renderer { - for _, decorator := range decorators { - r = decorator(r) - } - return r - } -} - func propagateLatest(key string, from, to report.Node) report.Node { if value, timestamp, ok := from.Latest.LookupEntry(key); ok { to.Latest = to.Latest.Set(key, timestamp, value) From e643ec56bee15eb9acf8525baa28aeaf337966b3 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 19 Nov 2017 10:17:54 +0000 Subject: [PATCH 6/8] refactor: move filter logic to FilterFunc so it becomes accessible w/o having to construct a Filter renderer. --- render/filters.go | 65 ++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 32 deletions(-) diff --git a/render/filters.go b/render/filters.go index e5bdf6278d..1b920be35c 100644 --- a/render/filters.go +++ b/render/filters.go @@ -82,41 +82,13 @@ func ComposeFilterFuncs(fs ...FilterFunc) FilterFunc { } } -// Filter removes nodes from a view based on a predicate. -type Filter struct { - Renderer - FilterFunc FilterFunc -} - -// MakeFilter makes a new Filter (that ignores pseudo nodes). -func MakeFilter(f FilterFunc, r Renderer) Renderer { - return Filter{ - Renderer: r, - FilterFunc: func(n report.Node) bool { - return n.Topology == Pseudo || f(n) - }, - } -} - -// MakeFilterPseudo makes a new Filter that will not ignore pseudo nodes. -func MakeFilterPseudo(f FilterFunc, r Renderer) Renderer { - return Filter{ - Renderer: r, - FilterFunc: f, - } -} - -// Render implements Renderer -func (f Filter) Render(rpt report.Report) Nodes { - return f.render(rpt) -} - -func (f Filter) render(rpt report.Report) Nodes { +// Apply applies the filter to all nodes +func (f FilterFunc) Apply(nodes Nodes) Nodes { output := report.Nodes{} inDegrees := map[string]int{} filtered := 0 - for id, node := range f.Renderer.Render(rpt).Nodes { - if f.FilterFunc(node) { + for id, node := range nodes.Nodes { + if f(node) { output[id] = node inDegrees[id] = 0 } else { @@ -152,6 +124,35 @@ func (f Filter) render(rpt report.Report) Nodes { return Nodes{Nodes: output, Filtered: filtered} } +// Filter removes nodes from a view based on a predicate. +type Filter struct { + Renderer + FilterFunc FilterFunc +} + +// MakeFilter makes a new Filter (that ignores pseudo nodes). +func MakeFilter(f FilterFunc, r Renderer) Renderer { + return Filter{ + Renderer: r, + FilterFunc: func(n report.Node) bool { + return n.Topology == Pseudo || f(n) + }, + } +} + +// MakeFilterPseudo makes a new Filter that will not ignore pseudo nodes. +func MakeFilterPseudo(f FilterFunc, r Renderer) Renderer { + return Filter{ + Renderer: r, + FilterFunc: f, + } +} + +// Render implements Renderer +func (f Filter) Render(rpt report.Report) Nodes { + return f.FilterFunc.Apply(f.Renderer.Render(rpt)) +} + // IsConnectedMark is the key added to Node.Metadata by // ColorConnected to indicate a node has an edge pointing to it or // from it From 0c435264650630d7b5893c9867397f6072373201 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 19 Nov 2017 10:29:03 +0000 Subject: [PATCH 7/8] refactor: use new FilterFunc.Apply instead of constructing temporary Filter renderers. This also makes clearer what is going on. --- app/api_topology.go | 10 +++------- render/render.go | 15 +++------------ 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/app/api_topology.go b/app/api_topology.go index c418afd14e..e0a3ded161 100644 --- a/app/api_topology.go +++ b/app/api_topology.go @@ -44,13 +44,9 @@ func handleNode(ctx context.Context, renderer render.Renderer, filter render.Fil ) // 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), then - // (2) performing a normal filtered render of the report. If the + // 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. - // - // To avoid repeating the work from step (1) in step (2), we - // replace the renderer in the latter with a constant renderer of - // the result obtained in step (1). nodes := renderer.Render(rc.Report) node, ok := nodes.Nodes[nodeID] if !ok { @@ -58,7 +54,7 @@ func handleNode(ctx context.Context, renderer render.Renderer, filter render.Fil return } if filter != nil { - nodes = render.Render(rc.Report, render.ConstantRenderer{Nodes: nodes}, filter) + nodes = filter.Apply(nodes) if filteredNode, ok := nodes.Nodes[nodeID]; ok { node = filteredNode } else { // we've lost the node during filtering; put it back diff --git a/render/render.go b/render/render.go index 502f68715f..1d7983229b 100644 --- a/render/render.go +++ b/render/render.go @@ -31,10 +31,11 @@ func (r Nodes) Merge(o Nodes) Nodes { // Render renders the report and then applies the filter func Render(rpt report.Report, renderer Renderer, filter FilterFunc) Nodes { + nodes := renderer.Render(rpt) if filter != nil { - renderer = MakeFilterPseudo(filter, renderer) + nodes = filter.Apply(nodes) } - return renderer.Render(rpt) + return nodes } // Reduce renderer is a Renderer which merges together the output of several @@ -147,16 +148,6 @@ func (cr conditionalRenderer) Render(rpt report.Report) Nodes { return Nodes{} } -// ConstantRenderer renders a fixed set of nodes -type ConstantRenderer struct { - Nodes -} - -// Render implements Renderer -func (c ConstantRenderer) Render(_ report.Report) Nodes { - return c.Nodes -} - // joinResults is used by Renderers that join sets of nodes type joinResults struct { nodes report.Nodes From a12d707d9b5adb2c554fc85fecf64e9ae56decc7 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Tue, 21 Nov 2017 19:42:59 +0000 Subject: [PATCH 8/8] refactor: rename decorateWithStats to computeStats to de-decorate --- app/api_topologies.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index 003742bb44..63ae66e284 100644 --- a/app/api_topologies.go +++ b/app/api_topologies.go @@ -485,17 +485,17 @@ func (r *Registry) renderTopologies(rpt report.Report, req *http.Request) []APIT req.ParseForm() r.walk(func(desc APITopologyDesc) { renderer, filter, _ := r.RendererForTopology(desc.id, req.Form, rpt) - desc.Stats = decorateWithStats(rpt, renderer, filter) + desc.Stats = computeStats(rpt, renderer, filter) for i, sub := range desc.SubTopologies { renderer, filter, _ := r.RendererForTopology(sub.id, req.Form, rpt) - desc.SubTopologies[i].Stats = decorateWithStats(rpt, renderer, filter) + desc.SubTopologies[i].Stats = computeStats(rpt, renderer, filter) } topologies = append(topologies, desc) }) return updateFilters(rpt, topologies) } -func decorateWithStats(rpt report.Report, renderer render.Renderer, filter render.FilterFunc) topologyStats { +func computeStats(rpt report.Report, renderer render.Renderer, filter render.FilterFunc) topologyStats { var ( nodes int realNodes int