chore: update peers table

[fsdiogo]

Here goes an initial refactor of the peers page discussed in #1058:

• Simplify the peers location
• Add a column showing the protocol and transport
• Add latency
• Add notes showing more info

[olizilla]

Some visual tweaks

• The latency column would be more helpful all the values are in milliseconds, and text-align right and monospace, so the user can visually scan the values to compare them.
• Where we don't know a latency value yet, try just a faded / light gray - or similar. From the users point of view, it's a little confusing to be told that latency is "not applicable" for a given connection, and I think we really mean, "we dont know"
• Try the <Cid> element for displaying the PeerId, as it'll truncate them in the middle. I think it'll be a little neater than truncating some amount of the right hand side.
• To visually anchor each row, lets try putting the Location flags as the left most column.
• Connection column is cool. Icons for different connection types might be rad (nice to have, not a blocker)

Also, paging @ericronne

[ericronne]

Some interesting stuff going on here. Currently scratching my head and wireframing a little on this …

[olizilla]

@ericronne Let's chat about it on the call in a few mins!

[ericronne]

Yes! Until then, would an approach such as this reflect reality? Thought is to fold related mystery peers into an accordion.

[olizilla]

@ericronne nice, but the location column is a best effort, and only works for peers who are connected via ipv4 connections... we don't want to give folks who have embraced the ipv6 future a second class status in the peers tab.

[lidel]

Posting quick feedback I mentioned during the call:

Location and Latency columns should be next to each other. If the default sort method uses either of them, peers from the same countries will usually have similar values, which will make the latency-location link more intuitive.

[JustMaier]

I added sorting by latency, on my fork of this branch, but I'm not sure if it makes sense to open a separate PR for it. How should I proceed?

[JustMaier]

Took a stab at adding peer Identicons to as well. What do you all think?

[lidel]

Identicons solve a lot of issues related to PeerIDs being super long and hard to compare.
It is subjective, but I was able to immediately tell which peers share the same relay.

We probably need to tweak the color palette used for creating identicons, to ensure they feel as they belong to the UI and IPFS colors, but I am 👍 for adding them, especially if we move LOCATION column between CONNECTION and LATENCY – PEER ID column will look more interesting with them.

@JustMaier Are we able to generate them with CSS?

[JustMaier]

@lidel We can pull the colors from the theme.json in ipfs-css, but we can't generate the icons in CSS.

Here's what it looks like with the theme colors:

I'm wondering if maybe it'd look better with a different style of identicon though. Here's a different type of identicon:

Between the two, I think I prefer the first one though...

[lidel]

@JustMaier let's move Identicon discussion to #935 to unblock this PR (if we add identicons, it should be in a separate PR)

[fsdiogo]

I added sorting by latency, on my fork of this branch, but I'm not sure if it makes sense to open a separate PR for it. How should I proceed?

Thanks for this @JustMaier! I'd argue we should add the ability to sort the columns as we have in the FilesPage, I'd prefer to leave the natural sorting instead of sorting by latency by default.

What do you think?

[JustMaier]

@fsdiogo I agree. It'd be nice to be able to sort the table and it looks like it's a feature in Table. I can take a stab at that. What would be the best way for me to submit that change? A PR to this branch?

As for default sorting, I do think that there should be some default, the "natural" sorting is just random as far as I can tell, and latency seems like a decent default since it's important to peer network performance.

[fsdiogo]

@fsdiogo I agree. It'd be nice to be able to sort the table and it looks like it's a feature in Table. I can take a stab at that. What would be the best way for me to submit that change? A PR to this branch?

Yeah, you can either open a PR to this branch or we could merge this one and you PR master after.

That way we could open another one to add the identicons too!

[JustMaier]

Just submitted the PR to this branch. In my opinion, the change is small enough that it should be rolled into this branch/PR.

[olizilla]

@fsdiogo please double check the logic for how we determine if a peer is a bootstrap node. I am connected to a bootstrap node, but it does not show up in the notes field:

$ ipfs bootstrap | grep 128.199.219.111
/ip4/128.199.219.111/tcp/4001/ipfs/QmSoLSafTMBsPKadTEgaXctDQVcqN88CNLHXMkTNwMKPnu

$ ipfs swarm peers | grep 128.199.219.111
/ip4/128.199.219.111/tcp/4001/ipfs/QmSoLSafTMBsPKadTEgaXctDQVcqN88CNLHXMkTNwMKPnu

[olizilla]

