Opened 16 years ago

Closed 14 years ago

#867 enhancement closed fixed (fixed)

Improvements to logging timestamp

Reported by: itamarst Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone, itamarst, anthony, jknight, therve, Stephen Thorne Branch:
Author:

Description


Attachments (1)

867_log.diff (3.1 KB) - added by therve 14 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 16 years ago by itamarst

Anthony wanted to add seconds timestamps, and I agree this is a good idea. His
suggestion was using ISO 8601 timestamps as a standard format
(http://www.cl.cam.ac.uk/~mgk25/iso-time.html).

With seconds and local timezone, format would be:
2005-01-20T11:30:20-0500

For comparision, current format is:
2005/01/20 11:32 EST
or sometimes (on Windows):
2005/01/20 11:32 Eastern Standard Time

so this would be 4 extra characters per message (on non-broken platforms).
Simply adding seconds to current format is 3 extra characters, so I think that's
reasonable.

This solves: 
1. Lack of resolution in current format.
2. Really really really long timezone names on Windows.
3. Ambiguity of timezone regarding daylight savings time.

comment:2 Changed 16 years ago by itamarst

There's code for doing ISO 8601 in xml.utils.iso8601. This is going to be sucky
and slow, I can tell already, until we make twisted have efficient "get the
current second" code  or something :(

comment:3 Changed 16 years ago by jknight

1) ISO8601 format is usually spelled without a T in the middle when meant 
for human readability. That is, an ISO8601 date and an ISO8601 time, 
separated by a space, instead of an ISO8601 combined datetime. 
"2005-01-20 11:30:20-0500".

2) It's too bad %z in strftime isn't standard (or else that python doesn't 
just implement its own strftime instead of using the OS's), because then a 
good format would be "%Y-%m-%D %H:%M:%S%z", and would allow the code and 
behavior to not be changed, just the default format string.

comment:4 Changed 14 years ago by therve

Cc: therve added
Keywords: review added

It seems the timezone problem has already been fixed. Attached a patch adding seconds and changing date separators to '-'.

comment:5 Changed 14 years ago by Jean-Paul Calderone

Still dislike this solution. The real problem is that setting the format from outside Twisted is hard. That's what should be fixed.

comment:6 Changed 14 years ago by therve

I can't agree more. But I thought it was beyond the scope of this ticket, which just talks about the default format used.

The best way for me to customize the format is to subclass FileLogObserver and to make t.p.log uses it: very easy if you don't use twistd. The real problem is setting the format inside twistd :).

comment:7 Changed 14 years ago by Jonathan Lange

Priority: normalhighest

comment:8 Changed 14 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from itamarst to therve

These strings are almost but not quite ISO8601.

"2001-02-02 23:05:06 -0500"
vs.
"2001-02-02 23:05:06-0500"

The docstring says: "human-readable string", update this to say, "ISO8601 string", and and please update the strings to be valid ISO8601 strings, then you may merge.

Changed 14 years ago by therve

Attachment: 867_log.diff added

comment:9 Changed 14 years ago by therve

Keywords: review added

OK I updated the patch according to your comments. I tried to be more explicit in the docstring, and changed the timestamp position (note that's still not ISO8601, as jknight pointed out).

comment:10 Changed 14 years ago by Stephen Thorne

Cc: Stephen Thorne added
Keywords: review removed

Facinating. I always considered the 'T' optional, but according to the sources I just read, it's valid to omit it, but you change the meaning. When you omit the 'T' and use a ' ' instead, you're actually putting an ISO8601 date next to an ISO8601 time, which isn't the same thing as having an ISO8601 datetime. How schizophrenic.

Oh well, mx.Datetime.ISO.ParseAny() pulls it in fine, so that's good enough for me.

I'm satisfied with this patch, please commit to trunk. Please make a note in your commit message intended for the release manager what the old format was and what the new format is. This will be important information for the release notes (someone, somewhere, might rely on the old timestamp format).

comment:11 Changed 14 years ago by therve

Owner: therve deleted

comment:12 Changed 14 years ago by Jean-Paul Calderone

Priority: highestnormal

I think this basically addresses #880 as well. However, I think the nasty crud in conch should be removed as part of this change as well (so I am not applying the patch now).

I'll probably get to the conch stuff soon, but if anyone feels like beating me to it...

comment:13 Changed 14 years ago by therve

For the conch stuff, you're talking about _LogTimeFormatMixin in test_ssh ? If it's that I can update the patch with its removal, it doesn't seem to have to much impact.

comment:14 Changed 14 years ago by Jean-Paul Calderone

Yea, that's what I mean.

Jerub has been threatening to actually fix this issue, though, by making it possible to configure logging on the twistd command line. If he makes makes any progress on that it might be a good idea to include this change as a part of that one. I generally like to keep changes as separate and tiny as possible, but with actual configurability, there wouldn't even need to be a change to the default logging format.

comment:15 Changed 14 years ago by therve

Keywords: review added
Priority: normalhighest

As I didn't get news from something into twistd, I went on and cleaned some stuff.

Ready to review in improve-log-867.

comment:16 Changed 14 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

I reflowed the part of the formatTime docstring which talks about ISO8601, since it looked wierd.

Everything else in this branch looks great. Please merge it.

comment:17 Changed 14 years ago by therve

Resolution: fixed
Status: newclosed

(In [19485]) Merge improve-log-867

Author: therve Reviewer: exarkun Fixes #867

Add seconds to default log format, for using ISO8601 date and an ISO8601 time. In the process some cleanups have been made.

People relying on the old default format (to parse log for example) should be warned.

comment:18 Changed 10 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.