Log4j 2
  1. Log4j 2
  2. LOG4J2-455

RingBufferLogEvent should use Messsage timestamp first

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
        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
        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.
        Hide
        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
        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
        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
        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
        Remko Popma added a comment -

        Fixed in revision 1553122.
        Please verify and close.

        Show
        Remko Popma added a comment - Fixed in revision 1553122. Please verify and close.
        Hide
        robin zhang tao added a comment -

        Verified And Thanks.

        Show
        robin zhang tao added a comment - Verified And Thanks.

          People

          • Assignee:
            Remko Popma
            Reporter:
            robin zhang tao
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development