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.

Sign up for GitHub

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

Jump to bottom

Show message if topology is empty #505

Merged
merged 1 commit into from
Nov 3, 2015
Merged

Show message if topology is empty #505

merged 1 commit into from
Nov 3, 2015

Conversation

davkal
Copy link
Contributor

@davkal davkal commented Sep 21, 2015

screen shot 2015-09-21 at 19 03 43

@2opremio
Copy link
Contributor

Nice design. I have tried to test it but miserably failed:

screen shot 2015-09-29 at 1 54 24 pm

To force an empty graph, I made the probe not report to the app, providing an unassigned IP to scope:

./scope launch 10.0.0.1

Here's the /api/report:

{"Endpoint":{"Nodes":{}},"Address":{"Nodes":{}},"Process":{"Nodes":{}},"Container":{"Nodes":{}},"ContainerImage":{"Nodes":{}},"Host":{"Nodes":{}},"Overlay":{"Nodes":{}},"Sampling":{"Count":0,"Total":0},"Window":0}

@tomwilkie
Copy link
Contributor

@2opremio try make static

@2opremio
Copy link
Contributor

I did that

tomwilkie
tomwilkie reviewed Oct 29, 2015
View reviewed changes
client/app/scripts/charts/nodes-chart.js
<div className="heading">Nothing to show. This can have any of these reasons:</div>
<ul>
<li>We haven't received any reports from probes recently. Are the probes properly configured?</li>
<li>There are nodes, but they're currently hidden. Check the hidden settings in the bottom-left.</li>

This comment was marked as abuse.

Sign in to view

@tomwilkie
Copy link
Contributor

Works nicely for me:

screen shot 2015-10-29 at 18 06 15

Mind if I make the 1 suggested change and merge it?

@tomwilkie
Copy link
Contributor

NB I rebased this.

@tomwilkie tomwilkie assigned tomwilkie and davkal and unassigned tomwilkie Oct 31, 2015
@tomwilkie
Copy link
Contributor

David wanted to solve some 'flickering' issues with this before it goes in.

@davkal davkal changed the title [WIP] Show message if topology is empty Show message if topology is empty Nov 2, 2015
tomwilkie
tomwilkie reviewed Nov 3, 2015
View reviewed changes
client/app/scripts/stores/app-store.js
@@ -44,6 +44,7 @@ function makeNode(node) {

// Initial values

let changingTopologies = false;

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@davkal
Copy link
Contributor Author

davkal commented Nov 3, 2015

@tomwilkie the implementation based on /api/topology is tricky, because the stats of a container topology return node_count: 0, but then send two nodes over the websocket: theinternet and uncontained. So those are rendered.

So either we adjust the node_count to reflect what would come over the websocket, or I dont render the graph when node_count is 0 as a hard, sufficient condition.

@davkal
Copy link
Contributor Author

davkal commented Nov 3, 2015

Just found another stats field: nonpseudo_node_count. I could use that as a sufficient condition.
Come to think of it, both fields were 0 and still rendered the Internet and Uncontained node. So maybe there is a bug in the stats.

@davkal davkal changed the title Show message if topology is empty [WIP] Show message if topology is empty Nov 3, 2015
@davkal
Copy link
Contributor Author

davkal commented Nov 3, 2015

Alright, a mix of stats and nodes map size does the trick:

currentTopology && currentTopology.stats && currentTopology.stats.node_count === 0 && nodes.size === 0;

@davkal davkal changed the title [WIP] Show message if topology is empty Show message if topology is empty Nov 3, 2015
tomwilkie
tomwilkie reviewed Nov 3, 2015
View reviewed changes
client/app/scripts/stores/__tests__/app-store-test.js
@@ -101,6 +106,15 @@ describe('AppStore', function() {
}
};

const ReceiveNodesDeltaEmptyAction = {

This comment was marked as abuse.

Sign in to view

@tomwilkie
Copy link
Contributor

One minor nit; otherwise LGTM.

I'm fixing master right now; once I have, could you rebase & squash please? Once you get a green off circle, go ahead and merge.

tomwilkie added a commit that referenced this pull request Nov 3, 2015
@tomwilkie
Merge pull request #505 from weaveworks/499-empty
7fdc328 
Show message if topology is empty
@tomwilkie tomwilkie merged commit 7fdc328 into master Nov 3, 2015
@tomwilkie tomwilkie deleted the 499-empty branch November 3, 2015 17:56
@tomwilkie tomwilkie mentioned this pull request Nov 3, 2015
Closed
@tomwilkie tomwilkie added this to the 0.10.0 milestone Nov 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@davkal davkal

Labels
None yet
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@davkal @2opremio @tomwilkie