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

Return peer count #3860

Merged
merged 4 commits into from
Mar 24, 2022
Merged

Return peer count #3860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2785b2a
Return peer count
dadepo Mar 16, 2022
25a9c92
for given connections for a peerID only pick the relevant connection …
dadepo Mar 21, 2022
1c9787d
Fix test
dadepo Mar 22, 2022
42aeb9f
Added to todo
dadepo Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 28 additions & 6 deletions packages/lodestar/src/api/impl/node/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {routes} from "@chainsafe/lodestar-api";
import {createKeypairFromPeerId} from "@chainsafe/discv5";
import {formatNodePeer} from "./utils";
import {formatNodePeer, getRevelantConnection} from "./utils";
import {ApiError} from "../errors";
import {ApiModules} from "../types";
import {IApiOptions} from "../../options";
Expand Down Expand Up @@ -51,13 +51,35 @@ export function getNodeApi(opts: IApiOptions, {network, sync}: Pick<ApiModules,
},

async getPeerCount() {
// TODO: Implement
// TODO: Implement disconnect count with on-disk persistence
let disconnected = 0;
Copy link
Contributor

@dapplion dapplion Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Lighthouse has a network_globals object that tracks this. So when a peer is disconnected, it is tracked, as can be seen here here which is then read and used to return the peer count as can be seen here only that the network_globals is not persistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let connecting = 0;
let connected = 0;
let disconnecting = 0;

for (const connections of network.getConnectionsByPeer().values()) {
const relevantConnection = getRevelantConnection(connections);
switch (relevantConnection?.stat.status) {
case "open":
connected++;
break;
case "closing":
disconnecting++;
break;
case "closed":
disconnected++;
break;
default:
connecting++;
}
}

return {
data: {
disconnected: 0,
connecting: 0,
connected: 0,
disconnecting: 0,
disconnected,
connecting,
connected,
disconnecting,
},
};
},
Expand Down
2 changes: 1 addition & 1 deletion packages/lodestar/src/api/impl/node/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export function formatNodePeer(peerIdStr: string, connections: Connection[]): ro
* - Otherwise, the first closing connection
* - Otherwise, the first closed connection
*/
function getRevelantConnection(connections: Connection[]): Connection | null {
export function getRevelantConnection(connections: Connection[]): Connection | null {
const byStatus = new Map<PeerStatus, Connection>();
for (const conn of connections) {
if (conn.stat.status === "open") return conn;
Expand Down
41 changes: 41 additions & 0 deletions packages/lodestar/test/unit/api/impl/node/node.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,47 @@ describe("node api implementation", function () {
});
});

describe("getPeerCount", function () {
let peer1: PeerId, peer2: PeerId, peer3: PeerId;

before(async function () {
peer1 = await createPeerId();
peer2 = await createPeerId();
peer3 = await createPeerId();
});

it("it should return peer count", async function () {
const connectionsByPeer = new Map<string, Connection[]>([
[peer1.toB58String(), [libp2pConnection(peer1, "open", "outbound")]],
[
peer2.toB58String(),
[libp2pConnection(peer2, "closing", "inbound"), libp2pConnection(peer2, "closing", "inbound")],
],
[
peer3.toB58String(),
[
libp2pConnection(peer3, "closed", "inbound"),
libp2pConnection(peer3, "closed", "inbound"),
libp2pConnection(peer3, "closed", "inbound"),
],
],
]);

networkStub.getConnectionsByPeer.returns(connectionsByPeer);

const {data: count} = await api.getPeerCount();
expect(count).to.be.deep.equal(
{
connected: 1,
disconnecting: 1, // picks most relevant connection to count
disconnected: 1, // picks most relevant connection to count
connecting: 0,
},
"getPeerCount incorrect"
);
});
});

describe("getPeers", function () {
let peer1: PeerId, peer2: PeerId;

Expand Down