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

Report number of filtered nodes in topology stats. #519

Merged
merged 2 commits into from
Sep 29, 2015

Conversation

tomwilkie
Copy link
Contributor

@davkal this adds a filtered_nodes field to the /api/topology endpoint. You'll need to ensure you pass the same query parameters (ie system=show) to this endpoint to get accurate stats. Could you do the UI pieces?

Fixes #509

@davkal
Copy link
Contributor

davkal commented Sep 24, 2015

Fixes #207

@@ -100,6 +100,17 @@ func makeReportPostHandler(a xfer.Adder) http.HandlerFunc {
}
}

func decorateTopoloyForRequest(r *http.Request, topology *topologyView) {

This comment was marked as abuse.

@peterbourgon
Copy link
Contributor

Looks great! I'd propose replacing "0 filtered" with just nothing, i.e. "11 nodes, 0 filtered" becomes just "11 nodes".

@davkal
Copy link
Contributor

davkal commented Sep 25, 2015

screen shot 2015-09-25 at 15 19 25

Alright, the presence of 0 filtered was confusing.

@tomwilkie
Copy link
Contributor Author

When clicking hide system containers, it doesn't hide them any more. No error reported in logs.

Also, we're not counting filtered psuedo nodes. I'll fix.

@davkal
Copy link
Contributor

davkal commented Sep 28, 2015

Show/hide works for me. Got a report?

@tomwilkie
Copy link
Contributor Author

Report: https://gist.github.com/tomwilkie/bfda494869cfc95a14c1

Screenshot:
screen shot 2015-09-28 at 10 36 18

I'm also seeing an occasional "Uncaught TypeError: Cannot read property 'get' of undefined" in the console.

@tomwilkie
Copy link
Contributor Author

Have tested and can confirm UI works as expected: LGTM for those bits.

Can someone give the go bits a quick look please?

@tomwilkie
Copy link
Contributor Author

@davkal got another issue - "System container hidden" but "2 nodes" showing in status - should be "0 nodes (2 filtered)"

screen shot 2015-09-28 at 11 58 24

To reproduce:

  • load UI
  • click show (next to system containers hidden)
  • switch to applications view
  • switch back to containers view.

@@ -11,6 +11,18 @@ import (
type Renderer interface {
Render(report.Report) RenderableNodes
EdgeMetadata(rpt report.Report, localID, remoteID string) report.EdgeMetadata
Stats(report.Report) Stats
}

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@davkal
Copy link
Contributor

davkal commented Sep 28, 2015

@paulbellamy @tomwilkie if I click hide system containers in the containers topology, do I expect system nodes to be hidden in other topologies, too? Or should that be independent of other topologies?

@tomwilkie
Copy link
Contributor Author

@davkal independent right now. Each topologies defines their own filters.

Tom Wilkie and others added 2 commits September 29, 2015 08:51
Include filtered psuedo nodes in stats.
* always set updated topology object when received
* track whether route was set on initial load
* removed connection count from status bar (was not deemed important)
* fixed issue where topology option changes did not affect details pane
* only show filtered nodes when count > 0
* clear nodes graph when empty topology is loaded
* also prevent JS error if nodes are hovered over that should be gone
* fixed options sync issue between graph and status bar
* implemented topology options with immutable DS
@tomwilkie
Copy link
Contributor Author

That works well @davkal.

I'll rebase, and we're just waiting on an LGTM from @paulbellamy

@peterbourgon
Copy link
Contributor

LGTM.

tomwilkie added a commit that referenced this pull request Sep 29, 2015
Report number of filtered nodes in topology stats.
@tomwilkie tomwilkie merged commit 9281e27 into master Sep 29, 2015
@tomwilkie tomwilkie deleted the 509-filtered-stats branch September 29, 2015 09:23
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.

4 participants