[rfc][icedtea-web] refactored logging

[Omair Majid]

Hi Jiri,

Some comments on the patch itself.

On 09/10/2013 04:19 PM, Jiri Vanek wrote:
> +public class ItwLogger {

> +    private static final String NULL_OBJECT = "Trying to log null object";

Should this be localized?

> +    private /*final*/ PrintStream err;
> +    private /*final*/ PrintStream out;

Why the /*final*/? I can understand /*package-private*/, but this is not
obvious to me.

> +    private static ItwLogger logger;
> +
> +    private ItwLogger() {
> +        this(System.out, System.err);
> +    }

It's not obvious that this is a singleton from these few lines. Could
you move the other (public/non-public) constructors and the
getInstance() method here too?

> +    /*
> +     * for testing puposes!
> +     */
> +
> +    public PrintStream getOut() {
> +        return out;
> +    }
> +    /*
> +     * for testing puposes!
> +     */
> +
> +    public PrintStream getErr() {
> +        return err;
> +    }
> +    /*
> +     * for testing puposes!
> +     */
> +
> +    public void setOut(PrintStream out) {
> +        this.out = out;
> +    }
> +    /*
> +     * for testing puposes!
> +     */
> +
> +    public void setErr(PrintStream err) {
> +        this.err = err;
> +    }

It's hard for me to tell what these comments correspond to? Are they for
the functions following the comment or before the comment?

If a method is exposed for a unit test, maybe 'protected' or
/*package-private*/ maybe a better scope?

> +    /**
> +     * for testing purposes the logger with custom streams canbe created
> +     * otherwise only getLogger()'s singleton can be called
> +     */
> +    public ItwLogger(PrintStream out, PrintStream err) {
> +        if (out == null || err == null) {
> +            throw new IllegalArgumentException("No stream canbe null");
> +        }
> +        this.err = err;
> +        this.out = out;
> +    }

Providing a public constructor basically breaks the singleton
encapsulation. Please reconsider if this is really needed.

> +    synchronized public static ItwLogger getLogger() {
> +        if (logger == null) {
> +            logger = new ItwLogger();
> +        }
> +        return logger;
> +    }

An enum is often used to implement a singleton with less synchronization
overhead.

> +    private void logToSystemLogger(StdStream stream, Object o) {

Maybe it would be nicer to simply accept a String or a Throwable here?
Since logging deals with strings, expecting the caller to make a string
explicitly sounds okay to me.


> +    public static String exceptionToString(Throwable t) {
> +        if (t == null) {
> +            //return "Exception was null";
> +            return null;

Please remove the commented return statement.

> +    public void log(Throwable o) {
> +        log(false, null, o);
> +
> +    }

Both java.util.logging and syslog define levels (things like DEBUG or
ERROR or SEVERE) and .NET defines EventLogEntryType with a similar
purpose. Maybe we should add this to the API?

> +    public void log(Object o) {

> +    public void log(boolean reprint, Object o) {

> +    public void logErrorStream(Object o) {

> +    public void logErrorStream(boolean reprint, Object o) {

> +    public void log(StdStream stream, Object o) {

> +    public void log(boolean reprint, StdStream stream, Object o) {

Something I am not very clear on: so this always logs to the system
logger and the icedtea-web log files? Is this a good idea? Looking at my
current system logs, I get maybe a couple of hundred lines per day (and
most of them are from buggy programs). Wouldn't a single javaws run
double that size?

Maybe we should think about making these methods more semantic and only
syslog'ing those errors that are important.

Or perhaps we should use the logging levels and only syslog
important-enough messages

> +    public void printErrorLn(String e) {

> +    public void printOutLn(String e) {

> +    public void printBothLn(String e) {

> +    public void printError(String e) {

> +    public void printOut(String e) {

> +    public void printBoth(String e) {

Do these really need to be public? They look like internal helper methods.

> +    private static String getCallerClass() {
> +        StackTraceElement[] stack = Thread.currentThread().getStackTrace();
> +        //0 is always thread
> +        //1..? is ItwLogger itself
> +        //pick up first after.
> +        StackTraceElement result = stack[0];
> +        String baseClass = stack[1].getClassName();

This probably will not work too well if reflection is used.

One suggestion when developing the syslog implementation: please keep it
as a separate class so we can add/remove that implementation separately.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681