[rfc][icedtea-web] refactored logging

[Jiri Vanek]

On 09/12/2013 07:59 PM, Omair Majid wrote:
> 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?

Definitely not, as should not be exception or other debugging informations.
Also this is extremly rare case in real life.

>
>> +    private /*final*/ PrintStream err;
>> +    private /*final*/ PrintStream out;
>
> Why the /*final*/? I can understand /*package-private*/, but this is not
> obvious to me.

Well it meant I would like to have them final :)
But few tests needs to switch the stream of ITW. Maybe some deeper reinventing of the affected test 
(see adaptedTests)  would alow me to avoid shis, but now I'mhappy with non final streams.

Commented final removed.
>
>> +    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?

sure, done,. even more javadoc added.
>
>> +    /*
>> +     * 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?

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

fixed.

getters are no public, and setters private. They needs to be assessed via reflection anyway.

>
>> +    /**
>> +     * 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.

fixed. MAde protected.
>
>> +    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.

In case of enum creation of singleton can not be controlled. I once burned myself on it, so I get 
used to this pattern.

If you insists, I will switch to enum.
>
>> +    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.

hmhmh... Will add few more declarations. As object.toString should be perfectly ok, then I'm more 
for this. It will allow us to log object instead of messages in time.

But no object (jsut string or throwable for now) is looged for now. If you really wont me to 
duplicate declaration, then use force, and it will be done. Anyway I like it more this way.


>
>
>> +    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?

Yah. I was thinking about it. And I think it would be worthy.
However the amount the work needed to evaluate each message and decide to which level it belongs, 
appeared to me as to much.

Can you imagine doing it? If so, then I will probably reconsider.
>
>> +    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

yes

> and the icedtea-web log files?

no - this must be explicitly enabled.
 > Is this a good idea? Looking at my

Not sure. I'm thinking about checkbox "diable systenm logging" intto debug pane in itw-settings.
> current system logs, I get maybe a couple of hundred lines per day

maybe reason to use custm syslog entry. And the amount should be contorled via logging subsytem or not?

>(and

One of the reason for this is to have it enabled by default. If somebody is using itw on daily work, 
and no exception appears, and logging will become blocker, then he will investiagte how to disable it.
> most of them are from buggy programs). Wouldn't a single javaws run
> double that size?

Dont forget that itw is not used in application like 24/7
>
> Maybe we should think about making these methods more semantic and only
> syslog'ing those errors that are important.

Which are which??? :((
>
> 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.

Parially. I have used them also in itw to print messages like -abourt and -help. //see the 
refactoring patch for them)

Maybe this change crossed the purpose of this patch. I was under obsession to have "bottleneck to 
control all outputs" Now I''m hesitating here
>
>> +    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.

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

Agree. Right now I would like to keep this in (As It is already subject of review) but later I will 
meove them to :system logger: implementation.


Thank you very much for review. Today - lack of time -  I will add tests to ItwLogger in later 
iteration.


J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extractedLogging-adaptedTests.patch
Type: text/x-patch
Size: 14048 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130913/876eea69/extractedLogging-adaptedTests.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extractedLogging-bottleNeck2.patch
Type: text/x-patch
Size: 9773 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130913/876eea69/extractedLogging-bottleNeck2.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extractedLogging-codeRemoval.patch
Type: text/x-patch
Size: 1635 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130913/876eea69/extractedLogging-codeRemoval.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: extractedLogging-refactoring.patch
Type: text/x-patch
Size: 202436 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130913/876eea69/extractedLogging-refactoring.patch