[rfc][icedtea-web] refactored logging

[Jiri Vanek]

On 09/11/2013 06:29 PM, Pavel Tisnovsky wrote:
> Hi Omair,
>
> ----- Omair Majid <omajid at redhat.com> wrote:
>> 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.

It is refactoring - the other changes are consequence - eg skeleton methods into which the 
ex.prrintstacktrace or system.out/err  are refactroed.

eh.. I admit they grow a bit over the plan.

>>
>> 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? ;)

You can consider System.out/err as same battleneck. Althoug I see your point, the lgging is always 
bottlenck for logging :)

>>
>>> 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)
>> }
>>

I'm aware of it. But openjdk is generating  endlessly more debugmessages...

>> 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?
>
> Yeah we thought about this one.

Research in progress:(

>
>>
>> Can you describe the motivation for logging into syslog? Is this
>> somethings lots of other user programs do?
>
> The main problem with the current solution is that most users don't know
> how to enable debug output and also how to run ITW (+browser) from the console
> to see error output. Syslog might be better and I think it's now the standard
> way how applications might log its messages.
>
>>
>>>   - 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?
>>

Not broken, as working, but broken as not doing its job - it was logging really only few, minor 
exceptions.  User needs to be told about it (or found on his own how to enable it and where to found 
the logs)

>
>>
>>> 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?
>
> We can, I'll inform you when simple demo will be created.
>
>>
>>> (**) The best way how to integrate the syslogs writer is still under
>>> research (tcp?, exec?)
>>
>> External-library?


As I wrote to jacob. All I know are incredibly big and complex. I hope to end with println int tcp 
socket...

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

Why it should be? /me being ironic, but have not found a real  answer to this question

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

Sure!

>>
>>> 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!

Not so sure... I was removing processing of printstacktrace, which I repalced (elsewhere) by the 
ItwLogger call, so I would rather keep it inside. But next time I will keep it in mind and at least 
post it to review alongised with rest.


J.