In LogRecord, the Javadoc of getInstant(): "Get event time." could be "Gets the instant that the event ocurred."
In LogRecord, the Javadoc of setInstant(): "Set event time." could be "Sets the instant that the event ocurred." (matching change to @param) I'd prefer to see the other methods in the class have their Javadoc changed from "Get" to Gets" and from "Set" to "Sets" to conform with Javadoc norms, but understand if this causes CCC issues. In PlatformLogger. There should be no need to assign to a static constant: private static final Clock systemUTC = Clock.systemUTC(); However, Clock.systemUTC() needs to be fixed: https://bugs.openjdk.java.net/browse/JDK-8073394 This ZonedDateTime zdt = ZonedDateTime.ofInstant(systemUTC.instant(), zoneId); can be replaced with ZonedDateTime zdt = ZonedDateTime.now(zoneId); Again, the implementation of the better method isn't perfectly optimised. The default format used in String.format does not need to time-zone, just the local date/time. As such, in theory a LocalDateTime could be used. But, since the format can be changed by the user, a ZonedDateTime is needed (probably not worth being clever here). Where you assign ZoneId.systemDefault() to a final instance variable, it is being locked for the lifetime of that object. I cannot immediately tell what the lifetime of the objects are. I also suspect that few if any will want to change their time-zone half way through logging. XMLFormatter has a reviewer's comment that will need to be removed before committing. In LogRecordWithNanosAPI you check the instant contents rather than just the instant: assertEquals(record.getInstant().getEpochSecond(), record2.getInstant().getEpochSecond(), "getInstant().getEpochSecond()"); assertEquals(record.getInstant().getNano(), record2.getInstant().getNano(), "getInstant().getNano()"); assertEquals(record.getInstant().toEpochMilli(), record2.getInstant().toEpochMilli(), "getInstant().toEpochMilli()"); could be assertEquals(record.getInstant(), record2.getInstant(), "getInstant()"); There are probably other instances of this point. Hope that helps Stephen On 17 February 2015 at 19:33, Daniel Fuchs <daniel.fu...@oracle.com> wrote: > Hi, > > Here is a new webrev - which should contain all the feedback received > so far: > > #1 LogRecord uses serialPersistentFields for serialization, and > contains an Instant instead of the two fields millis + > nanoAdjustment. > #2 getMillis/setMillis are deprecated, replaced by getInstant/setInstant > getNanoAdjustment/setNanoAdjustment have been dropped. > > [Thanks Peter for prototyping these 2 first changes!] > > #3 XMLFormatter uses ISO_INSTANT to print the instant in the date field. > new constructor has been dropped. printNanos property is renamed into > useInstant. > #4 SimpleFormatter still uses ZonedDateTime - for compatibility and > flexibility and because it proved to be faster than Date [1]. > #5 Tests have been updated WRT to the changes above. > #6 various doc tweaks compared to last webrev > > new webrev: > http://cr.openjdk.java.net/~dfuchs/webrev_8072645/webrev.01/ > > specdiff updated in place (unfortunately the serial form does > not show up): > http://cr.openjdk.java.net/~dfuchs/webrev_8072645/specdiff-logging-time/overview-summary.html > > best regards, > > -- daniel > > [1] benchmarks related to formatting the date: > http://cr.openjdk.java.net/~dfuchs/webrev_8072645/benchmarks.html > > > > On 16/02/15 20:24, Daniel Fuchs wrote: >> >> Hi Stephen, >> >> Thanks again for your support and suggestions! >> >> On 14/02/15 14:03, Daniel Fuchs wrote: >>> >>> If I'm not mistaken the previous SimpleFormatter used to use >>> java.util.Date >>> and printed the time in the local time zone. I have tried to keep this >>> behavior. >>> I'm not sure we would want to change it to print the time in the UTC >>> time zone >>> by default. A lot of developers use logging for debugging - and when >>> reading >>> debug messages on the console I usually prefer to see the time in my own >>> time zone. >>> >>> Would there be a more efficient way to keep the default formatting of >>> the time >>> in the SimpleFormatter? >> >> >> I spent part of the day reading the documentation & trying out the >> various TemporalAccessors and DateTimeFormatters... >> >> I have also done some microbenchmark measurements (jmh) WRT >> the performance of formatting a date/time. >> Results can be seen here [1]: >> http://cr.openjdk.java.net/~dfuchs/webrev_8072645/benchmarks.html >> >> What it shows is that when using String.format (as the SimpleFormatter >> is specified to do), then using ZonedDateTime is actually an improvement >> over using Date. >> >> Now that I have read a bit more about LocalDateTime, ZonedDateTime, >> Date, and Instant, I do agree with you that for recording an event time, >> printing the Instant is the better alternative. >> However, since SimpleFormatter has always printed the local date, >> I would tend to want to keep it that way. >> >> So I'm going to propose to keep using ZonedDateTime in >> the SimpleFormatter, as I believe it's what gives it the maximum of >> flexibility - WRT to using String.format (+ we will get some >> boost as bonus by no longer using Date). >> If you strongly feel that java.util.logging should offer a better >> alternative - then maybe we should log an RFE to explore it? >> >> Things are - I believe - a bit different for the XMLFormatter. >> First, the XMLFormatter is not specified to use String.format, so >> it gives us a bit more flexibility. >> In addition we already need to tweak the format in order to introduce >> the new <nanos> element - (or should it be <nanoOfMilli> as Peter >> suggested?). >> >> So for the XMLFormatter, and given the results of my >> microbenchmark [1], here is what I would suggest: >> >> #1 rename the property printNanos into useInstant >> #2 if useInstant is false, use the old code for formatting the >> date (the old appendISO8601() method) and omit the <nanos> >> element - so applications that specify useInstant=false should >> see the same formatting than before, without paying the >> performance cost that using ZonedDateTime would bring. >> #3 if useInstant is absent - or not false, then we use the 'new' >> format. The <date> element will contain the instant printed >> using DateTimeFormatter.ISO_INSTANT, and the <nanos> element >> will be printed after <millis> >> >> Does that sound right? If so I will include these changes in my >> next webrev, once I have digested the feedback sent by Peter :-) >> >> Best regards, >> >> -- daniel >> >> [1] microbenchmark: >> http://cr.openjdk.java.net/~dfuchs/webrev_8072645/benchmarks.html >> >> >>> >>>> (The webrev is broken wrt zones as it stores ZoneId.systemDefault() in a >>>> static constant, but that method depends on the mutable >>>> TimeZone.getDefault() ). >>> >>> >>> Would making it a final (non static) variable be better? >>> I now wonder whether there should be a way to configure the time zone for >>> an instance of SimpleFormatter (something like what I did with >>> 'printNanos' >>> for the XMLFormatter). >>> >>>> LogReecord.getInstantUTC() should be getInstant(). >>>> >>>> (An Instant is fully defined as a concept, and it cannot be in or not in >>>> UTC). >>> >>> >>> Right. Thanks for pointing that out. >>>> >>>> In SimpleFormatter, you have {@code java.util.loggin} (missing a "g"). >>> >>> >>> Good catch. >>> >>>> In XMLFormatter, instead of using DateTimeFormatter.ISO_LOCAL_DATE_TIME, >>>> create a new instance of DateTimeFormatter that does not output the >>>> fraction of a second. That way, there is no need to use >>>> truncatedTo(SECONDS). >>>> >>>> StringBuilder appends can be used directly with formatters: >>>> >>>> sb.append(ZonedDateTime.ofInstant(time, zoneId).format(dtformatter)); >>>> >>>> replace with >>>> >>>> dtformatter.formatTo(ZonedDateTime.ofInstant(time, zoneId), sb); >>> >>> >>> Will look into this. >>> >>> Thanks again for your review! This is quite helpful. >>> >>> -- daniel >>> >>> >>>> >>>> thanks >>>> Stephen >>>> >>>> >>>> >>>> On 13 Feb 2015 14:57, "Daniel Fuchs" <daniel.fu...@oracle.com> wrote: >>>> >>>>> Hi, >>>>> >>>>> Please find below a patch for: >>>>> >>>>> 8072645: java.util.logging should use java.time to get more >>>>> precise time stamps >>>>> >>>>> >>>>> http://cr.openjdk.java.net/~dfuchs/webrev_8072645/webrev.00/ >>>>> >>>>> specdiff: >>>>> http://cr.openjdk.java.net/~dfuchs/webrev_8072645/ >>>>> specdiff-logging-time/java/util/logging/package-summary.html >>>>> >>>>> Overview: >>>>> --------- >>>>> >>>>> The patch is made of the following pieces: >>>>> >>>>> - LogRecord uses java.time.Clock's systemClock to get an >>>>> Instant in the best available resolution. >>>>> >>>>> The instant is split into a number of milliseconds (a long) >>>>> and a nanosecond adjustment (an int). >>>>> The number of milliseconds is the same than what would have >>>>> been obtained by calling System.currentTimeMillis(). >>>>> >>>>> - LogRecord acquires a new serializable int nanoAdjustement field, >>>>> which can be used together with the number of milliseconds >>>>> to reconstruct the instant. >>>>> >>>>> - SimpleFormatter is updated to pass a ZoneDateTime >>>>> instance to String.format, instead of a Date. >>>>> >>>>> The effect of that is that the format string can now >>>>> be configure to print the full instant precision, if >>>>> needed. >>>>> >>>>> - XMLformatter will add a new <nanos> element after the >>>>> <millis> element - if the value of the nanoAdjustment >>>>> field is not 0. >>>>> >>>>> The <date> string will also contain the nano second >>>>> adjustment as well as the zone offset as formatted by >>>>> DateTimeFormatter.ISO_OFFSET_DATE_TIME >>>>> >>>>> Compatibility considerations: >>>>> ----------------------------- >>>>> >>>>> - The serial for of log record is backward/forward compatible. >>>>> I added a test to verify that. >>>>> >>>>> - XMLFormatter has acquired a new configurable property >>>>> '<FQCN>.printNanos' which allows to revert to the old >>>>> XML format, should the new format cause issues in >>>>> existing applications. >>>>> >>>>> - The logger.dtd will need to be updated, to support the >>>>> new optional <nanos> element. And for this matter, >>>>> should we update the logger.dtd or rather define a >>>>> logger-v2.dtd? >>>>> See planned modification: >>>>> >>>>> <http://cr.openjdk.java.net/~dfuchs/webrev_8072645/logger- >>>>> dtd/logger.dtd.frames.html> >>>>> >>>>> best regards, >>>>> >>>>> -- daniel >>>>> >>> >> >
