Ticket #2404 (closed defect: fixed )

Opened 2 years ago

Last modified 2 years ago

DST calculation problem in log.py/FileLogObserver.getTimezoneOffset

Reported by: thor Assigned to: exarkun
Type: defect Priority: highest
Milestone: Component: core
Keywords: regression Cc: thor, exarkun, itamarst, jknight
Branch: Author:
Launchpad Bug:

Description

getTimezoneOffset uses Python's "time.daylight" to determine whether or not localtime currently needs to be adjusted by DST rules, but time.daylight indicates just that the local time zone uses DST at all, not that it should be used at the moment, thus getTimezoneOffset always returns the DST offset even in winter (i.e. now in the northern hemisphere) resulting in a local time that is off by 1 hour for me at the moment (CEST instead of CET is used).

Cf. http://docs.python.org/lib/module-time.html , the Python src code (Modules/timemodule.c) and the following from log.py

def getTimezoneOffset(self):

""" Return the current local timezone offset from UTC.

@rtype: C{int} @return: The number of seconds offset from UTC. West is positive, east is negative. """

if time.daylight:

return time.altzone

return time.timezone

Attachments

Change History

  2007-01-29 03:30:44+00:00 changed by exarkun

  • keywords set to regression, review
  • owner deleted
  • priority changed from normal to highest

Added a test and a fix in logging-timezone-2404

Someone please review!

  2007-01-29 03:48:40+00:00 changed by exarkun

  • cc set to thor

It occurs to me that the code in the branch does not explicitly account for the possibility of tm_isdst being -1. I don't know when it takes on this value, apparently never on Linux with glibc 2.3.6 in EST/EDT. Maybe other platforms/timezones have it, though. It's not really clear what can be done differently to handle this case. Maybe we should just log in UTC always.

  2007-01-30 15:37:50+00:00 changed by zseil

There's been a recent bug report with a similar content in the Python bugtracker: http://www.python.org/sf/1647654

The reporter suggests the following solution:

def getTimezoneOffset(when):
    offset = datetime.fromtimestamp(when) - datetime.utcfromtimestamp(when)
    return offset.days * 86400 + offset.seconds

  2007-01-31 16:39:21+00:00 changed by therve

  • keywords changed from regression, review to regression
  • owner set to exarkun

What do you think ? It seems a bit cleaner than the current solution ?

  2007-01-31 16:57:29+00:00 changed by exarkun

  • cc changed from thor to thor, exarkun

  2007-01-31 17:08:03+00:00 changed by exarkun

  • cc changed from thor, exarkun to thor, exarkun, itamarst, jknight

It does push some logic out into a lower level library. It's also more correct (although maybe not in any real-life situation). Unfortunately, it's harder to test. In fact, I can't think of any correct way to test any implementation of this method.

I think I'm now convinced that the default log observer should log in UTC, not local time.

  2007-01-31 17:27:52+00:00 changed by jknight

To test: Set the TZ env var to something well-known, call time.tzset(), check results, put back TZ and call tzset() again.

  2007-01-31 18:56:03+00:00 changed by exarkun

  • keywords changed from regression to regression, review
  • owner deleted

Hoorj. Okay, thanks. :)

utcfromtimestamp/fromtimestamp/tzset based implementation in the branch now.

  2007-01-31 19:58:14+00:00 changed by thor

I don't think logging that time in UTC is such a great idea, as I guess most people interested in the actual value of that timestamp would want to know that time in relation to their normal real life timescales or in relation to other log files, which are mostly in local time as well. Sure you can adjust that time in your head, but who really wants to do that every time..

Having to customize the logger in every twistd 10 liner just to be able to read the time when you're still half asleep is cumbersome....

  2007-02-01 01:35:51+00:00 changed by itamarst

calendar.timegm might be useful based on one of the comments you made in the test.

thor: I'm pretty sure log display will not be in UTC, but exarkun can correct me.

  2007-02-01 03:03:52+00:00 changed by exarkun

It does look like timegm would be useful in replacing the mktime() line, but since that was there already I'm not going to touch it here.

Indeed, the log display will still be in local time. Since James pointed out a way to actually test this, I preserved the existing behavior. I would still probably prefer UTC, but I'm not actually going to implement that until I have a more compelling reason to.

  2007-02-01 09:54:50+00:00 changed by therve

  • keywords changed from regression, review to regression
  • owner set to exarkun

Sorry, but tzset is only available on Unix.

  2007-02-01 13:20:25+00:00 changed by thor

OK, sorry about that then, I seem to have missed the "+ tzOffset" bit in line 325

  2007-02-01 17:41:57+00:00 changed by jknight

+1 for test.skip = "STBWindows" on Windows.

  2007-02-02 15:53:43+00:00 changed by exarkun

  • keywords changed from regression to regression, review
  • owner deleted

Okay, skipped if tzset is missing.

  2007-02-02 19:39:22+00:00 changed by therve

  • keywords changed from regression, review to regression
  • owner set to exarkun

Good :). Please merge.

  2007-02-03 18:16:23+00:00 changed by exarkun

  • status changed from new to closed
  • resolution set to fixed

(In [19561]) Merge logging-timezone-2404

Author: exarkun Reviewer: therve Fixes #2404

Correct FileLogObserver?.getTimezoneOffset to properly handle the difference between daylight saving and standard time, by delegating to a lower level (something inside the datetime type) rather than trying to determine whether daylight saving time is on effect or not itself.

Note: See TracTickets for help on using tickets.