[tor-bugs] #16596 [Metrics/ExoneraTor]: Change database queries towards making only a single query per request

[Tor Bug Tracker & Wiki]

#16596: Change database queries towards making only a single query per request
--------------------------------+--------------------------------
 Reporter:  karsten             |          Owner:  karsten
     Type:  enhancement         |         Status:  needs_revision
 Priority:  Medium              |      Milestone:
Component:  Metrics/ExoneraTor  |        Version:
 Severity:  Normal              |     Resolution:
 Keywords:                      |  Actual Points:
Parent ID:                      |         Points:
 Reviewer:                      |        Sponsor:
--------------------------------+--------------------------------

Comment (by karsten):

 Replying to [comment:6 iwakeh]:

 Thanks for starting this review!

 > Already found a bit.  There are also a few things that are part of other
 tickets (#19623, #19624, #21145), which I tried to omit here.

 Okay.

 > ExoneraTorServlet: `exoneratorHost` should not be configured via
 web.xml, rather use simple java properties file or even simpler hard code.
 After all it shouldn't change that often, should it?

 Hmm, right. My idea was that we'll later copy over this servlet to
 metrics-web where it would make a little more sense to make the host name
 configurable. But on second thought that's not really the case. I'll just
 hard-code it.

 > In 'step 3' I see some problems with `null` values, for example,
 `"".equals(null)` would evaluate to false (line 147 following).

 Note that we're using `null` for invalid parameter values and `""` for
 empty parameter values. Do you see actual problems or potential problems
 there?

 In the latter case (potential problems), maybe we can resolve them by
 documenting things a bit better, or by making things clearer in subsequent
 commits.

 In the former case (actual problems), let's of course fix the issues now.
 But I'd have to see the actual problem first.

 > For most exceptions caught the error message should be logged; and, it
 might be time to switch to slf4j-api and logback, now.

 Yes, we can switch to slf4j, though we should do that in a separate commit
 on top of these, probably in a new ticket.

 > In addition, reading the response query should also catch
 RuntimeExceptions (possibly from Gson).

 Ugh, indeed. Will fix. Good '''catch'''. :)

 > The version received should also be logged, if it differs from the known
 version.

 Yep, good idea.

 > The known version(s) could be a constant in `ExoneraTorServlet`; either
 just the major part or all.  This makes it more obvious.

 Also a good idea.

 > QueryServlet: Helper methods ` private String parseTimestampParameter`
 and `private String parseIpParameter` should be `public static` and tests
 should be added.  Similarly, `private String convertIpV*ToHex`, which
 could be combined by simply adding another argument, i.e., `public static
 String convertIpV4ToHex(String relayIp, boolean isIpV4)`.

 Agreed, though let's save that for another ticket and not overload this
 ticket with too many improvements all at once. I know it's tempting. Stay
 strong, we'll eventually fix these things.

 > A `MILLISEC_IN_DAY = 24L * 60L * 60L * 1000L` constant would be useful.

 Agreed. That's probably simple enough for a separate commit on top of this
 branch.

 > Other: Using the object `Boolean exit` for a triplet state is a bit too
 much of a hack.  There could be a simple enum, as simple as, for example,
 `public enum Exit { U, Y, N; }`.

 Ugh, that would turn the JSON field into a string, which doesn't seem very
 intuitive for "is an exit". And whoever processes this JSON will need to
 check for `null` anyway, regardless of boolean or string. In fact, I think
 that we're using the boolean field exactly in the way it's supposed to be
 used: `true` means "is an exit", `false` means "is not an exit", and
 `null` means "we don't know". I think I'd like to leave this one
 unchanged. It's not a hack.

 > Regarding SQL:
 > Order-by statement should be using names not numbers.

 Hmm, now that you mention that, I wonder if we even need results to be
 sorted at all. I'll try to take that out. Otherwise I'd change numbers to
 names in a subsequent commit, because it's not directly related to this
 ticket.

 > Couldn't the exit-information be part of the query already?

 Yes, but I'd like to save database changes for a later ticket. There's so,
 so much more we can do to make the database schema more efficient. (I'd
 have to look at my notes from last year, but I believe that we're storing
 exit information directly in the database rather than the entire raw
 status entry.)

 > (I could also look into providing some patches, if we agree on the
 changes and you don't have the time?)

 I'll try to make changes tomorrow morning. Thanks again for the initial
 review!

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