[metrics-bugs] #21272 [Metrics]: Onionperf deployment

[Tor Bug Tracker & Wiki]

#21272: Onionperf deployment
-------------------------+------------------------------
 Reporter:  hiro         |          Owner:  metrics-team
     Type:  enhancement  |         Status:  needs_review
 Priority:  Medium       |      Milestone:
Component:  Metrics      |        Version:
 Severity:  Normal       |     Resolution:
 Keywords:               |  Actual Points:
Parent ID:               |         Points:
 Reviewer:               |        Sponsor:
-------------------------+------------------------------

Comment (by iwakeh):

 Replying to [comment:26 karsten]:
 > Replying to [comment:25 iwakeh]:
 > > Replying to [comment:23 karsten] and what's left from [comment:18]:
 > > > iwakeh, please find my updated task-21272 branch with some tweaks as
 discussed above.
 > >
 > > Thanks, for these changes!
 > >
 > > Couldn't `downloadFromOnionPerfHost` do some of the filename checking
 before
 > > calling `downloadAndParseOnionPerfTpfFile`?
 >
 > Well, that wouldn't change functionality but would be a simple
 refactoring, right?  What's the goal there?  Make methods more testable or
 easier to read or something else?  In any case, would you want to suggest
 new methods, and I'll move around code?  Or do you want to work on a
 patch?

 No, I intended to avoid the [https://gitweb.torproject.org/karsten
 /metrics-
 db.git/tree/src/main/java/org/torproject/collector/torperf/TorperfDownloader.java?h=task-21272&id=3318eb8ca769392cca1a6ddc3344c43eba62da91#n750
 superfluous parsing] of the URL for each file and avoid download when the
 filename doesn't make sense.
 No refactoring.

 >
 > > About the older code:
 > > Could we just also make the change for #20514 now?
 > > In addition, there are quite a few places where try-with resources
 > > or some of `Files`' methods would prevent unclosed readers/writers.
 > > Still this older code needs even more work, sigh.  Maybe, after
 Amsterdam?
 >
 > No need to spend time on this.  Let's just remove the Torperf code in a
 few weeks when we're certain that the OnionPerfs can take over.

 That's fine, too.

 >
 > > And, regarding the descriptor parsing don't the checks inside
 [https://gitweb.torproject.org/karsten/metrics-
 db.git/tree/src/main/java/org/torproject/collector/torperf/TorperfDownloader.java?h=task-21272&id=3318eb8ca769392cca1a6ddc3344c43eba62da91#n797
 this loop] belong into descriptor/metrics-lib itself?
 > > (that might be a new ticket, though)
 >
 > Well, if we moved that code to metrics-lib, users wouldn't be able to
 read Torperf results from anything else than the originally named .tpf
 file.  We usually avoid dependencies on file names if we can.  This case
 is a bit different, because we're archiving .tpf files, and we should be
 certain that they contain what they say.  I'd say that's specific to the
 CollecTor case though and cannot be generalized in metrics-lib.

 There could be a boolean parameter for strict checking?
 But, I didn't mean to hijack this ticket.  I can just write that down on a
 list on my desk ;-)

 >
 > Note that we could have picked a different approach by parsing
 descriptors from files and appending them to new files with file names
 taken from descriptor contents.  The result might be the exact same
 output, or it could be a file with fewer descriptors or descriptors in a
 different order, etc.  But I felt it's easier to verify .tpf file contents
 and either take them or leave them.

 Hmm, ok, good to know.

 >
 > > I can take a look at the Persistence topic at some point.
 >
 > Sounds good!  The last paragraph above might be relevant here.  Hope
 this works with the persistence classes.

 Yes, I noticed this.  We'll see.

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