[rfc][icedtea-web] refactored logging

[Omair Majid]

Hi Jiri,

I am mostly interested in this patch as a user (especially if some else
has agreed to a full review of this giant patch). But it caught my
interest so I have a few questions.

First, please don't abuse 'refactoring'. It's defined here:
http://refactoring.com/. This patch is a change (probably a good one);
it's not (just) a refactor.

On 09/10/2013 04:19 PM, Jiri Vanek wrote:
> This patch is introducing bottleneck for all stdout/err and
> printstacktrace.

I thought bottlenecks were bad? ;)

> By this it is first part of
> http://icedtea.classpath.org/wiki/IcedTea-Web#IcedTea-Web_1.5 "introduce
> new logging and think about abrt connection"

Okay, but that's not explaining things any better :)

> Inspiration:
>  - get rid of repeated if JNLPRuntime.isDebug

I am sure it's not relevant here, but OpenJDK switched from:

log(foo)

to

if (debug) {
  log(foo)
}

in a couple of places due to performance reasons. Just something to keep
in mind.

>  - log properly into syslog (comming as next patch(*)(**) after this one
> s in)

This sounds (at once) really cool and really scary.

Can you elaborate on exactly what you mean by syslog? Is it the syslog
protocol as defined in RFC 3164? What if a syslog daemon is not running?

Can you describe the motivation for logging into syslog? Is this
somethings lots of other user programs do?

>  - revive the old logger which logs into ~/..../logs to log more then
> few start up exception (if enabled)

Are you saying the old logger is broken? Can you elaborate?

>  - maybe something more, I got very asleep :)

You mean to say, you are not working 24/7 any more? That's not acceptable :P

> The only real code  is in extractedLogging-bottleNeck. The syslog
> writer(*) is not yet implemented and will be inside method write, which
> is now just dummy

Have you looked at existing implementations? Maybe we can reuse something?

> (**) The best way how to integrate the syslogs writer is still under
> research (tcp?, exec?)

External-library?

> The extractedLogging-refactoring is extremely long, And I do not expect
> somebody to read it whole.
> It is moreover just all the same:
> 
> - if (JNLPRuntime.isDebug())
> -            System.err.println("UNIQUEKEY=" + this.uniqueKey);
> +        ItwLogger.getLogger().logErrorStream("UNIQUEKEY=" +
> this.uniqueKey);

Just to be clear, logging is not the same as messages displayed on
stderr. There are probably some messages that we still want to show to
the user on stderr (and log too, maybe).

> or
> 
> -            if (JNLPRuntime.isDebug())
> -                ex.printStackTrace();
> +            ItwLogger.getLogger().log(ex);
> 
> or its variants without  if (JNLPRuntime.isDebug()) and so adding "true"
> parameter which force logging always:
> 
> -            System.err.println("UNIQUEKEY=" + this.uniqueKey);
> +        ItwLogger.getLogger().logErrorStream(true, "UNIQUEKEY=" +
> this.uniqueKey);
> 
> or
> 
> -                ex.printStackTrace();
> +            ItwLogger.getLogger().log(true, ex);
> 
> And  hope I did not miss something or not changed if debug/if not debug
> (/me have to try reprodcuers with/without patch)

I am probably missing something, but why isn't ItwLogger a
java.util.logging.Logger?

Please add javadocs to indicate when to use which methods. Also, please
add unit tests for (at least) the ItwLogger class.

> The only real changes are minor and are in
> /tests/netx/unit/net/sourceforge/jnlp/DefaultLaunchHandlerTest.java

s/baos1/outputStream and s/baos2/errorStream/

> and removed duplicated code arround exceptionToString.
> netx/net/sourceforge/jnlp/ParseException.java (removed redundant code
> 1.3 compatibility..hehe)

A separate patch, please!

Cheers,
Omair

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