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

feat: forest-cli net info #3292

Merged
merged 8 commits into from
Aug 1, 2023
Merged

feat: forest-cli net info #3292

merged 8 commits into from
Aug 1, 2023

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Jul 31, 2023

Summary of changes

As part of #3218

Changes introduced in this pull request:

  • Add forest-cli net info command that prints libp2p stats directly from Swarm::network_info API

Sample output:

forest libp2p swarm info:
num peers: 47
num connections: 59
num pending: 3
num pending incoming: 0
num pending outgoing: 3
num established: 56

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@hanabi1224 hanabi1224 marked this pull request as ready for review July 31, 2023 13:21
@hanabi1224 hanabi1224 requested a review from a team as a code owner July 31, 2023 13:21
@hanabi1224 hanabi1224 requested review from lemmih and LesnyRumcajs and removed request for a team July 31, 2023 13:21
let info = net_info((), &config.client.rpc_token)
.await
.map_err(handle_rpc_err)?;
println!("forest libp2p swarm info:");
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could implement Display for the NetInfoResult?

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 thought about it but not sure if it's good practice to implement a multi-line Display message, formatting a multi-line message with print macro could end up with messy result

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with either approach.

@@ -419,6 +419,10 @@ where
metrics::LIBP2P_MESSAGE_TOTAL
.with_label_values(&[metrics::values::PEER_DISCONNECTED])
.inc();
// Unset heavist tipset for unset peers
Copy link
Member

Choose a reason for hiding this comment

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

why? what does it have to do with this subcommand?

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 was using this to cross-check the correctness of the metrics, it's part of #3218 and I can convert it into a separate PR if you like.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do it in a separate PR and comment on it some more, then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@hanabi1224 hanabi1224 added this pull request to the merge queue Jul 31, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 31, 2023
@hanabi1224 hanabi1224 added this pull request to the merge queue Jul 31, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 31, 2023
@hanabi1224 hanabi1224 added this pull request to the merge queue Aug 1, 2023
Merged via the queue into main with commit cd33993 Aug 1, 2023
20 checks passed
@hanabi1224 hanabi1224 deleted the hm/net-info branch August 1, 2023 00:38
sudo-shashank pushed a commit that referenced this pull request Aug 3, 2023
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.

None yet

3 participants