logging-log4j2 icon indicating copy to clipboard operation
logging-log4j2 copied to clipboard

Published 20 hours ago • verified publisher icon apache

LOG4J2-3280 - Add support for microsecond precision in RFC5424Layout

Open atulpendse opened this issue 3 years ago • 14 comments

This pull request is to imlement RFE LOG4J2-3280 The change

  • Adds new attribute timestampPrecision to Rfc5424Layout and SyslogAppender.
  • Replaces existing logic to append timestamp and uses two new fixed date formats to format timestamp.
  • Added new unit tests
  • Verified that 'mvn clean install' and 'mvn verify' are successful.

atulpendse avatar Jan 05 '22 17:01 atulpendse

I believe the failure in PropertiesConfigurationTest is unrelated to this change, and being fixed with #685 Once that is fixed, how do I get this PR to build and run CI tests again?

atulpendse avatar Jan 06 '22 06:01 atulpendse

Hi @atulpendse You will need to rebase this PR to pick up the latest changes.

garydgregory avatar Jan 06 '22 12:01 garydgregory

Hi @atulpendse You will need to rebase this PR to pick up the latest changes.

Thanks for the info. I have updated PR with the latest from upstream. Latest commit seems to have passed all the tests.

atulpendse avatar Jan 06 '22 13:01 atulpendse

Fantastic work @atulpendse! Thanks so much!

Mind adding SyslogAppender tests ~~and a changes.xml entry~~, please?

I am also not sure if a timestampPrecision argument of type String is the right way to go. Thinking out loudly:

  • Why don't we simply make the instant formatting pattern configurable?
  • Doesn't the spec enforce a certain precision?
  • If not, why don't we simply (automatically?) pick the most precise representation possible?

vy avatar Jan 09 '22 20:01 vy

I would NOT update changes.xml, this makes cherry-picking the commit to master impossible.

garydgregory avatar Jan 09 '22 21:01 garydgregory

IMHO, cherry-picking to master is almost already impossible. Nevertheless, I will rephrase my comment to exclude the changes.xml part.

vy avatar Jan 09 '22 22:01 vy

@vy Thanks for taking time to review the PR. Here are my thoughts about questions you raised.

I am also not sure if a timestampPrecision argument of type String is the right way to go. Thinking out loudly:

  • Why don't we simply make the instant formatting pattern configurable?

With making pattern configurable, there is risk of the message not being compliant to RFC5424 spec (if someone updates format that is not as per RFC5424)

  • Doesn't the spec enforce a certain precision?

The spec does not enforce 'a particular' precision, rather it supports three precisions

  1. Without any sub-second value
  2. Milliseconds
  3. Microseconds
  • If not, why don't we simply (automatically?) pick the most precise representation possible?

We could, that would work for my usecase. But if someone is expecting precision to be millisecond (like my app was expecting it to be microsecond), it would break their app.

I will add a test for SyslogAppender.

atulpendse avatar Jan 10 '22 04:01 atulpendse

Commit fbccefc updates existing test for SyslogAppender

atulpendse avatar Jan 11 '22 05:01 atulpendse

@vy @garydgregory anything else that I should do to make progress on this PR?

atulpendse avatar Jan 17 '22 10:01 atulpendse

I only took a cursory look this AM. Please use the style of the files you are editing: Use spaces, no tabs. The bigger question is whether we should use an enum for the microsecond vs millisecond format, @jvz any thoughts?

Thanks for taking time to review. I have fixed the concerns with latest commit. Now timestampPrecision is an enum.

atulpendse avatar Jan 20 '22 13:01 atulpendse

Looks like a test is failing on MacOS. Failure doesn't look related to my changes though. I don't have access to Mac to find out why exactly it's failing.

atulpendse avatar Jan 21 '22 17:01 atulpendse

@garydgregory, please let me know how I can help to get the PR going. I rebased the PR with latest code and submitted back with commit b425497

atulpendse avatar Mar 02 '22 13:03 atulpendse

Just FYI.. I was able to build it successfully on my Mac. Last time @garydgregory tried to submit build with this PR, it failed on Mac, not sure why.

atulpendse avatar Mar 03 '22 06:03 atulpendse

Hello @atulpendse In general, you want to avoid breaking binary compatibility, so that means not editing public and protected type names and method signatures. This is critical in the log4j-api module and less so in others, but, in this case it seems easy to do. Also, we have been switching from factory methods to builders as we go from release to release in order to avoid keeping on deprecating factory methods. So edit or add builders, don't edit factory methods is the idea. TY!

garydgregory avatar Mar 28 '22 19:03 garydgregory

@vy Why was this closed? To be honest, I am not sure why this hasn't been merged.

rgoers avatar Mar 01 '23 06:03 rgoers

It's a side effect of deleting the release-2.x branch: Github deletes the related PRs.

@atulpendse: can submit again against the 2.x branch?

ppkarwasz avatar Mar 01 '23 06:03 ppkarwasz