[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #21236 [Metrics/Metrics website]: Put a visualization of Tor Browser downloads and updates on the Metrics website



#21236: Put a visualization of Tor Browser downloads and updates on the Metrics
website
-------------------------------------+------------------------------
 Reporter:  karsten                  |          Owner:  karsten
     Type:  enhancement              |         Status:  needs_review
 Priority:  High                     |      Milestone:
Component:  Metrics/Metrics website  |        Version:
 Severity:  Normal                   |     Resolution:
 Keywords:                           |  Actual Points:
Parent ID:                           |         Points:
 Reviewer:                           |        Sponsor:
-------------------------------------+------------------------------

Comment (by karsten):

 Thanks for the quick response!  And thanks a lot for making those build
 improvements.

 I made some tweaks to your commits and pushed them to
 [https://gitweb.torproject.org/karsten/metrics-web.git/log/?h=task-21236-2
 my task-21236-2 branch].  I'd like to squash that commit and yours into my
 initial commit 597e715 at the end, if you don't mind.  Unless there's a
 reason to keep them separate, in which case, please let me know.

 But before we squash or don't squash anything, let's talk about the
 `Database` class.  I like the idea of encapsulating database access---in
 theory.  I even had a similar class like yours a day or two ago.  But I
 took that out again, because I felt it only added complexity for no
 particular gain.  Let me elaborate.

 The `Main` class still imports types from `java.sql`, meaning that the
 `Database` class does not hide that we're talking to an SQL database via
 JDBC.  And the methods in `Database` all contain just a single line of
 code or two, meaning that we're not really hiding much complexity.  The
 reason is that JDBC is already an interface hiding away all the complexity
 that comes with talking to an SQL database.

 So, let's talk about the purpose of having this separate `Database` class.
 I assume the main reason for having it is testing.  Is it going to be
 easier to test the `Database` class than to test the single methods in
 modules that are related to database access?  And maybe even more
 importantly, is it easier to mock the `Database` class than to provide a
 dummy database via a custom JDBC string and just test the methods
 contained in modules that way?  If the answers are "yes and yes", would
 you mind writing a simple test for the new `Database` class, just to get
 us started?

 Otherwise, would you mind if we postpone this change and keep all database
 methods in the `Main` class for now?  It's easy to extract database
 functionality (and downloading functionality or .csv-file writing
 functionality) as part of the restructuring.  Don't get me wrong, I'm a
 supporter of encapsulation if something is fully encapsulated and for a
 good reason.  But I'm also a fan of self-contained and therefore often
 more readable code.  (For example, do you think we'll remember in a year
 when we write the next module that we disable auto-commit when connecting
 to the database?!)

 And here are some minor points that need discussion:

  - If we keep the `Database` class, we should either move it to the
 `org.torproject.metrics.webstats` package, or start a `shared/src/`
 directory for code that is supposed to be shared between modules.  My
 favorite would be to start with the former and consider doing the latter
 once we have a second module using it.

  - Should the arguments `--filter` and `--db` be switched around, so that
 people can specify their own database but omit the filter?  Alternatively,
 we might want to say more clearly how to pass an argument for `--filter`
 that doesn't filter anything.

  - I didn't run this code yet.  Does `writeStatistics()` add a newline
 after writing the header?

  - As you point out, we should write tests.  I'm happy to do that, though
 I'm afraid the most interesting parts require mocking either the web
 server or the database.  If you have suggestions for doing that, please
 let me know.  Otherwise I'd start with some simple tests for
 `parseLogLines()` or `writeStatistics()`.

 And here's another remaining issue from above, listed for completeness:

  - I didn't find time to fix yet is that locale detection is not always as
 precise as it could be. Example from the .csv file:
 `2017-01-10,tmup,o,,0.3.0b1-0.3.0b2-linux32-en-US.xml,,3`

 Thanks again!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/21236#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs