Log4net
  1. Log4net
  2. LOG4NET-59

[PATCH] to RollingFileAppender.cs to add the ability to roll files based on universal time (UTC).

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.9
    • Fix Version/s: 1.2.11
    • Component/s: Appenders
    • Labels:
      None

      Description

      the file naming and rolling time zone is selected via a new property. the default value of this property provides backwards compatible bahviour. the patch is done on the published 1.2.9beta version. the diff file is only informative (not made with the right tool), use original and patched files to create a useful diff (accpet my apologies for this). -L

      1. RollingFileAppender_UniversalDateTime.patch
        2 kB
        Ron Grabowski
      2. Patched-1.2.9-beta-RollingFileAppender.cs
        48 kB
        Lanchon
      3. Original-1.2.9-beta-RollingFileAppender.cs
        47 kB
        Lanchon
      4. LOG4NET-59-LastWriteTime.patch
        0.8 kB
        Bassam Tabbara
      5. informative-diff.txt
        4 kB
        Lanchon

        Activity

        Hide
        Ron Grabowski added a comment -

        Added UniversalDateTime class that implements IDateTime using DateTime.UtcNow instead of DateTime.Now. Added public property to RollingFileAppender called DateTimeStrategy which defaults to DefaultDateTime (DateTime.Now). This is how you set the DateTimeStrategy of the RollingFileAppender via an XML config file:

        <dateTimeStrategy type="log4net.Appender.RollingFileAppender+UniversalDateTime" />

        Show
        Ron Grabowski added a comment - Added UniversalDateTime class that implements IDateTime using DateTime.UtcNow instead of DateTime.Now. Added public property to RollingFileAppender called DateTimeStrategy which defaults to DefaultDateTime (DateTime.Now). This is how you set the DateTimeStrategy of the RollingFileAppender via an XML config file: <dateTimeStrategy type="log4net.Appender.RollingFileAppender+UniversalDateTime" />
        Hide
        Nicko Cadell added a comment -

        Ron,

        I much prefer exposing the DateTimeStrategy rather than adding specialised control flags, this allows the user to implement their own strategy if the built-in options do not suit.

        A few questions/suggestions/enhancements:

        How about renaming the DefaultDateTime class to LocalDateTime. This would fit better with the name of the UniversalDateTime alternative and is a better description of the behaviour.

        Could you add a <value> and <remarks> section to the doc comments for the new DateTimeStrategy property. In the remarks indicate what the default value is; what the expected behaviour of the IDateTime is (to return a timestamp for the event); and list the implementations that are available and how they should be specified in the config file. The nested class syntax is not widely known so it would be good to include a very brief paragraph on that which gives the example: <c>log4net.Appender.RollingFileAppender+UniversalDateTime</c>.

        Should it be considered an error to set the strategy to null? Should the DateTimeStrategy setter check for null and throw an exception? The alternative is to leave the m_dateTime null until ActivateOptions. If null then it is set to an instance of DefaultDateTime, then it wouldn't be an error to set DateTimeStrategy to null. I think we need to do one or the other, and probably the second, i.e. the appender tries its best to recover.

        Can you commit your patch with the above changes and mark this issue as resolved.

        Cheers.

        Show
        Nicko Cadell added a comment - Ron, I much prefer exposing the DateTimeStrategy rather than adding specialised control flags, this allows the user to implement their own strategy if the built-in options do not suit. A few questions/suggestions/enhancements: How about renaming the DefaultDateTime class to LocalDateTime. This would fit better with the name of the UniversalDateTime alternative and is a better description of the behaviour. Could you add a <value> and <remarks> section to the doc comments for the new DateTimeStrategy property. In the remarks indicate what the default value is; what the expected behaviour of the IDateTime is (to return a timestamp for the event); and list the implementations that are available and how they should be specified in the config file. The nested class syntax is not widely known so it would be good to include a very brief paragraph on that which gives the example: <c>log4net.Appender.RollingFileAppender+UniversalDateTime</c>. Should it be considered an error to set the strategy to null? Should the DateTimeStrategy setter check for null and throw an exception? The alternative is to leave the m_dateTime null until ActivateOptions. If null then it is set to an instance of DefaultDateTime, then it wouldn't be an error to set DateTimeStrategy to null. I think we need to do one or the other, and probably the second, i.e. the appender tries its best to recover. Can you commit your patch with the above changes and mark this issue as resolved. Cheers.
        Hide
        Ron Grabowski added a comment -

        Fixed in 398209.

        Show
        Ron Grabowski added a comment - Fixed in 398209.
        Hide
        Bassam Tabbara added a comment -

        I believe there is still a bug with this fix that went into r398209. RollingFileAppender.RollOverIfDateBoundaryCrossing() still uses the local file time. Attached is a patch that fixes the problem.

        Show
        Bassam Tabbara added a comment - I believe there is still a bug with this fix that went into r398209. RollingFileAppender.RollOverIfDateBoundaryCrossing() still uses the local file time. Attached is a patch that fixes the problem.
        Hide
        Bassam Tabbara added a comment -

        Fixes the lastwritetime issue.

        Show
        Bassam Tabbara added a comment - Fixes the lastwritetime issue.
        Hide
        Lanchon added a comment -

        Ron,

        assuming the reported bug is real (i've never used a LOG4NET version newer than 1.2.9), the "LOG4NET-59-LastWriteTime.patch" uses a switch-on-type strategy to workaround the issue that is unacceptable for release:

        + if (this.DateTimeStrategy is UniversalDateTime)
        + //...
        + else
        + //...

        when i submitted the first patch i used a "DateTime AdjustTimeZone(DateTime localTime)" instead of a getCurrentTime()-style function for a reason: the previous code was a bit of a mess, major surgery was required for a cleaner solution which was not warranted (assuming the previous code was well tested).

        if your patch was based on code similar to 1.2.9, then there are a few more things to do than just GetCurrentTime() in the desired timezone. see my diff file for a guide on other things that needed to be done on 1.2.9. if you need me i'm available for questions regarding the 1.2.9 era code.

        best,
        L

        Show
        Lanchon added a comment - Ron, assuming the reported bug is real (i've never used a LOG4NET version newer than 1.2.9), the " LOG4NET-59 -LastWriteTime.patch" uses a switch-on-type strategy to workaround the issue that is unacceptable for release: + if (this.DateTimeStrategy is UniversalDateTime) + //... + else + //... when i submitted the first patch i used a "DateTime AdjustTimeZone(DateTime localTime)" instead of a getCurrentTime()-style function for a reason: the previous code was a bit of a mess, major surgery was required for a cleaner solution which was not warranted (assuming the previous code was well tested). if your patch was based on code similar to 1.2.9, then there are a few more things to do than just GetCurrentTime() in the desired timezone. see my diff file for a guide on other things that needed to be done on 1.2.9. if you need me i'm available for questions regarding the 1.2.9 era code. best, L

          People

          • Assignee:
            Ron Grabowski
            Reporter:
            Lanchon
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development