[rfc][icedtea-web] refactored logging

[Jiri Vanek]

On 09/18/2013 02:03 AM, Omair Majid wrote:
> On 09/13/2013 10:12 AM, Jiri Vanek wrote:
>> On 09/12/2013 07:59 PM, Omair Majid wrote:
>
>>>> +    synchronized public static ItwLogger getLogger() {
>
> I am trying to think of a case where logging an object by itself
> (instead of a larger string containing a string representation of the
> object too) will be useful, but I can't come up with one.

ok then. I will do this.

>
>>> 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.
>
> Do you really want to log completely informational messages (and ~1000
> lines of them) to syslog for each javaws run?
>

It is not so much. even in verbose mode, when you imagine-out reprinted jnlp (which I do not log in 
no way)  then it is quit ok ammount (fromjava side) The c side with debug on is quite more treacherous.

> If you do, then I think we really need to take a step back and consider
> the purpose of logging to syslog, because I don't think throwing (by
> default) lots of (for all intents and purposes) useless messages in
> there is helping anyone.
>
> If you want to filter messages and decide what goes into syslog, then
> you need a mechanism for doing so. A debug level seems like the obvious
> solution for this but if you have others, I would like to hear about
> them too.

fair enough
>
>>> (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
>
> But if I run javaws 2 times a day, I have already tripled the volume of
> my syslog. If no errors happened, then all this information is useless.
> I don't know which is worse, lots of useless information or useful
> information missing.

This stil needs ot be investigated. But yes, your points are valid.

>
>>> Maybe we should think about making these methods more semantic and only
>>> syslog'ing those errors that are important.
>>
>> Which are which??? :((
>
> Even if there's no way to find this out now, I think it's worth working
> towards this in the long run.

Exactly.

---

In all cases I think you took the review maybe to much precise.
My intention in this patch is not to provide new logging, but to create bottleneck, moreover 
compatible with current system.

Although I agree that my api was not perfect at all for new loggin but it was supposed to be an 
gateway to another refactorings which have to come.

But - to negotiate may above - better api now, much less work later.



>
>>> 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)
>
> Please don't. There's no reason why -about has to use the logger for output.


no no, you misunderstood me a bit - I do not log those messages. Those wraper methods are ensuring, 
that the "standard output" is going to same standard output as is used by ItwLogger for "standard 
output" reprint

So I'm still for keeping them inside, so the messages will coem to correct stream (se eg trerrible 
method of redirectStreems (under refactoring right now)

>
> In fact, a logger which logs messages is the last place I would except
> to see formatting methods (like printError vs printErrorLn).

As I told, those are not logging methods. Those are ensuring methods that message is going where it 
should go.
>
>> 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.
>
> I think the (slightly) older-and-wiser-you are right. While it's good to
> have debugging messages go to logger, not everything needs to (or even
> should) go to the logger. If you really want to have all outputs go to
> logger, why not have swing widgets output to logger too?

What do yo think by swing wdgets? I would like to log itw messages.

But I got the rebuke, level will be necessary.

>
>> diff -r 6124fd87eaba tests/netx/unit/net/sourceforge/jnlp/splashscreen/parts/JEditorPaneBasedExceptionDialogTest.java
>> --- a/tests/netx/unit/net/sourceforge/jnlp/splashscreen/parts/JEditorPaneBasedExceptionDialogTest.java	Wed Sep 11 12:05:44 2013 +0200
...
>> -        Assert.assertTrue("message from dialog mus be same as pattern", d4.getMessage().equals(s4));
>> +        Assert.assertTrue("message from dialog must be same as pattern", d1.getMessage().equals(s1));
...
>
> Please go ahead and commit this change as a separate patch. It's
> unrelated to everything else and good enough that it go in right now.

pushed

>
>> diff -r 6124fd87eaba netx/net/sourceforge/jnlp/util/ItwLogger.java
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/netx/net/sourceforge/jnlp/util/ItwLogger.java	Fri Sep 13 15:41:56 2013 +0200
>
>> +    public static enum StdStream {
>> +
>> +        OUT, ERR, BOTH
>> +    }

This was mentioned for replacement of various stderr/stdout
>
> Stupid question: is there any output from icedtea-web that we want to go
> do stdout? Everything I can think of is basically an error message and
> probably should go to stderr.

I think that info message should go to stdout, warning messages to std out and err, and errors and 
exceptions to std_err
>
> If you think this makes sense, maybe we can reduce the API to two methods:
>
> logInformation(String message)
> logError(String message)

hmhmh. My current opinion is to include it to "current (new) logging levels" levels of messages:

MESSAGE_ALL - stdout in all cases
MESSAGE_VERBOSE - stdout in verbose/debug mode
ERROR_ALL - stderr in all cases (default for
ERROR_VERBOSE - stderr in verbose/debug mode

Those may change during development, especially in phase of tuning messages - some fine,finer,finest 
or similar :)

And still three checkboxes to control output:
std streams
file
system log

>
> (and maybe variants that accept Object or Throwable arguments, or maybe
> change the methods tolog(Level level, String message)).
>
> Giving user-code the option to reprint, select streams and so on is
> rather unnecessary, IHMO. And based on these two methods, we can select
> what should go into low-volume output (syslog) and high-volume output
> (terminal if -verbose is used or a icedtea-web log file), with the
> option to change policy on what-goes-where later.


Hm, I wonted to unify (internal) form of debug/verbose. But you sounds different. May you elaborate 
here on your prerequisites?
>
>> +    public void log(boolean reprint, StdStream stream, Object o) {
>> +        //log to system
>> +        logToSystemLogger(stream, o);
>> +        //reuse old logger - finally will log something
>> +        //on/off wia itw-settings (enableLogging)
>> +        logToUserFile(stream, o);
>> +        //log to streams if should
>> +        if (reprint || isDebug() || force) {
>> +            logToStreams(stream, o);
>> +        }
>> +    }
>
> There's a probably a better object-oriented design possible. Have you
> thought about making a tiny logger-implementation-interface that syslog,
> stderr-log and file-log can emulate? We will be able to unit test each
> logger-implementation in isolation too. This main class can simply
> compose them.
>
> It should be much easier to add additional log-implementations based on,
> say, whether -verbose is used or not.

I see. I will elaborate here a bit
>
> Have you seen TeeOutputStream?

If you mean the apache one or general ide of it then yes.
>
>> diff -r 6124fd87eaba netx/net/sourceforge/jnlp/ParseException.java
>> --- a/netx/net/sourceforge/jnlp/ParseException.java	Wed Sep 11 12:05:44 2013 +0200
>..snip...
>> -        }
>> -    }
>> +        super(message, cause);
>> +    }
>>
>>   }
>
> Please go ahead and commit this as a separate patch too.

pushed. Thank you



Have an hope to this patch !o) It is going to evolve in time even if pushed not yet ripe :)