Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement - Internal Logging Facade (decouple from java.util.logging) #829

Closed
gissuebot opened this issue Oct 31, 2014 · 7 comments
Closed

Comments

@gissuebot
Copy link

@gissuebot gissuebot commented Oct 31, 2014

Original issue created by jmsignup on 2011-12-18 at 09:27 PM


Guava is an excellent library, and it prudently uses logging in places like Closeables.closeQuietly(Closeable) when an IOException is caught but not rethrown.

However, it is coupled to the java.util.logging logging implementation. This makes adapting guava logging to existing projects difficult, if those projects use a different log framework. Converting to slf4j has been discussed but rejected to limit non-JRE dependencies. The discussion suggests using slf4j's jul-to-slf4j bridge, but that comes with a 20% performance reduction, even with the latest logback implementation. As the jul-to-slf4j bridge documentation notes, the java.util.logging implementation is more difficult to work around because it is a part of the java namespace, which has certain restrictions on it.

Instead, enhance guava to intoduce an internal logging facade. This is in some ways similar to slf4j but without bringing in any new dependencies. By using an internal logging facade, guava because decoupled from java.util.logging, allowing pluggable logging implementations.

I suggest using a model similar to Netty's internal logging pattern, which exists for this very purpose. By having a logger and logger factory internal interface, you allow a simple extension to the library for using whatever logging implementation the user wants. By default, you can enable a java.util.logging implemetation (I suggest calling it something like JulLoggerFactory instead of Netty's JdkLoggerFactory, but that's a minor note). There are plenty of good logger interfaces available to model after. As I see it, you mainly need isXxxxLevelEnabled() and log.xxxx(String, Throwable) for the decided upon logging priority levels (like in log4j's Logger).

One other recommendation to Netty's implementation. I would prefer configuring the logger factory from a static call and also by specifying a class name in a system property. A system property ensures configurability at startup and is a pattern used throughout the JRE APIs, allowing for greater flexibility and adaptability to existing projects.

Minimizing non-JRE dependencies is a great goal for a common library like Guava but allow for plugging in other logging implementations for easier acceptance into more projects.

References:

@gissuebot
Copy link
Author

@gissuebot gissuebot commented Oct 31, 2014

Original comment posted by wasserman.louis on 2011-12-21 at 12:01 PM


Not sure how I feel about this.

I really, really like that Guava doesn't make you think about details that are irrelevant to most users...which I'd think applies here.

The least intrusive possibility I can imagine would be something like this: we currently have Caches swallow and log exceptions under certain conditions. We could add a small method to CacheBuilder that would take some other logging system. That would be relatively non-intrusive, depending on the precise API.

That said...I'm inclined to be conservative on adding new top-level interfaces or classes to Guava for this issue, given that it's a non-issue for most users.

Since there is already a workaround -- this would primarily be a performance enhancement, and the cases in which Guava needs to do logging are relatively rare -- I'm registering my -1 for this issue.

@gissuebot
Copy link
Author

@gissuebot gissuebot commented Oct 31, 2014

Original comment posted by toellrich on 2011-12-22 at 06:12 AM


As a user of Guava who does all his logging via SLF4J, I too would welcome not having to use the jul-to-slf4j bridge. Creating your own logging interface would also have the advantage that you could create a much better one, e. g. something similar to the one from JBOSS logging which uses varargs and String formats (https://github.com/dmlloyd/jboss-logging/blob/master/src/main/java/org/jboss/logging/Logger.java). If there's one thing I don't like about SLF4J, it's how it's still stuck in a pre-Java 5 world.

@gissuebot
Copy link
Author

@gissuebot gissuebot commented Oct 31, 2014

Original comment posted by jmsignup on 2011-12-22 at 06:14 AM


The default configuration would use java.util.logging. Without any configuration, the library would behave the same as today, logging to java.util.logging. This preserves the goal of keeping irrelevant details away from most users while providing flexibility for those who want to log with a different framework.

I recommend against adding logging flexibility on builder factories. I think peppering various classes with logging configuration actually creates more irrelevant details for users. Having to add another high level interface is unfortunate, but at least it centralizes logging concerns instead of spreading them throughout the API.

@gissuebot
Copy link
Author

@gissuebot gissuebot commented Oct 31, 2014

Original comment posted by wasserman.louis on 2011-12-22 at 06:02 PM


I'm still not clear how you would configure the logging behavior, in your ideal world. I'm not very comfortable with static state, and I can't tell if that's your suggestion, or something else.

@gissuebot
Copy link
Author

@gissuebot gissuebot commented Oct 31, 2014

Original comment posted by jmsignup on 2011-12-24 at 03:56 AM


Yes, I was suggesting static state initialized to a java.util.logging LoggerFactory, as Netty does with InternalLoggerFactory,

https://github.com/netty/netty/blob/master/src/main/java/io/netty/logging/InternalLoggerFactory.java#L37

I agree that static state should be used sparingly, but it seems appropriate for logging because loggers are typically used as static members. Again, take logging in Closeables as an example. It only contains static methods and has the static member "logger" for logging.

@gissuebot
Copy link
Author

@gissuebot gissuebot commented Oct 31, 2014

Original comment posted by kevinb@google.com on 2012-01-10 at 11:51 PM


The only problem here is a 20% performance penalty (when logging warnings that shouldn't really be happening anyway)? That doesn't seem worth being concerned about.

I don't think Guava should take on a slf4j dependency. And mutable static state is an evil I don't want to perpetrate on anyone ever again.


Status: WontFix
Labels: Type-Enhancement

@gissuebot
Copy link
Author

@gissuebot gissuebot commented Oct 31, 2014

Original comment posted by jmsignup on 2012-01-13 at 02:54 PM


I understand and respect your reasons for not implementing this request. Thank you for your consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant