Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decorators, begone! #2947

Merged
merged 8 commits into from
Nov 21, 2017
Merged

Decorators, begone! #2947

merged 8 commits into from
Nov 21, 2017

Conversation

rade
Copy link
Member

@rade rade commented Nov 19, 2017

Decorators turn out to be an unnecessary abstraction that rather obscures what's going on. So let's get rid of them!

Decorators were used to dynamically construct filters from UI selections - e.g. namespaces, system vs app containers, etc; the selectors in the bottom left corner of the current scope UI - and apply them to the output of topology renderers - pods, containers-by-image, etc; the selectors at the top centre of the current scope UI. Decorators were accomplishing that in a rather complicated way, by composing renderers that wrap the base topology renderers.

This PR unravels all that.

The filter selection from the UI is now turned into a FilterFunc, which we Apply() to the output of topology renderers. The render.Render() function does exactly that, and very clearly so. The one place where we need to do something special is when rendering a single node, e.g. for the details panel in the UI. See comments in the handleNode code.

The PR looks larger than it actually is. There are a bunch of mechanical changes that touch several files, especially the removal of Decorators from the Renderer.Render() signature, and use of the new render.Render() function. The individual commits are self-contained, but the first one is very much an intermediate step which is largely superseded by later commits. Also note that GH does a poor job of showing the diff for the change that moves the filter logic to FilterFunc.Apply().

rade added 2 commits November 18, 2017 18:04
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.
@rade rade requested a review from bboreham November 19, 2017 11:17
@rade
Copy link
Member Author

rade commented Nov 20, 2017

I've tested the correctness of this by running

echo > report.json
for r in processes containers pods hosts \
         "processes?unconnected=hide" \
         "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

against master and this branch and diffing the resulting reports.

and rename existing IsConnected const to IsConnectedMark
@rade rade force-pushed the simplify-decoration branch from 9c8422a to 5095116 Compare November 20, 2017 09:29
Copy link
Collaborator

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice.

I think I would have broken the steps down a little more, e.g. the refactoring to extract isNotBar in render/filters_test.go can stand alone.

}
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 {

This comment was marked as abuse.

rade added 5 commits November 21, 2017 20:13
Decorators were just a complicated way of constructing filters.
so it becomes accessible w/o having to construct a Filter renderer.
instead of constructing temporary Filter renderers.

This also makes clearer what is going on.
@rade rade force-pushed the simplify-decoration branch from b42d456 to a12d707 Compare November 21, 2017 20:18
@rade rade merged commit 8c4ae05 into master Nov 21, 2017
@rade rade deleted the simplify-decoration branch December 25, 2017 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants