Opened 6 years ago

Closed 6 years ago

#8932 closed enhancement (implemented)

Add support for grouping by network family

Reported by: lunar Owned by: gsathya
Priority: Medium Milestone:
Component: Metrics/Compass Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Tor currently avoid to pick two relays in the same /16 IP network. The attached patches allow Compass to group relays by these network families.

Child Tickets

Attachments (5)

0001-Allow-the-grouping-function-to-return-multiple-group.patch (2.1 KB) - added by lunar 6 years ago.
Allow the grouping function to return multiple groups
0002-Add-support-for-grouping-by-network-family-requires-.patch (4.4 KB) - added by lunar 6 years ago.
Add support for grouping by network family
0001-Add-a-column-with-the-relay-primary-IPv4-address.patch (3.7 KB) - added by lunar 6 years ago.
Add a column with the relay primary (IPv4) address
0002-Refactor-RelayStats._get_group_function.patch (1.5 KB) - added by lunar 6 years ago.
Refactor RelayStats._get_group_function
0003-Add-support-for-grouping-by-network-family.patch (4.4 KB) - added by lunar 6 years ago.
Add support for grouping by network family

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by lunar

Allow the grouping function to return multiple groups

Changed 6 years ago by lunar

Add support for grouping by network family

comment:1 Changed 6 years ago by lunar

Status: newneeds_review

comment:2 Changed 6 years ago by karsten

Status: needs_reviewneeds_revision

I like the idea. But the patches need some tweaking:

  • When I try out the -N option without the ipaddr module, I get: NameError: name 'LoadError' is not defined
  • Nickname is not the right column to put in the /16 network. Should we add an IP address column? It could by default list the IP address that the relay used to register in the network, that is, the first entry in or_addresses. (Speaking of, I should guarantee in the Onionoo protocol specification that the first entry is always that primary IP address, not some other additional OR address. If you think that makes sense, I'll make the change.)
  • The -N option doesn't work together with the -A and/or -C options. Should these combinations be allowed? If not, their combined usage should be prohibited. It would be easier though to allow these combinations, also with respect to the web version.
  • We should only look at the primary IP address of a relay for /16 comparison. That's what tor does, too. Also, once a relay is in more than one /16 network, it would be contained in more than one group, which is not allowed. The effect would be that Compass shows in total more relays, more CW, more adv_bw, etc. than there are in the network. This means that your first patch is not required, because the grouping function can only return a single group per relay. For example, if the -N option is used together with -A and -C, that group would be a tuple ('AS 123', 'DE', '4.5.0.0/16').

Do these suggestions make sense to you? Do you want to tweak the patch?

Changed 6 years ago by lunar

Add a column with the relay primary (IPv4) address

Changed 6 years ago by lunar

Refactor RelayStats._get_group_function

Changed 6 years ago by lunar

Add support for grouping by network family

comment:3 Changed 6 years ago by lunar

Status: needs_revisionneeds_review

Here's another try following your advices. I've removed the dependency on ipaddr.

comment:4 Changed 6 years ago by karsten

Resolution: implemented
Status: needs_reviewclosed

Tweaked, merged, and deployed. Thanks! Closing.

Note: See TracTickets for help on using tickets.