Add filters for pseudo nodes. #1581

tomwilkie · 2016-06-14T11:11:58Z

app/api_topologies.go

+		Default: "show",
+		Options: []APITopologyOption{
+			{"show", "Show pseudo nodes", nil, false},
+			{"hide", "Hide pseudo nodes", render.IsNotPseudo, true},


2opremio · 2016-06-14T11:25:58Z

I'm not fully convinced this is the best way to solve #1129 . Instead of adding another knob I think we should filter out pseudo-nodes not connected to the meaningful nodes of the view being displayed (e.g. container nodes).

rade · 2016-06-14T14:25:30Z

I'm not fully convinced this is the best way to solve #1129

It certainly isn't, but the more interesting question is whether it is a net improvement...

I imagine it will be quite common to have both proper and pseudo nodes connected to the internet, and only wanting to see the former nodes&connections but not the latter. Combined with the "extra knob that really shouldn't be needed", that swings the answer slightly in the 'non' direction for me.

tomwilkie · 2016-06-14T14:31:10Z

Feedback from actual user that inspired this PR contradicts your views:

Give me the option to hide/filter anything that is not a Pod, service, or container. Some of the items on the screen don't make sense to show in most contexts.

davkal · 2016-06-14T14:39:27Z

Maybe this is a naming thing. "pseudo" is Scope jargon. What do those nodes have in common? Are they ancillary, supporting, external, bogus, extra, helper, secondary?

rade · 2016-06-14T14:51:46Z

Feedback from actual user that inspired this PR contradict your views

It does not. Bear in mind that it was me who raised the issue and proposed this very solution as a quick fix. Three months ago. Since then the UI has moved on a lot. Yes, for the users that have no pseudo-nodes connected to their proper nodes the quick fix is an improvement. For them, not showing pseudo nodes at all would be even better. For everybody else it isn't an improvement and it burdens everyone with an additional knob.

If it takes little effort to review this then I am willing to let it through under one condition: we raise a separate issue for the proper fix.

rade · 2016-06-14T14:58:42Z

I imagine it will be quite common to have both proper and pseudo nodes connected to the internet, and only wanting to see the former nodes&connections but not the latter.

It occurs to me that this could be addressed by not including the internet nodes in this filter.

Which would then also address David's concern, since the filter could say 'Uncontained' or 'Unmanaged', which makes it very obvious what this is hiding since it corresponds to labels in the UI.

Question: if we did exclude internet nodes from the filter, would the internet nodes be hidden if the filter is on and only unmanaged/uncontained nodes are connected to them?

davkal · 2016-06-15T09:21:59Z

Question: if we did exclude internet nodes from the filter, would the internet nodes be hidden if the filter is on and only unmanaged/uncontained nodes are connected to them?

I think always showing the internet node (exclude from the filter) would be fine by me. Then it would be independent of the knob.

rade · 2016-06-15T09:24:21Z

My point was that ideally it wouldn't show the internet nodes if they don't have edges to any other shown nodes. So I was wondering whether perhaps that would be happening anyway in the current algorithm.

davkal · 2016-06-15T09:26:32Z

My point was that ideally it wouldn't show the internet nodes if they don't have edges to any other shown nodes. So I was wondering whether perhaps that would be happening anyway in the current algorithm.

Ah yes, that's what I meant too. I had that algo in mind (not showing internet nodes that dont have edges to them)

tomwilkie · 2016-06-15T14:52:34Z

This is a one line change.

tomwilkie · 2016-06-15T15:04:40Z

@2opremio @davkal @rade PTAL

rade · 2016-06-15T15:15:43Z

can't/won't comment on the code, but if it does what I suggested then that SGTM.

render/filters.go

+		if (f.filterPseudo || node.Topology != Pseudo) && f.FilterFunc(node) {
+			output[id] = node
+			inDegrees[id] = 0
+		} else if !f.filterPseudo && node.Topology == Pseudo {


render/filters.go

@@ -73,25 +73,24 @@ func ComposeFilterFuncs(fs ...FilterFunc) FilterFunc {
 // Filter removes nodes from a view based on a predicate.
 type Filter struct {
 	Renderer
-	FilterFunc   FilterFunc
-	filterPseudo bool


- Don't filter the internet node as a pseudo node. - Rename pseudo filter to unmanaged/uncontained. - Review feedback - Move the FilterFoo funcs into the tests - Drop the 'nodes' from filter labels.

tomwilkie assigned 2opremio Jun 14, 2016

2opremio reviewed Jun 14, 2016
View reviewed changes

tomwilkie added this to the 0.16.0 milestone Jun 15, 2016

tomwilkie assigned tomwilkie and unassigned 2opremio Jun 15, 2016

tomwilkie assigned rade, davkal and 2opremio and unassigned tomwilkie Jun 15, 2016

tomwilkie removed this from the 0.16.0 milestone Jun 15, 2016

2opremio reviewed Jun 15, 2016
View reviewed changes

tomwilkie unassigned rade and davkal Jun 15, 2016

2opremio reviewed Jun 15, 2016
View reviewed changes

davkal unassigned 2opremio Jun 16, 2016

davkal assigned tomwilkie Jun 16, 2016

tomwilkie force-pushed the 1129-filter-pseudo branch from d2ad965 to 575805c Compare June 16, 2016 17:21

tomwilkie added 2 commits June 16, 2016 19:09

Add filters for pseudo nodes.

f3bb12a

- Don't filter the internet node as a pseudo node. - Rename pseudo filter to unmanaged/uncontained. - Review feedback - Move the FilterFoo funcs into the tests - Drop the 'nodes' from filter labels.

Fix experimental

48936ef

tomwilkie force-pushed the 1129-filter-pseudo branch from 2c6df19 to 48936ef Compare June 16, 2016 18:09

tomwilkie merged commit c80eb42 into master Jun 16, 2016

tomwilkie deleted the 1129-filter-pseudo branch June 16, 2016 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filters for pseudo nodes. #1581

Add filters for pseudo nodes. #1581

tomwilkie commented Jun 14, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

2opremio commented Jun 14, 2016

rade commented Jun 14, 2016

tomwilkie commented Jun 14, 2016 •

edited

Loading

davkal commented Jun 14, 2016 •

edited

Loading

rade commented Jun 14, 2016 •

edited

Loading

rade commented Jun 14, 2016

davkal commented Jun 15, 2016

rade commented Jun 15, 2016

davkal commented Jun 15, 2016

tomwilkie commented Jun 15, 2016

tomwilkie commented Jun 15, 2016

rade commented Jun 15, 2016

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.

Add filters for pseudo nodes. #1581

Add filters for pseudo nodes. #1581

Conversation

tomwilkie commented Jun 14, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

2opremio commented Jun 14, 2016

rade commented Jun 14, 2016

tomwilkie commented Jun 14, 2016 • edited Loading

davkal commented Jun 14, 2016 • edited Loading

rade commented Jun 14, 2016 • edited Loading

rade commented Jun 14, 2016

davkal commented Jun 15, 2016

rade commented Jun 15, 2016

davkal commented Jun 15, 2016

tomwilkie commented Jun 15, 2016

tomwilkie commented Jun 15, 2016

rade commented Jun 15, 2016

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.

tomwilkie commented Jun 14, 2016 •

edited

Loading

davkal commented Jun 14, 2016 •

edited

Loading

rade commented Jun 14, 2016 •

edited

Loading