[tor-bugs] #28859 [Metrics/ExoneraTor]: Use Java 8 date-time functionality in ExoneraTor

[Tor Bug Tracker & Wiki]

#28859: Use Java 8 date-time functionality in ExoneraTor
------------------------------------+----------------------
     Reporter:  karsten             |      Owner:  karsten
         Type:  enhancement         |     Status:  assigned
     Priority:  Medium              |  Milestone:
    Component:  Metrics/ExoneraTor  |    Version:
     Severity:  Normal              |   Keywords:
Actual Points:                      |  Parent ID:  #23752
       Points:                      |   Reviewer:
      Sponsor:                      |
------------------------------------+----------------------
 We had plans to use Java 8 date-time functionality in all our code bases
 for quite a while now. I finally picked this up by reading a few
 tutorials, mostly to figure out which of the three types we're going to
 use: `Instant`, `LocalDateTime`, or `ZonedDateTime`. I picked ExoneraTor
 for this discussion/review, because nothing else depends on it and we're
 currently not working on this code base in other tickets.

 So, I think I have a plan now, and I'm presenting it here using snippets
 from my ExoneraTor patch:

 {{{
 @@ -131,10 +132,11 @@ public class ExoneraTorDatabaseImporter {
      try {
        if (lockFile.exists()) {
          BufferedReader br = new BufferedReader(new FileReader(lockFile));
 -        long runStarted = Long.parseLong(br.readLine());
 +        Instant runStarted = Instant.ofEpochMilli(Long.parseLong(
 +            br.readLine()));
          br.close();
 -        if (System.currentTimeMillis() - runStarted
 -            < 6L * 60L * 60L * 1000L) {
 +        if (runStarted.plus(Duration.ofHours(6L))
 +            .compareTo(Instant.now()) >= 0) {
            logger.warn("File 'exonerator-lock' is less than 6 "
                + "hours old.  Exiting.");
            System.exit(1);
 }}}

 This code is used to write the current system time to a local lock file to
 determine when we need to obey that or can safely delete it. Let's ignore
 the design and just look at the date-time part for now.

 So, I think that we can replace code where we used
 `System.currentTimeMillis()` in the past with `Instant`. The concept is
 close enough to the "machine time" concept rather than "human time" that
 `Instant` will work for us. We care about hours here, not things like
 calendar months.

 {{{
 @@ -223,7 +225,8 @@ public class ExoneraTorDatabaseImporter {

    /* Parse a consensus. */
    private static void parseConsensus(RelayNetworkStatusConsensus
 consensus) {
 -    long validAfterMillis = consensus.getValidAfterMillis();
 +    LocalDateTime validAfter =
 LocalDateTime.ofInstant(Instant.ofEpochMilli(
 +        consensus.getValidAfterMillis()), ZoneOffset.UTC);
      for (NetworkStatusEntry entry :
 consensus.getStatusEntries().values()) {
        if (entry.getFlags().contains("Running")) {
          String fingerprintBase64 = null;
 }}}

 Things are a bit different here. The valid-after time in a consensus is
 not just a point on the timeline. It's an actual date and time that has a
 meaning for humans. That somewhat rules out the `Instant`.

 Now, why use `LocalDateTime` here und not `ZonedDateTime`, as suggested
 before? Well, I could imagine doing either, as long as we're consistent
 across our codebases.

 However, one reason that made me pick `LocalDateTime` in the end was the
 comparison with PostgreSQL's `TIMESTAMP [WITHOUT TIMEZONE]` vs. `TIMESTAMP
 WITH TIMEZONE` types. We're importing lots of timestamps into PostgreSQL,
 but we're always using `TIMESTAMP [WITHOUT TIMEZONE]`. Because we know
 that timestamps in Tor are always UTC and we simply don't need timezone
 information. The [https://jdbc.postgresql.org/documentation/head/8-date-
 time.html corresponding types] for `TIMESTAMP [WITHOUT TIMEZONE]` and
 `TIMESTAMP WITH TIMEZONE` are `LocalDateTime` and `OffsetDateTime`, by the
 way.

 The only use case I see where `LocalDateTime` could be problematic is when
 somebody wants to convert our timestamps to their local timezone. But
 that's not a common use case for us, and we're primarily building our
 tools for ourselves and the services we provide.

 {{{
 @@ -248,26 +251,21 @@ public class ExoneraTorDatabaseImporter {
 [...]
      try {
        for (String orAddress : orAddresses) {
          insertStatusentryStatement.clearParameters();
 -        insertStatusentryStatement.setTimestamp(1,
 -            new Timestamp(validAfterMillis), calendarUTC);
 +        insertStatusentryStatement.setObject(1, validAfter);
          insertStatusentryStatement.setString(2, fingerprintBase64);
          if (!orAddress.contains(":")) {
            insertStatusentryStatement.setString(3, orAddress);
 }}}

 This is what it looks like when we're passing a `LocalDateTime` to the
 database.

 {{{
 @@ -47,7 +50,9 @@ public class QueryServlet extends HttpServlet {

    private DataSource ds;

 [...]
 +  private static final DateTimeFormatter validAfterTimeFormatter
 +      = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")
 +      .withZone(ZoneOffset.UTC);

    @Override
    public void init() {
 }}}

 And this is how we're parsing/formatting a Tor date-time. It's not a
 standard format, but we can easily define it once per class or package or
 codebase. Yay, thread safety!

 Okay, these are the highlights. I'll post the branch once I have a ticket
 number.

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