Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-63437

Checks timestamps sent in local timezone instead of UTC

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      According to Gerrit REST API protocol, timestamps are in UTC.

      In America/New_York timezone for example, if the check starts at 13:00, it sends the timestamp to Gerrit as "<date> 13:00:00", which Gerrit interprets as UTC. It should be sending "17:00" in UTC, so that the web UI displays it in the correct local timezone. Instead, I see "9:00 AM" when it should be "1:00 PM".

      I think it might be because the plugin is using java.sql.Timestamp, which subclasses java.util.Date, which tries to recalculate the timestamp with local timezone taken into account in normalize(). I haven't dug into it yet.

      Jenkins, Gerrit, and local workstation are all in America/New_York timezone. Gerrit reports correct timezones in the Comments section, and also in the log files. Jenkins reports the correct timezones as well. The only part that's wrong is the Checks panel's timestamps.

      Looking at the REST API responses in the Chrome console, I see that comments are coming back in UTC (i.e., "17:00" UTC for 1PM local time); but the Checks REST API responses are showing timestamps with local time presentation, when they should be UTC times. That is what causes the UI to show the times as 9AM instead of 1PM.

        Attachments

          Activity

          Hide
          jhansche Joe Hansche added a comment -

          I verified this with a scratch file:

          class Scratch {
              public static void main(String[] args) {
                  // Current time is 6:14 PM EDT
                  System.out.println(new java.sql.Timestamp(System.currentTimeMillis()));
                  // Expected: 2020-08-18 22:14:45.024
                  //   Actual: 2020-08-18 18:14:45.024
              }
          }
          
          Show
          jhansche Joe Hansche added a comment - I verified this with a scratch file: class Scratch { public static void main( String [] args) { // Current time is 6:14 PM EDT System .out.println( new java.sql.Timestamp( System .currentTimeMillis())); // Expected: 2020-08-18 22:14:45.024 // Actual: 2020-08-18 18:14:45.024 } }
          Hide
          jhansche Joe Hansche added a comment - - edited

          A change was submitted for review: https://review.gerrithub.io/c/jenkinsci/gerrit-code-review-plugin/+/513811

          The problem stems from Timestamp subclassing Date, which attempts to normalize the representation into local timezone. We're also using Gson's built-in Date/Timestamp type adapter, which also assumes that it should serialize date objects into local timezone. There is no way to change that behavior in Gson directly, so the solution is to register our own Gerrit-compatible TypeAdapter that serializes the Timestamp object to Gerrit-compatible format (like ISO-8601, but no 'T', no 'Z', and no timezone offset), in UTC timezone.

          The unit test was the hardest part about this: the current tests don't do anything to validate the format that actually gets sent over the wire, and they only set some fields but not others. Using the MockServerClient, there are two approaches we could take when stubbing the endpoints with getClient().when():

          1. Register the URL, method, and/or full expected request body as the stub
            • This means if anything was unexpected in one of those, the test fails with "404" instead of explaining what was missing or unexpected
          2. Register just the URL, or URL+method, but don't set expectations on the body; and instead validate the body using getClient().verify()
            • This way, if a request matches the URL and method, but the body was unexpected, the test failure explains the expected/actual differences, making it easier to identify what went wrong.

          However, I wasn't able to get JsonBody to work with verify() #2; and since we aren't mocking the system clock, I also couldn't set the expected request body with #1 because I couldn't predict System.currentTimeMillis(). Instead, I added a way to extract the full request body from the MockServerClient, parse it as JSON, and extract the field I wanted to check. Then because of the unpredictable time, I could only verify the format via simple regex, but then I parse that string back to a long (epoch seconds) and compare that to the current time. Before the fix, the parsed time would be Z-hours off from current time (where Z = timezone offset).

          Unfortunately, that means the test could still produce a false-pass, if it is executed on a JVM that is set to UTC time (i.e., my change from Patchset 1 which contains only the test). This could be fixed by setting the timezone before starting the test, but I was trying not to spend too much time changing how the whole jenkins environment is configured, and I'm also not entirely sure the "best way" to set the Jenkins timezone in a unit test.

          If I were going to make more changes here, I would:

          1. Add the ability to inject the system clock used at GerritCheckStep.setCheckTimestamps, so that we can mock it in the unit tests, and verify the exact JSON timestamp output
          2. Configure/mock the timezone that will be inferred when JenkinsRule starts up, so we can verify that a non-UTC server will send timestamps in UTC.
          3. Maybe a better way to use the mock API endpoint, so that request inputs are easier to verify.
          4. Consider moving away from java.sql.Timestamp (both here and in the gerrit-checks plugin source), and use something easier to work with; like maybe Instant, ZonedDateTime, or Calendar.
          Show
          jhansche Joe Hansche added a comment - - edited A change was submitted for review: https://review.gerrithub.io/c/jenkinsci/gerrit-code-review-plugin/+/513811 The problem stems from Timestamp subclassing Date, which attempts to normalize the representation into local timezone. We're also using Gson's built-in Date/Timestamp type adapter, which also assumes that it should serialize date objects into local timezone. There is no way to change that behavior in Gson directly, so the solution is to register our own Gerrit-compatible TypeAdapter that serializes the Timestamp object to Gerrit-compatible format (like ISO-8601, but no 'T', no 'Z', and no timezone offset), in UTC timezone. The unit test was the hardest part about this: the current tests don't do anything to validate the format that actually gets sent over the wire, and they only set some fields but not others. Using the MockServerClient, there are two approaches we could take when stubbing the endpoints with getClient().when(): Register the URL, method, and/or full expected request body as the stub This means if anything was unexpected in one of those, the test fails with "404" instead of explaining what was missing or unexpected Register just the URL, or URL+method, but don't set expectations on the body; and instead validate the body using getClient().verify() This way, if a request matches the URL and method, but the body was unexpected, the test failure explains the expected/actual differences, making it easier to identify what went wrong. However, I wasn't able to get JsonBody to work with verify() #2; and since we aren't mocking the system clock, I also couldn't set the expected request body with #1 because I couldn't predict System.currentTimeMillis(). Instead, I added a way to extract the full request body from the MockServerClient, parse it as JSON, and extract the field I wanted to check. Then because of the unpredictable time, I could only verify the format via simple regex, but then I parse that string back to a long (epoch seconds) and compare that to the current time. Before the fix, the parsed time would be Z-hours off from current time (where Z = timezone offset). Unfortunately, that means the test could still produce a false-pass, if it is executed on a JVM that is set to UTC time (i.e., my change from Patchset 1 which contains only the test). This could be fixed by setting the timezone before starting the test, but I was trying not to spend too much time changing how the whole jenkins environment is configured, and I'm also not entirely sure the "best way" to set the Jenkins timezone in a unit test. If I were going to make more changes here, I would: Add the ability to inject the system clock used at GerritCheckStep.setCheckTimestamps , so that we can mock it in the unit tests, and verify the exact JSON timestamp output Configure/mock the timezone that will be inferred when JenkinsRule starts up, so we can verify that a non-UTC server will send timestamps in UTC. Maybe a better way to use the mock API endpoint, so that request inputs are easier to verify. Consider moving away from java.sql.Timestamp (both here and in the gerrit-checks plugin source), and use something easier to work with; like maybe Instant, ZonedDateTime, or Calendar.

            People

            Assignee:
            lucamilanesio Luca Domenico Milanesio
            Reporter:
            jhansche Joe Hansche
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: