-
Notifications
You must be signed in to change notification settings - Fork 712
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
Hide pseudo nodes #99
Comments
They're not phantom. They're real. Scope is making it clear that your browser is opening lots of short-lived connections to the app. If you install a probe on your local machine, you will see them grouped into the Chrome process. (Or maybe not into a single process given Chrome's architecture, but they'll at least all be labeled Chrome.)
I personally have no problem with the current behavior, and even prefer it, as it closely aligns with the idea of Scope presenting "just the facts". But we can prototype other behaviors and see how they feel. |
I think the problem comes when you have ~10 of these connections hanging around, and they start to detract from the real graph you want to see. I also guess you guys don't see the problem as you are running probes on your local machine - not something I think our intended audience will be doing. Perhaps an alternative would be to group by unique (local process, port, remote ip), so all the processes connecting to the app from one machine would be grouped into one, but any processes on the same machine connecting to something else would be distinct? |
For the record, the official term for nodes in a rendered topology that don't come from a probed machine, but rather are just the "other side" of a probed connection, are pseudo-nodes. It won't be possible to group them based on the other side of their connection. Remember the mapping of physical node to rendered node is done with a MapFunc, which has as input only the node itself (and the 'grouped' boolean). It's also unclear what to do when a single pseudo-node has multiple known connections, or vice-versa. I'll try 3 versions:
and present some options. |
@tomwilkie alternately, what do you think about just rendering the pseudo-nodes much smaller/grayer than the others? |
Let me have a play and see what I can come up with. |
/me bows |
Heres a poc master...tomwilkie:pseudo-nodes Its groups together all pseudo nodes connecting to the same addr:port from the same addr. |
A fine approach, IMO, modulo tests :) |
Okay good to hear. I've added tests for pseudo nodes being emitted only once per (remote address, local address, local port) and I'm just going to add another test to ensure they are not being "over" merged, and one for non-pid topologies. Any others you can think of? |
That sounds reasonable — make sure the Network topology test includes a few pseudonodes and we should be good. The pseudonode story isn't strongly tested ATM so I'm just looking for some meaningful attention. |
Guess this is done now. |
8949b2b Merge pull request #98 from weaveworks/git-status-tag ac30687 Merge pull request #100 from weaveworks/python_linting 4b125b5 Pin yapf & flake8 versions 7efb485 Lint python linting function 444755b Swap diff direction to reflect changes required c5b2434 Install flake8 & yapf 5600eac Lint python in build-tools repo 0b02ca9 Add python linting c011c0d Merge pull request #79 from kinvolk/schu/python-shebang 6577d07 Merge pull request #99 from weaveworks/shfmt-version 00ce0dc Use git status instead of diff to add 'WIP' tag 411fd13 Use shfmt v1.3.0 instead of latest from master. 31d069d Change Python shebang to `#!/usr/bin/env python` git-subtree-dir: tools git-subtree-split: 8949b2b5eaa698701c681273c23de18bcc9a4f90
74dc626 Merge pull request #108 from weaveworks/disable-apt-daily b4f1d91 Merge pull request #107 from weaveworks/docker-17-update 7436aa1 Override apt daily job to not run immediately on boot 7980f15 Merge pull request #106 from weaveworks/document-docker-install-role f741e53 Bump to Docker 17.06 from CE repo 61796a1 Update Docker CE Debian repo details 0d86f5e Allow for Docker package to be named docker-ce 065c68d Document selection of Docker installation role. 3809053 Just --porcelain; it defaults to v1 11400ea Merge pull request #105 from weaveworks/remove-weaveplugin-remnants b8b4d64 remove weaveplugin remnants 35099c9 Merge pull request #104 from weaveworks/pull-docker-py cdd48fc Pull docker-py to speed tests/builds up. e1c6c24 Merge pull request #103 from weaveworks/test-build-tags d5d71e0 Add -tags option so callers can pass in build tags 8949b2b Merge pull request #98 from weaveworks/git-status-tag ac30687 Merge pull request #100 from weaveworks/python_linting 4b125b5 Pin yapf & flake8 versions 7efb485 Lint python linting function 444755b Swap diff direction to reflect changes required c5b2434 Install flake8 & yapf 5600eac Lint python in build-tools repo 0b02ca9 Add python linting c011c0d Merge pull request #79 from kinvolk/schu/python-shebang 6577d07 Merge pull request #99 from weaveworks/shfmt-version 00ce0dc Use git status instead of diff to add 'WIP' tag 411fd13 Use shfmt v1.3.0 instead of latest from master. 31d069d Change Python shebang to `#!/usr/bin/env python` git-subtree-dir: tools git-subtree-split: 74dc626b6de3ffb38591510f7cb7bc2db33743c4
I'm not sure phantom is the right term, but these are connections for which we don't have information about one end:
Right now, we should all the connections. Its only three above, but I've seen it be as many as 10, and its tends to dominate the view then.
We could:
I'm leaning towards (1), but could be persuaded by (2) or (3). What do you guys think?
The text was updated successfully, but these errors were encountered: