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

ignore endpoints with >1 adjacency in process rendering #2668

Merged
merged 1 commit into from
Jun 30, 2017

Conversation

rade
Copy link
Member

@rade rade commented Jun 29, 2017

This eliminates the worst effects of #2665.

It is imperfect because it

  • simply drops all connections that originate from the same endpoint, if there is more than one. Not only will this eliminate the problematic connections (i.e. those where it is indeed the case that the same source ip+port was used by different processes), but also those where the source ip+port was used multiple times by the same process. That is unlikely for ephemeral ports (the odds of the same process getting given a particular ephemeral port more than once in a short period of time are much lower than the odds of any two processes getting given a particular ephemeral port in a short period of time, which is the problematic case we are wanting to eliminate here), but a dead certainty when the process is specifically choosing a source port, which is unusual but possible.
  • does not eliminate incorrect process attribution when destination ip+ports are used by different processes. The most common case for that is pre-forking in the likes of apache. In that scenario, all the processes are "siblings". Incorrect attribution is not that bad here; yes, things will be wrong at the process level, but not in a "talking to some completely random process" sort of way, and higher level topologies are unaffected.

I have checked the effects of this change on recent Weave Cloud dev topologies, and it makes them look sane, while not obviously removing any edges that should be there.

@rade rade requested a review from 2opremio June 29, 2017 22:08
@2opremio
Copy link
Contributor

I don't like this solution, but I also don't have a better suggestion, so merge away.

@rade rade merged commit c5b2c9d into master Jun 30, 2017
@rade rade deleted the 2665-connection-pid-attribution branch July 5, 2017 13:07
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.

2 participants