Uploaded image for project: 'Log4j 2'
  1. Log4j 2
  2. LOG4J2-455

RingBufferLogEvent should use Messsage timestamp first

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0-beta9
    • Fix Version/s: None
    • Component/s: Appenders
    • Labels:
      None

      Description

      When AsyncLogger received log message (method

       public void log(final Marker marker, final String fqcn, final Level level, final Message data,
                  final Throwable t)
      

      ), currently it uses

      clock.currentTimeMillis()
      

      (see at Line 221) as the timestamp.

      But it should check the message implements TimestampMessage or not, if yes, should use TimestampMessage.getTimestamp() as the timestamp, if not , then using clock.currentTimeMillis().

      The new Code:

      data instanceof TimestampMessage ? ((TimestampMessage) data ).getTimestamp() : clock.currentTimeMillis();
      

        Activity

        Hide
        robinzt robin zhang tao added a comment -

        Verified And Thanks.

        Show
        robinzt robin zhang tao added a comment - Verified And Thanks.
        Hide
        remkop@yahoo.com Remko Popma added a comment -

        Fixed in revision 1553122.
        Please verify and close.

        Show
        remkop@yahoo.com Remko Popma added a comment - Fixed in revision 1553122. Please verify and close.
        Hide
        remkop@yahoo.com Remko Popma added a comment -

        That is an option but would entail an API change: adding the method getTimestamp to the Message interface in the api module.

        I don't mind making the suggested change (and avoiding the API change).
        I just want to do some performance testing: It is possible that (instanceof + method invocation on message) is faster than the call to clock.currentTimeMillis().
        If that is the case I'll do this in the calling application thread, otherwise in the background logging thread. Either will solve Robin's issue.

        Show
        remkop@yahoo.com Remko Popma added a comment - That is an option but would entail an API change: adding the method getTimestamp to the Message interface in the api module. I don't mind making the suggested change (and avoiding the API change). I just want to do some performance testing: It is possible that (instanceof + method invocation on message) is faster than the call to clock.currentTimeMillis(). If that is the case I'll do this in the calling application thread, otherwise in the background logging thread. Either will solve Robin's issue.
        Hide
        garydgregory Gary Gregory added a comment -

        The proposed code is not very OO how about making all messages implement getTimestamp with the default implement of using the system current milliseconds?

        Show
        garydgregory Gary Gregory added a comment - The proposed code is not very OO how about making all messages implement getTimestamp with the default implement of using the system current milliseconds?
        Hide
        remkop@yahoo.com Remko Popma added a comment -

        Robin,

        Sorry for the late response.
        I'll try to take a look at this soon.
        You do have a good point but I'd like to see what the performance impact is before making this change.

        Show
        remkop@yahoo.com Remko Popma added a comment - Robin, Sorry for the late response. I'll try to take a look at this soon. You do have a good point but I'd like to see what the performance impact is before making this change.

          People

          • Assignee:
            remkop@yahoo.com Remko Popma
            Reporter:
            robinzt robin zhang tao
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development