[metrics-bugs] #19640 [Metrics/metrics-lib]: review and improve interface hierarchy

[Tor Bug Tracker & Wiki]

#19640: review and improve interface hierarchy
---------------------------------+-----------------------------------
 Reporter:  iwakeh               |          Owner:  metrics-team
     Type:  enhancement          |         Status:  assigned
 Priority:  Medium               |      Milestone:  metrics-lib 2.0.0
Component:  Metrics/metrics-lib  |        Version:
 Severity:  Normal               |     Resolution:
 Keywords:                       |  Actual Points:
Parent ID:                       |         Points:
 Reviewer:                       |        Sponsor:
---------------------------------+-----------------------------------
Changes (by karsten):

 * owner:  karsten => metrics-team
 * status:  new => assigned


Comment:

 I'd like us to flesh out how we'd like to improve the interface hierarchy,
 and I'll start by going through the ideas above.

  1. Comment 10 of #19398 says: "There shouldn't be two interfaces
 declaring the very same method; there should be a more structured
 hierarchy for the interfaces, too."  I think I agree in theory, though I'd
 want to discuss how strictly we want to implement this principle.  Here's
 a contrasting principle: let's avoid introducing interfaces that have the
 sole purpose of deduplicating methods.  I think we need to find a middle
 ground here.

  2. The linked comment in `NetworkStatusImpl` seems related to possible
 improvements to the interface hierarchy, though I'd prefer if we address
 that issue after improving the interface hierarchy.

  3. Comment 4 above, splitting `ParseHelper` into interface and
 implementation seems unrelated, unless we're planning to make the
 interface part of the metrics-lib API.  Right now it's solely an
 implementation helper to avoid duplicating code.

  4. #17861 is indeed something where we could improve the interface
 hierarchy, by having a separate interface for each descriptor type.  But
 I'm not sure how to best implement such a new interface.  See the heavy
 overlap between `RelayNetworkStatusVote` and
 `RelayNetworkStatusConsensus`.  A new
 `RelayNetworkStatusMicrodescConsensus` interface following the current
 code practice would basically copy over the entire consensus interface and
 change a method here and there.  We should avoid that.

 In addition to those items, I came up with a few more ideas:

  5. When we added `RelayServerDescriptor`, `RelayExtraInfoDescriptor`,
 `BridgeServerDescriptor`, and `BridgeExtraInfoDescriptor`, we left all
 methods in the superinterfaces `ServerDescriptor` and
 `ExtraInfoDescriptor`.  We should consider moving methods that are
 specific to relays or bridges to the subinterfaces.

  6. We're still using a single `NetworkStatusEntry` for entries in all the
 different network statuses.  We should consider using an interface
 hierarchy there, too, to only provide relevant methods depending on the
 network status at hand.  I started working on a patch, but I'd like to
 keep this discussion on the conceptual level for now.

 And here are some more concrete suggestions for improving the interface
 hierarchy, somewhat overlapping with the ideas above:

  7. Introduce a `NetworkStatus` interface to capture all common parts of
 network statuses in directory protocol versions 2 and 3, including all
 `RelayNetworkStatus*` and `BridgeNetworkStatus`.

  8. Introduce another interface called `NetworkStatusVote` for common
 parts of network statuses in directory protocol version 3, following the
 common part in URLs like `http://<hostname>/tor/status-
 vote/next/authority.z`.  Though there's the risk that this interface will
 be confused with `RelayNetworkStatusVote`.  Another possible interface
 name could be `RelayNetworkStatusVersion3`, though I'm not a big fan of
 including numbers in type names.

  9. Introduce a new interface `NetworkStatus.Entry` just like
 `ExitList.Entry` and a set of subinterfaces like
 `RelayNetworkStatusConsensus.Entry` to address issue 6 above.  Requires
 some heavy generics lifting.

  10. Use `RelayNetworkStatusConsensus` for the common parts in consensuses
 of any flavor and introduce `RelayNetworkStatusNsConsensus` for unflavored
 /ns-flavored and `RelayNetworkStatusMicrodescConsensus` for microdesc-
 flavored consensuses.  That's basically what we did with adding new
 subinterfaces to `ServerDescriptor`.

 What else can/should we do?  Or which parts should we rather not do?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/19640#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online