Reduce the number of places topologies are explicitly listed #2436

ekimekim · 2017-04-10T22:22:12Z

This is a series of changes meant to make it easier to modify the list of topologies
by inferring information or using iterative APIs instead of explicitly referencing each
topology by name, as well as some misc other refactoring. A short list:

Explicitly list topology fields in report.Report methods less often
Refactor how information is looked up in render/detailed so that common functionality to all topologies is not repeated
Refactor Map2Parent, MapContainer2Pod and MapContainer2Task into one function

Much more could be done, but this is a reasonable first pass.

As a side note, I found it very confusing
that the codebase refers to both report topologies (Process, Container, etc) and in-UI topologies (aka views,
eg. container, container by image, container by dns name) as "topologies", in many cases
using the two (different!) "topologyID"s immediately next to each other with no clear distinction.
I have taken to labelling them "topology" vs "apiTopology" in new code.

Note to reviewer: This PR will likely be easiest understood by reading through each commit in order.

By reducing the number of times we refer to every topology by name line by line, we make it easier to add new topologies, reduce the risk of bugs where a topology is not listed, and reduce the risk of the repeated lines getting out of sync with each other. We introduce two new methods to assist this: WalkPairedTopologies, a modified WalkTopologies that gives the called function the same topology from two reports. This is used, for example, to implement Copy and Merge. TopologyMap, which returns a map of all topologies by name. This is then used to implement all other methods. This leaves only 4 instances of listing topologies: In the consts at the top of the file, to give it a name In the struct itself In the constructor, where we need to set per-topology settings In TopologyMap

Prefer WalkTopologies to apply a uniform action to every topology, reducing need to make multiple changes and risk of errors if you forget one.

…opology This gives us a single source of truth in a variety of situations where we want to know what view to direct a user to in order to 'open' a particular node. I wanted to put this in app/api_topologies where the views are defined, but that creates a circular import.

… topologies Currently, if a topology does not have any specific info in nodeSummariesByID, any children of the node that belong to that topology will be silently omitted. This change adds a default behaviour for such topologies, with no special columns but at least it is displayed at all. Unlisted topologies are displayed after all listed ones, in arbitrary order. Note that completely bogus or other special cases (eg. topology = Pseudo) still will not be displayed as report.Topology() will fail.

The default sets the node label to the node ID. This is likely to not look very good, but the intent is that it creates an obvious problem, ie. that the node ID is being used as the label, rather than a silent omission or more subtle problem. Possible future work: * For single-component IDs, extract the component automatically and use that instead. * Instead of functions, in simple cases just have a LUT by topology with common behaviours like 'stack = true or false', 'label = this key in node.Latest' The latter opens up to eventually moving this info inside the report itself ala topology templates, or at least centralizing it in the source.

We replace the existing data structure with a simpler one that only specifies how to get the parent label, which is the only part of the Parent struct that can't be generated from the node info alone. Future work: Standardize this concept of a label and put it in the topology instead. Though that already exists...so just use it?

This greatly improves code reuse while keeping the behaviour flexible

To avoid needing to allocate a new map every time, since we're already hitting GC-related perf issues

Instead, we can infer them from the render topology and the primaryAPITopology map

report/report.go

+	f(&r.Host, &o.Host)
+	f(&r.Overlay, &o.Overlay)
+	f(&r.ECSTask, &o.ECSTask)
+	f(&r.ECSService, &o.ECSService)


ekimekim · 2017-04-11T17:59:50Z

I'm rolling #2091 into this PR, since changes introduced here eliminate the special case that made things work without #2091. Easier than waiting until that change hits master.

ekimekim · 2017-04-14T19:49:07Z

@2opremio since I don't have anyone around who can review this and I need it merged today, I'm just going to merge now. Could you please review when you're back?

report/report.go

-		ECSService:     r.ECSService,
-	}[name]
-	return t, ok
+	if t, ok := r.TopologyMap()[name]; ok {


bboreham

Couple of things I found confusing when going through the commits one by one.
At a high level, there are too many unrelated things in this PR.
However, considering it has already been merged, LGTM.

render/detailed/node.go

-			ID:      apiTopology,
-			Label:   topology.LabelPlural,
-			Columns: []Column{},
+			TopologyID: apiTopology,


render/detailed/summary.go

@@ -84,6 +84,7 @@ var templates = map[string]struct{ Label, LabelMinor string }{

 // For each report.Topology, map to a 'primary' API topology. This can then be used in a variety of places.
 var primaryAPITopology = map[string]string{
+	report.Process:        "processes",


ekimekim · 2017-04-25T17:15:56Z

right, apologies for the messiness between commits, I did try to iron those out but must've missed some.

ekimekim added 9 commits March 30, 2017 16:38

Reduce manually listing all topologies in a few places

75314cb

Prefer WalkTopologies to apply a uniform action to every topology, reducing need to make multiple changes and risk of errors if you forget one.

Refactor Map2Parent and family into one function

3656965

This greatly improves code reuse while keeping the behaviour flexible

report: Reintroduce explicit topology listing for Walk functions

7ba3555

To avoid needing to allocate a new map every time, since we're already hitting GC-related perf issues

Remove explicit listing of api topologies in render/detailed/node specs

9f0f120

Instead, we can infer them from the render topology and the primaryAPITopology map

rade reviewed Apr 11, 2017

View reviewed changes

report/report.go

f(&r.Host, &o.Host)

f(&r.Overlay, &o.Overlay)

f(&r.ECSTask, &o.ECSTask)

f(&r.ECSService, &o.ECSService)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

ecs: Also make service a parent of task

18ba2c4

ekimekim mentioned this pull request Apr 11, 2017

ecs: Make service a parent of task #2091

Closed

ekimekim merged commit 460352d into master Apr 14, 2017

bboreham reviewed Apr 24, 2017

View reviewed changes

report/report.go

ECSService: r.ECSService,

}[name]

return t, ok

if t, ok := r.TopologyMap()[name]; ok {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

bboreham reviewed Apr 24, 2017

View reviewed changes

dlespiau deleted the mike/easier-added-topologies branch November 2, 2017 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the number of places topologies are explicitly listed #2436

Reduce the number of places topologies are explicitly listed #2436

ekimekim commented Apr 10, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ekimekim commented Apr 11, 2017

ekimekim commented Apr 14, 2017

This comment was marked as abuse.

This comment was marked as abuse.

bboreham left a comment

This comment was marked as abuse.

This comment was marked as abuse.

ekimekim commented Apr 25, 2017

Reduce the number of places topologies are explicitly listed #2436

Reduce the number of places topologies are explicitly listed #2436

Conversation

ekimekim commented Apr 10, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ekimekim commented Apr 11, 2017

ekimekim commented Apr 14, 2017

This comment was marked as abuse.

This comment was marked as abuse.

bboreham left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

ekimekim commented Apr 25, 2017