Return peer count #3860
Merged
Return peer count #3860
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3860 +/- ##
=======================================
Coverage 36.11% 36.11%
=======================================
Files 325 325
Lines 9043 9043
Branches 1419 1419
=======================================
Hits 3266 3266
Misses 5634 5634
Partials 143 143 |
|
Trying the same endpoint on lighthouse after running for about 5 minutes returns count for both connected and disconnected, while for lodestar, the endpoint only returns count for connected and nothing for disconnected or the other status. Still need to see if the current way is the most accurate way of getting the connection info from libp2p. |
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
reviewed
Mar 18, 2022
| disconnected++; | ||
| break; | ||
| default: | ||
| connecting++; |
There was a problem hiding this comment.
So if we have 1 peer with 20 open connections it will return connected: 20, is this the expected behavior?
There was a problem hiding this comment.
updated to use the most relevant connection for a peer in the counting
…and used that in returning the peer count
Looked into trying to get disconnected peer by making a diff with the peers found in the peerstore and the one returned by the connection manager (reasoning is if it is found in the peerstore but not in the connection manager, then those are disconnected). The issue with that is the peerstore persists across restarts, meaning the result of the diff would keep on increasing. Leaving the implementation for now to use the connection manager for all the counting. Open to other suggestions, if that is not fit enough |
wemeetagain
previously approved these changes
Mar 22, 2022
dapplion
reviewed
Mar 23, 2022
| @@ -51,13 +51,34 @@ export function getNodeApi(opts: IApiOptions, {network, sync}: Pick<ApiModules, | |||
| }, | |||
|
|
|||
| async getPeerCount() { | |||
| // TODO: Implement | |||
| let disconnected = 0; | |||
There was a problem hiding this comment.
So disconnected means all peers that have ever connected to us and are no longer connected? That sounds like a memory leak to track lol. How does Lighthouse track this?
There was a problem hiding this comment.
It's persisted to disk that's how it doesn't leak memory. I would leave a // TODO: Implement disconnect count with on-disk persistence
There was a problem hiding this comment.
I assumed it is not being persisted as the values don't persist after each restarts. Will dig later to see how that is implemented...
I would leave a // TODO: Implement disconnect count with on-disk persistence
Ok. Will do.
There was a problem hiding this comment.
Will dig later to see how that is implemented...
I would ignore completely, tracking disconnected peers is an extremely unnecessary feature in my opinion haha
wemeetagain
approved these changes
Mar 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Implements the endpoint to get the peer count which was not implemented before
Closes #3857
Steps to test or reproduce
curl localhost:9596/eth/v1/node/peer_countshould not return just 0 for all values