Hi Stephen, Thanks a lot for your feedback, it is really helpful!
On 2/14/15 11:28 AM, Stephen Colebourne wrote:
Elements of the date/time handling here don't really work. Logging is essentially a UTC only problem, using a time-zone is slow and unecessary. This indicates that all uses of ZonedDateTime should be replaced with either Instant or an OffsetDateTime using ZoneOffset.UTC. Any string format should have the ISO-8601 string end with "Z", and not end with "-05:00" or any other zone offset.
If I'm not mistaken the previous SimpleFormatter used to use java.util.Dateand 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?
(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" <[email protected]> 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