@fsdiogo please right align the latency column so the user can visually scan the numbers

[olizilla]

@JustMaier very exicited to see your work on peer identity icons. In general we've been sticking to keeping PRs as small and focused as possible, even if that means having lots of small ones. We've also found it useful to keep a single contributer per PR so that each changeset has a clear owner to drive it through. We'll get this PR merged today, then we can focus on sorting and icons!

[olizilla]

@fsdiogo could you give some background on why we only display the country code? I'm mildly in favour of being less explicit about the inferred location of a given peer, and I still feel that it sends a confusing message to new users (is ipfs tracking me? is my node telling people where I live?) but as @ericronne pointed out it's perhaps the only information that we show that has immediate meaning to new users, and it's kinda fun to know that there are folks all over the world using IPFS.

The change to use just the 2 letter country code makes the geolocation more corse grained, but perhaps US and CN are just too large an area to be interesting. Also we used to use the english name instead of the country code. I am a biased english speaker, but my guess is that country names even in english are more widely recogniseable, for counties other than your own than the 2 letter code.

Unless there are strong reasons to change the location format, I suggest we revert it the location values to how they are in the current release.

[fsdiogo]

@fsdiogo could you give some background on why we only display the country code?

Yeah, my main point was for privacy reasons, as you said, so the user wouldn't think we were tracking them. The other reason was to make the table a bit less wide.

Alright, I'll revert to keep it as is! 👍

[fsdiogo]

@fsdiogo please right align the latency column so the user can visually scan the numbers

Done!

Just submitted the PR to this branch. In my opinion, the change is small enough that it should be rolled into this branch/PR.

Thanks so much for your contribution @JustMaier 🙌

I agree with @olizilla, we should hold it for a minute until this one is merged, then we'll review and merge yours!

[fsdiogo]

@fsdiogo please double check the logic for how we determine if a peer is a bootstrap node. I am connected to a bootstrap node, but it does not show up in the notes field:

Yeah, something is not right.

What I'm doing right now is fetching the bootstrap peers that are in the current IPFS config and comparing them with the current peer being analyzed.

It doesn't seem to be working because the current peer is something like

/ip4/18.185.241.99/tcp/20012

and a bootstraps is

/ip4/104.236.179.241/tcp/4001/ipfs/QmSoLPppuBtQSGwKDZT2M73ULpjvfd3aZ6ha4oFGL1KrGM

so they never match up.

Am I missing something obvious?

[olizilla]

The items in the array returned from ipfs.swarm.peers() have the the peerId and the multiaddr as seperate properties, so we'd have to join them back up like:

const fullAddr = addr.encapsulate(`/ipfs/${peerId.toB58String()}`)

[lidel]

Something to be aware of: /ipfs/ in libp2p's multiaddrs is being (slowly) replaced with /p2p/. Perhaps the code that does the comparison should support both (to avoid surprises when IPFS switches to /p2p/ notation).

Ref. ipfs/notes#201, multiformats/multiaddr#34, libp2p/specs#45

[olizilla]

For this check, do we want to just compare peerIds? A unique peerId could be flagged as a bootstrap node if it's peer ID matches one of the bootstrap peerIds? We'd have to slice off the peerId from each boostrap multiaddr, but then we'd just been comparing multihashes rather than needing to check for /p2p or /ipfs

[fsdiogo]

This is what I have right now, but it still doesn't seem to match anything:

https://github.com/ipfs-shipyard/ipfs-webui/blob/42170645603bb73cd028f863cbddf9f1c572b8ab/src/bundles/peer-locations.js#L302-L317

[lidel]

I'd go with @olizilla suggestion (#1062 (comment)). Just make sure you do the bootstrap check before the relay check, because bootstraps could also be relays in the future.

[fsdiogo]

I'd go with @olizilla suggestion (#1062 (comment)).

It's not clear to me what the suggestion is.

Is it to slice off the peerId from each of the boostraps and just compare the multihashes?

[olizilla]

@fsdiogo yep that's the suggestion. Grab the peerids for the bootstrap nodes, and compare it with the peerId of the peer.

Of note, I think your proposal could work, but .encapsulate returns a multiaddr, so i think you'd need to call a toString() on the return value.

[fsdiogo]

Of note, I think your proposal could work, but .encapsulate returns a multiaddr, so i think you'd need to call a toString() on the return value.

You are 💯right, calling toString() and then comparing works. Do you think we should keep this version or the one you mentioned before?

[olizilla]

Your version is fine, let's get it mergable!