sentry-java

sentry-java copied to clipboard

Hi,

We are running the 3.2.0 logback appender and have noticed the performance discussed in https://github.com/getsentry/sentry-java/issues/1081. When looking at the improvement that was made in https://github.com/getsentry/sentry-java/pull/1111 it seems that it can be improved even further:

The current implementation instantiates a Calendar just to call Calender.getTime(). Looking at the openjdk implementation of Calendar.getTime(), this is equivalent to just constructing a Date directly from UTC millis - new Date(millis). Could the relatively expensive calendar be eliminated?

I don't understand this comment https://github.com/getsentry/sentry-java/issues/1081#issuecomment-742027482 as java.util.Date will always, well at least on openjdk, represent its internal UTC timestamp in the system default timezone. The following snippet

    public static void main(String[] args) {
        Date currentDateTime = io.sentry.DateUtils.getCurrentDateTime();
        Date dateTime = io.sentry.DateUtils.getDateTime(System.currentTimeMillis());
        System.out.println(currentDateTime.getTimezoneOffset());
        System.out.println(currentDateTime);
        System.out.println(dateTime.getTimezoneOffset());
        System.out.println(dateTime);
    }

outputs

-60
Fri Mar 12 16:55:22 CET 2021
-60
Fri Mar 12 16:55:22 CET 2021

on my system. Might there be some confusion about UTC vs GMT?

@jan-berge-ommedal we always get the Calendar using a TimeZone.UTC instead of Device's TimeZone, that does the trick. Are you experiencing slowness because of that or just trying to improve things? without that, it won't get the correct UTC date and time

@marandaneto just trying to improve things, we haven't experienced performance issues with the latest version so far.

I am not still not convinced that a TimeZone.UTC actually does the trick when the calendar is converted to a java.util.Date. Here is the internal state of one of the dates in the snippet above:

dateTime = {Date@2231} "Fri Mar 19 13:12:47 CET 2021"
 fastTime = 1616155967484
 cdate = {Gregorian$Date@2247} "2021-03-19T13:12:47.484+0100"
  cachedYear = 2021
  cachedFixedDateJan1 = 737791
  cachedFixedDateNextJan1 = 738156
  era = null
  year = 2021
  month = 3
  dayOfMonth = 19
  dayOfWeek = 6
  leapYear = false
  hours = 13
  minutes = 12
  seconds = 47
  millis = 484
  fraction = 47567484
  normalized = true
  zoneinfo = {ZoneInfo@2249} "sun.util.calendar.ZoneInfo[id="Europe/Oslo",offset=3600000,dstSavings=3600000,useDaylight=true,transitions=141,lastRule=java.util.SimpleTimeZone[id=Europe/Oslo,offset=3600000,dstSavings=3600000,useDaylight=true,startYear=0,startMode=2,startMonth=2,startDay=-1,startDayOfWeek=1,startTime=3600000,startTimeMode=2,endMode=2,endMonth=9,endDay=-1,endDayOfWeek=1,endTime=3600000,endTimeMode=2]]"
  zoneOffset = 3600000
  daylightSaving = 0
  forceStandardTime = false
  locale = null

This date has a correct UTC timestamp similarly to what you get by just new Date(millis). Its timezone is Europe/Oslo, not GMT

@jan-berge-ommedal when I tested that, it failed on Android, also, we don't use the millis always but sometimes the ISO format, that was the only way it worked properly, I'd need to double-check that maybe.