filter out unconnected pseudo nodes just once, at the end #2951

rade · 2017-11-24T10:29:02Z

The current rendering pipeline looks like this:

render . filterUnconnectedPseudo . memoise . filter(options)

We render a report, filter unconnected pseudo nodes, memoise the result, and apply user-specified filters.

Some of the steps are composites. filterUnConnectedPseudo nodes colours connected nodes in one step and then applies an ordinary filter that drops uncoloured nodes:

filterUnconnectedPseudo = colorConnected . filter(unconnectedPseudo)

And filtering always filters unconnected pseudo nodes:

filter(x) = filter'(x) . filterU

where filter'(x) is the "raw" filter operation and filterU is the "filter unconnected pseudo nodes" step.

So the current pipeline actually looks like this:

render . colorConnected . filter'(unconnectedPseudo) . filterU . memoise . filter'(options) . filterU

There are couple of problems with this. Firstly, we filter unconnected pseudo nodes three times, which is inefficient. Secondly, colorConnected marking is expensive since it updates nodes' LatestMap.

Thirdly, while the filtering done by colorConnected . filter'(unconnectedPseudo) drops nodes which are only connected to themselves, filterU fails to do that. Hence end result may contain such nodes because the colorConnected step and subsequent filtering occurs prior to applying the user-specified filters.

The changes in this PR yield a pipeline like this:

render . memoise . filter'(options) . filter'(unconnectedPseudo')

where unconnectedPseudo' is a filter predictate that is dynamically constructed from a calculation of a connected nodes, that correctly excludes self-connections.

One slight complication is that for the process topology we want to filter all unconnected nodes, not just unconnected pseudo nodes. We do this by calculating connected nodes differently in that case.

There is one wart: we cannot get rid of ColorConnected completely. We still need it in ColorConnectedProcessRenderer, which uses it to mark nodes; the marks are required by detailed.processNodeSummary to determine whether a process in the details panel can be rendered as a link.

rade · 2017-11-24T13:54:17Z

There's no significant improvement in performance, which is ok since correctness and saner structure alone are sufficient motivators for this PR.

Best result of three of running go test -tags 'unsafe' -run=x -cpu=2 -bench=Topology -bench-report-file=/tmp/prod-on-prod-report.json
master

BenchmarkTopologyList-2         	       2	 696855104 ns/op	97747168 B/op	  918009 allocs/op
BenchmarkTopologyHosts-2        	       2	 553922460 ns/op	72166384 B/op	  734743 allocs/op
BenchmarkTopologyContainers-2   	       5	 269104905 ns/op	30147406 B/op	  303597 allocs/op

branch

BenchmarkTopologyList-2         	       2	 670668899 ns/op	94420696 B/op	  908547 allocs/op
BenchmarkTopologyHosts-2        	       2	 511880816 ns/op	71913592 B/op	  733575 allocs/op
BenchmarkTopologyContainers-2   	       5	 253800350 ns/op	29085857 B/op	  302659 allocs/op

And time curl -s "http://localhost:4040/api/topology/<topology>...

	master	branch
processes	0.236	0.240
containers	0.706	0.679
pods	0.464	0.566
services	0.549	0.484
hosts	0.690	0.652

rade · 2017-11-24T15:18:10Z

I've checked the correctness by verifying that the output of

echo > report.json
for r in processes containers pods hosts \
         "containers?pseudo=hide&namespace=default,scope&system=application&stopped=running" \
         "pods?pseudo=hide&namespace=default,scope" ; do \
    echo $r
    curl -s "http://localhost:4040/api/topology/$r" | jq -S '.' >> report.json; \
done

is identical between master and branch.

bboreham

I found this PR very hard to follow. Maybe that is unavoidable.

render/filters_test.go

-func isNotBar(node report.Node) bool {
-	return node.ID != "bar"
-}
+var isNotBar = render.Transformers([]render.Transformer{


app/api_topologies_test.go

@@ -164,14 +164,14 @@ func getTestContainerLabelFilterTopologySummary(t *testing.T, exclude bool) (det

 	var (
 		topologyRegistry = app.MakeRegistry()
-		filter           render.FilterFunc
+		filterFunc       render.FilterFunc


app/api_topology.go

-			nodes.Nodes[nodeID] = node
-			nodes.Filtered--
-		}
+	nodes = transformer.Transform(nodes)


app/api_topologies.go

@@ -553,9 +553,9 @@ func (r *Registry) RendererForTopology(topologyID string, values url.Values, rpt
 		}
 	}
 	if len(filters) > 0 {
-		return topology.renderer, render.ComposeFilterFuncs(filters...), nil
+		return topology.renderer, render.Transformers([]render.Transformer{render.ComposeFilterFuncs(filters...), topology.filter}), nil


rade · 2017-11-27T17:42:34Z

@bboreham Addressed comments. PTAL

Instead of three passes 1. building a 'connected' node set 2. marking nodes from that set with a LatestMap entry 3. removing unmarked nodes we just do two 1. building a 'connected' node set 2. removing nodes not in that set This does entail duplication of the adjecency list pruning code from FilterFunc.Apply. We will be able to eliminate that eventually, but not just yet. Also, we cannot get rid of ColorConnected completely; ColorConnectedProcessRenderer uses it to mark nodes, and that information is required by detailed.processNodeSummary to determine whether a process in the details panel can be rendered as a link.

so we can generalise the filter step in render.Render et al. That will allow us to apply whole-topology filters in that step.

This is a step towards filtering unconnected nodes after all custom filters have been applied.

...rather than before. That way, nodes which become unconnected during filtering are removed, which is what we want. ATM we are depending on some 'unconnected' filtering inside every filter, which is expensive and largely redundant. We should soon be able to remove that. downside: 'unconnected' filtering is no longer memoised.

It's now done via a special filter, once, after all other filters have been applied. Some tests need updating since they were relying on ordinary filters doing that filtering.

This eliminates the awkward distinction between ProcessRenderer and ColorConnectedProcessRenderer. It also ensures that processes resulting from direct rendering of the process topology (/api/topology/processes is invoking ProcessWithContainerNameRenderer and /api/topology/processes-by-name is invoking ProcessNameRenderer) are colored and hence summarising them correctly sets the 'linkable' property. This was the behaviour prior to the revamping of the rendering pipeline. However, it doesn't actually make a practical difference since process detail panels only show other processes as connection endpoints, and these are always marked linkable anyway.

rade requested a review from bboreham November 24, 2017 10:29

bboreham requested changes Nov 27, 2017

View reviewed changes

rade force-pushed the one-pass-unconnected branch from cbfc7af to 2b5bc80 Compare November 27, 2017 17:39

bboreham approved these changes Nov 28, 2017

View reviewed changes

rade added 7 commits November 28, 2017 06:54

refactor(ish): introduce Tranformers

7bbded9

so we can generalise the filter step in render.Render et al. That will allow us to apply whole-topology filters in that step.

refactor: separate filtering from rendering in topology description

2d964b6

This is a step towards filtering unconnected nodes after all custom filters have been applied.

remove filtering of unconnected pseudo nodes from ordinary filters

1416fe9

It's now done via a special filter, once, after all other filters have been applied. Some tests need updating since they were relying on ordinary filters doing that filtering.

refactor: remove duplication of filtering logic

f193a21

rade force-pushed the one-pass-unconnected branch from 2b5bc80 to 9563036 Compare November 28, 2017 07:36

rade merged commit e73ba58 into master Nov 28, 2017

rade deleted the one-pass-unconnected branch December 25, 2017 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filter out unconnected pseudo nodes just once, at the end #2951

filter out unconnected pseudo nodes just once, at the end #2951

rade commented Nov 24, 2017

rade commented Nov 24, 2017 •

edited

Loading

rade commented Nov 24, 2017

bboreham left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Nov 27, 2017

filter out unconnected pseudo nodes just once, at the end #2951

filter out unconnected pseudo nodes just once, at the end #2951

Conversation

rade commented Nov 24, 2017

rade commented Nov 24, 2017 • edited Loading

rade commented Nov 24, 2017

bboreham left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Nov 27, 2017

rade commented Nov 24, 2017 •

edited

Loading