Use SLF4J to allow easily routing logs to multiple logging frameworks

[ghost]

This PR is relative to #32

This is a full conversion to SLF4J's API, including using dynamic string formatting (this prevents String operations when the log levels are not enabled).

Since the logging framework used as backend for SLF4J is going to be responsible for selecting log levels and other details, all we have to do is call SLF4J transparently.

Using JUL, JCL, logback, Log4J or Loj4J2 is now possible with the addition of one of the SLF4J implementations as a dependency to projects that use this library. The logs will then be routed to whichever framework the user chooses to use.

[cowtowncoder]

Thank you! I'll have to think about adding one new static dependency here: I am against it, but just want to think it through. And then decide how to increase version number (at least minor version bump, maybe major).

[ghost]

Generally speaking, linking to slf4j-api is safe. As long as there's some version of the API jar (and they're basically all compatible with each other), it'll work.

SLF4J has a nice page talking about the ideal way of dealing with dependencies. I'll put the link here for you later :)

[ghost]

Basically

However, from the client's perspective all versions of slf4j-api are compatible. Client code compiled with slf4j-api-N.jar will run perfectly fine with slf4j-api-M.jar for any N and M. You only need to ensure that the version of your binding matches that of the slf4j-api.jar. You do not have to worry about the version of slf4j-api.jar used by a given dependency in your project. You can always use any version of slf4j-api.jar, and as long as the version of slf4j-api.jar and its binding match, you should be fine.

At initialization time, if SLF4J suspects that there may be an slf4j-api vs. binding version mismatch problem, it will emit a warning about the suspected mismatch.

https://www.slf4j.org/manual.html

[ghost]

More details here: https://www.slf4j.org/faq.html#optional_dependency And here: https://www.slf4j.org/faq.html#maven2

But the bottom line is, linking to slf4j-api directly is totally safe.

[andrebrait]

I deleted my other GitHub account. Contact this account if you need.

[cowtowncoder]

@andrebrait gotcha.

[cowtowncoder]

@andrebrait Ok so, yes, I think this makes sense. Code looks good, although I have one question: would it make more sense to try to retain class name of caller, for logger, instead of basically making them all appear being logged by Logger class? That would require bit more complicated handling so not sure it's worth it, but would probably be bit less confusing to end users.

[andrebrait]

@cowtowncoder yes, it makes sense to do it that way as well. The reason why I had it like this is because it would allow for a smoother transition, let's say, as all logging would be under the same logger, as it already is with the current implementation using log4j.

However, given people can control the whole set of loggers pra using only part of their names (which means, here, the name of your root package, com.fasterxml.uuid) that concern is not exactly a big downside, so maybe I should indeed refactor it and have per-class static instance of the logger.

[cowtowncoder]

@andrebrait if that'd be simple enough to do, then given that there will also be major version bump, I think that'd be the way to go here.

[andrebrait]

@cowtowncoder it's super simple. I'll do that today.

[andrebrait]

@cowtowncoder I performed the change and also rebased on your current branch.

I also added the version of a maven plugin you use since it was complaining that it was absent:

[WARNING] 
[WARNING] Some problems were encountered while building the effective model for com.fasterxml.uuid:java-uuid-generator:bundle:3.3.1-SNAPSHOT
[WARNING] 'build.plugins.plugin.version' for org.apache.maven.plugins:maven-source-plugin is missing. @ line 95, column 17
[WARNING] 
[WARNING] It is highly recommended to fix these problems because they threaten the stability of your build.
[WARNING] 
[WARNING] For this reason, future Maven versions might no longer support building such malformed projects.
[WARNING] 
[WARNING] The project com.fasterxml.uuid:java-uuid-generator:bundle:3.3.1-SNAPSHOT uses prerequisites which is only intended for maven-plugin projects but not for non maven-plugin projects. For such purposes you should use the maven-enforcer-plugin. See https://maven.apache.org/enforcer/enforcer-rules/requireMavenVersion.html

[andrebrait]

@cowtowncoder I don't think I tested using SLF4J while running this as a standalone application. I assume that's still important to work, right?

[andrebrait]

I fixed it, but I found that, with the upstream JRE 8 (current 242_b08) on Linux x86-64, some tests are failing randomly:

junit.framework.AssertionFailedError: start time was not before the end time
        at com.fasterxml.uuid.UUIDGeneratorTest.checkUUIDArrayForCorrectCreationTime(UUIDGeneratorTest.java:507)
        at com.fasterxml.uuid.UUIDGeneratorTest.testGenerateTimeBasedUUID(UUIDGeneratorTest.java:186)

[ERROR] Failures: 
[ERROR]   UUIDGeneratorTest.testGenerateTimeBasedUUID:186->checkUUIDArrayForCorrectCreationTime:507 start time was not before the end time

[andrebrait]

It seems it runs too fast :sweat_smile:

[ERROR] testGenerateTimeBasedUUID(com.fasterxml.uuid.UUIDGeneratorTest)  Time elapsed: 0.013 s  <<< FAILURE!
junit.framework.AssertionFailedError: Start time: 1581692244668 was not before the end time: 1581692244668
        at com.fasterxml.uuid.UUIDGeneratorTest.checkUUIDArrayForCorrectCreationTime(UUIDGeneratorTest.java:507)
        at com.fasterxml.uuid.UUIDGeneratorTest.testGenerateTimeBasedUUID(UUIDGeneratorTest.java:186)

[ERROR] Failures: 
[ERROR]   UUIDGeneratorTest.testGenerateTimeBasedUUID:186->checkUUIDArrayForCorrectCreationTime:507 Start time: 1581692244668 was not before the end time: 1581692244668

[andrebrait]

@cowtowncoder maybe we should change the assertion from < to <=?

[cowtowncoder]

Hmmh. Ok, first, yeah; stand-alone app would still be use case so good to have that handled (slf4j complains about missing impl I guess?).

As to test, I'll have a look... been a long time so do not remember logic behind it.

[cowtowncoder]

@andrebrait Yes, changing to <= seems correct: can not remember why assumption was that it would have to move -- while logical time used may need to move (wrt block of 10k uuids), it does not necessarily mean system timer would advance (there is some delay logic for case where generation occurs too fast wrt 100 nsec slots but that's more of impl detail).

Apologies for slow replies, too, and thank you for making changes. I'll try to get this through quickly now.

[andrebrait]

@cowtowncoder no problem :-) Want me to commit the change before merging? I can do it in the editor here on GitHub. Or would you rather just do it afterwards?

[cowtowncoder]

Either way is fine -- I was thinking of merging tonight from home system; if you want to make change before that, fine, if not, I can do post-merge too.

[andrebrait]

Ok, I'll leave it to you then. I have a busy evening now.

[cowtowncoder]

@andrebrait that works -- thank you for the contribution again!