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: get list of ConnectionInfos or an individual node's ConnectionInfo #1435

Merged
merged 13 commits into from
Sep 1, 2023

Conversation

ramfox
Copy link
Contributor

@ramfox ramfox commented Aug 31, 2023

Description

Display connection information for all nodes we have connected with or have supplied our node with addresses for.

Display connection information on a particular node, using its node id.

Change checklist

  • Self-review.
  • Documentation updates if relevant.

@ramfox ramfox requested review from Frando and b5 August 31, 2023 01:48
@ramfox ramfox self-assigned this Aug 31, 2023
@b5
Copy link
Member

b5 commented Aug 31, 2023

ZOMG I WANT THIS.

Howeve, given that you had to censor the screenshot, I'd say right off the bat we should go with listing by NodeID, and omitting IP addresses. wdyt?

@ramfox
Copy link
Contributor Author

ramfox commented Aug 31, 2023

@b5

ZOMG I WANT THIS.

Howeve, given that you had to censor the screenshot, I'd say right off the bat we should go with listing by NodeID, and omitting IP addresses. wdyt?

Screen Shot 2023-08-30 at 22 04 06

What do you think is the "right" language when we don't have any latency info?

@ramfox ramfox requested a review from rklaehn August 31, 2023 02:06
@b5
Copy link
Member

b5 commented Aug 31, 2023

Can we use short NodeIDs, like we do for doc & author ids in the console? We could also do this in a follow-up.

What do you think is the "right" language when we don't have any latency info?

with a latency of Unknown, using "unknown" is the right thing to do here imho

Self::Connections => {
let mut connections = client.server_streaming(ConnectionsRequest).await?;
println!("Connections:");
while let Some(Ok(Ok(ConnectionsResponse { node_info }))) = connections.next().await
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rklaehn can I get some RPC help?

I don't understand how I ended up with double Results. Did I do something incorrectly, or is expected?

Copy link
Member

@Frando Frando Aug 31, 2023

Choose a reason for hiding this comment

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

I don't understand how I ended up with double Results. Did I do something incorrectly, or is expected?

This is expected behavior. The first Result is around a connection error while reading the response, and the second Result is the RpcResult that you are sending as Item in the stream.

@ramfox
Copy link
Contributor Author

ramfox commented Aug 31, 2023

@b5
Screen Shot 2023-08-30 at 22 44 39

Connections:
	hpf6qa4h…
	derp region: 1
	direct connection
	5175250ns latency

	nzx6mxi6…
	derp region: 1
	relay connection
	unknown latency

	xce2h4zq…
	derp region: 1
	relay connection
	unknown latency

	xuko33wo…
	derp region: 1
	direct connection
	3489375ns latency

If you write out one example of how you think it should look, I'll just do it, because it doesn't look right yet but i don't know how to adjust it.

@ramfox
Copy link
Contributor Author

ramfox commented Aug 31, 2023

Connections:
	xuko33wo…
	derp region: 1
	connection type: direct
	latency: 3489375ns 

?

@b5
Copy link
Member

b5 commented Aug 31, 2023

zomg you fast. the problem is you're not doing one entry per line. aim for a table layout:

nodeId    derp region    conn type    latency
xuko33wo…  1             direct       56.5678ms

Huge win here is each row will be a constant-ish length except for the last value (latency), which makes the formatting much easier, just use \t characters between values.

latency should be displayed in ms not ns.

@ramfox
Copy link
Contributor Author

ramfox commented Aug 31, 2023

Screen Shot 2023-08-30 at 23 37 51

will look at other ways to display latency tomorrow!

@ramfox ramfox changed the base branch from main to ramfox/peer_info August 31, 2023 03:47
@ramfox ramfox marked this pull request as ready for review August 31, 2023 03:47
Copy link
Member

@Frando Frando left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Maybe let's add a node connection NODE_ID command as well that shows all details for a single node id, including IP addresses. This info is available at the node, so I think it makes sense to expose it in some way. Useful for debugging etc.

Self::Connections => {
let mut connections = client.server_streaming(ConnectionsRequest).await?;
println!("Connections:");
while let Some(Ok(Ok(ConnectionsResponse { node_info }))) = connections.next().await
Copy link
Member

@Frando Frando Aug 31, 2023

Choose a reason for hiding this comment

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

I don't understand how I ended up with double Results. Did I do something incorrectly, or is expected?

This is expected behavior. The first Result is around a connection error while reading the response, and the second Result is the RpcResult that you are sending as Item in the stream.

iroh/src/node.rs Outdated Show resolved Hide resolved
iroh/src/rpc_protocol.rs Outdated Show resolved Hide resolved
@b5 b5 added this to the v0.6.0-alpha2 milestone Aug 31, 2023
@dignifiedquire
Copy link
Contributor

https://github.com/manlge/human-time makes nicer timestamps

@dignifiedquire
Copy link
Contributor

https://github.com/Nukesor/comfy-table can render easy very nice tables :)

@dignifiedquire
Copy link
Contributor

Can extend this functionality to the console.

lets add this to the repl for sure

@dignifiedquire
Copy link
Contributor

We should add this to the client api as well, this will be very useful from the API

@ramfox ramfox changed the base branch from ramfox/peer_info to main August 31, 2023 17:03
@ramfox ramfox changed the title feat: iroh node connections command to list connection info for all known nodes feat: get list of ConnectionInfos on the iroh client, CLI, & console Aug 31, 2023
@ramfox ramfox force-pushed the ramfox/connections_command branch 3 times, most recently from 0be217a to 7a1cbe8 Compare August 31, 2023 22:15
@ramfox
Copy link
Contributor Author

ramfox commented Sep 1, 2023

iroh node connections

CLI
Screen Shot 2023-08-31 at 23 15 21

Console
Screen Shot 2023-08-31 at 23 15 59

Adjusted the node id field to show the full ID, otherwise you wouldn't be able to fetch any individual node connection information with iroh node connection NODE_ID

iroh node connection NODE_ID

CLI
Screen Shot 2023-08-31 at 23 15 31

Console
Screen Shot 2023-08-31 at 23 16 10

@ramfox ramfox changed the title feat: get list of ConnectionInfos on the iroh client, CLI, & console feat: get list of ConnectionInfos or an individual node's ConnectionInfo Sep 1, 2023
adds `connection_info(node_id: PublicKey)` method to the client & adds `node connection NODE_ID` to the repl

also adjusts some formatting
@dignifiedquire dignifiedquire added this pull request to the merge queue Sep 1, 2023
Merged via the queue into main with commit bdf966e Sep 1, 2023
15 checks passed
@ramfox ramfox deleted the ramfox/connections_command branch November 1, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants