[tor-bugs] #21759 [Metrics/CollecTor]: Add persistence for torperf/onionperf

[Tor Bug Tracker & Wiki]

#21759: Add persistence for torperf/onionperf
-------------------------------+---------------------------------
 Reporter:  iwakeh             |          Owner:  iwakeh
     Type:  enhancement        |         Status:  needs_revision
 Priority:  Medium             |      Milestone:  CollecTor 1.3.0
Component:  Metrics/CollecTor  |        Version:
 Severity:  Normal             |     Resolution:
 Keywords:                     |  Actual Points:
Parent ID:                     |         Points:
 Reviewer:                     |        Sponsor:
-------------------------------+---------------------------------

Comment (by iwakeh):

 Replying to [comment:8 karsten]:
 > It took a bit longer to respond to your comment 5, but here it comes:

 Thanks for reviewing!

 >  - The following is entirely unrelated to the given patch, but can you
 try to change your Git commit messages to follow the 50/72 rule? That is,
 the first line has no more than 50 characters (unless that's just too
 hard, in which case 60 are still okay), the next line is blank, and all
 remaining lines are wrapped at 72 characters with newlines added between
 paragraphs. And can you write the first line using the imperative, as in
 "Add sync functionality for tpf files." rather than "Implements XY"?
 Again, this is unrelated to this specific commit, but just like we're
 trying to use a common code style we should aim for using a common commit
 message style. Sorry for the nitpick. :)

 No nitpicking; we even have that
 [https://trac.torproject.org/projects/tor/wiki/org/teams/MetricsTeam/ContributorGuide#PatchFormat
 documented].  Will adapt the commits.

 >  - Can you add a change log entry for this change?

 Sure.

 >  - Should we put out a metrics-lib 2.1.0 first towards the end of the
 month, so that we don't have to require a -dev version in the next
 released CollecTor?

 Agreed, the new CollecTor release should only use release versions of
 metrics-lib.
 Wouldn't the next be 2.0.1 for the tiny fix?  Are there more things we
 could release soon?
 Maybe continue the metrics-lib release discussion on #22912 or via mail as
 it feels out of place here?

 >  - Can we keep the old capitalization of OnionPerf in the configs? It
 seems that's what Rob used. It also means fewer changes to the code.

 Well, in general I'd prefer:
 1. consistent naming and capitalization of classes and configuration
 (i.e., OnionperfDownloader.class might need to be renamed too, if
 'OnionPerf*' is chosen.)
 2. rather less capitalization, e.g., prefer 'Exitlist' over 'ExitList'

 Maybe, there are more rules for naming.

 >  - Typo: "Instantiate", not "Instanciate".
 >  - The line `byte[] db = desc.getRawDescriptorBytes();` in
 `SyncPersistence` is probably still left over from testing. This method
 has become really expensive, so we should be careful not to call it more
 often than necessary.

 Get to these two in a new patch when the final topic is addressed.

 >  - I'd like to avoid that we handle descriptors differently depending on
 whether we download them from an OnionPerf host vs. synchronize them from
 a CollecTor host. And I believe that the approach taken in this
 synchronization patch makes more sense than the strict validation approach
 taken in the download part.

 Yes, I agree the simpler version will be better.

 > But maybe we can first take a step back and look at all the other
 modules and find a common approach for all or at least most of them. Let's
 have this discussion first and then merge this patch if it still does what
 we think should be done. Does that make sense?

 Totally, I'll survey that question as next step before any other work
 here.


 > Thanks!

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