[rfc][icedtea-web] refactored logging
Omair Majid
omajid at redhat.com
Tue Sep 17 17:03:12 PDT 2013
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() {
>>> + 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.
No. There are pros and cons of each singleton implementation. For the
purpose of testing, enum is often the wrong answer.
>>
>>> + 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.
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.
>> 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?
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.
>> (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.
>> 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.
>> 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.
In fact, a logger which logs messages is the last place I would except
to see formatting methods (like printError vs printErrorLn).
> 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?
> 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
> +++ b/tests/netx/unit/net/sourceforge/jnlp/splashscreen/parts/JEditorPaneBasedExceptionDialogTest.java Fri Sep 13 15:41:45 2013 +0200
> @@ -132,10 +132,10 @@
> JEditorPaneBasedExceptionDialog d2 = new JEditorPaneBasedExceptionDialog(null, false, ex, ec, null);
> JEditorPaneBasedExceptionDialog d3 = new JEditorPaneBasedExceptionDialog(null, false, ex, null, ai);
> JEditorPaneBasedExceptionDialog d4 = new JEditorPaneBasedExceptionDialog(null, false, null, ec, ai);
> - Assert.assertTrue("message from dialog mus be same as pattern", d1.getMessage().equals(s1));
> - Assert.assertTrue("message from dialog mus be same as pattern", d2.getMessage().equals(s2));
> - Assert.assertTrue("message from dialog mus be same as pattern", d3.getMessage().equals(s3));
> - 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));
> + Assert.assertTrue("message from dialog must be same as pattern", d2.getMessage().equals(s2));
> + Assert.assertTrue("message from dialog must be same as pattern", d3.getMessage().equals(s3));
> + Assert.assertTrue("message from dialog must be same as pattern", d4.getMessage().equals(s4));
>
> }
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.
> 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
> + }
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.
If you think this makes sense, maybe we can reduce the API to two methods:
logInformation(String message)
logError(String message)
(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.
> + 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.
Have you seen TeeOutputStream?
> diff -r 6124fd87eaba netx/net/sourceforge/jnlp/ParseException.java
> --- a/netx/net/sourceforge/jnlp/ParseException.java Wed Sep 11 12:05:44 2013 +0200
> +++ b/netx/net/sourceforge/jnlp/ParseException.java Fri Sep 13 15:42:08 2013 +0200
> @@ -30,9 +30,7 @@
> // todo: add meaningful information, such as the invalid
> // element, parse position, etc.
>
> - /** the original exception */
> - private Throwable cause = null;
> -
> +
> /**
> * Create a parse exception with the specified message.
> */
> @@ -45,44 +43,7 @@
> * cause.
> */
> public ParseException(String message, Throwable cause) {
> - super(message);
> -
> - // replace with setCause when no longer 1.3 compatible
> - this.cause = cause;
> - }
> -
> - /**
> - * Return the cause of the launch exception or null if there
> - * is no cause exception.
> - */
> - public Throwable getCause() {
> - return cause;
> - }
> -
> - /**
> - * Print the stack trace and the cause exception (1.3
> - * compatible)
> - */
> - public void printStackTrace(PrintStream stream) {
> - super.printStackTrace(stream);
> -
> - if (cause != null) {
> - stream.println("Caused by: ");
> - cause.printStackTrace(stream);
> - }
> - }
> -
> - /**
> - * Print the stack trace and the cause exception (1.3
> - * compatible)
> - */
> - public void printStackTrace(PrintWriter stream) {
> - super.printStackTrace(stream);
> -
> - if (cause != null) {
> - stream.println("Caused by: ");
> - cause.printStackTrace(stream);
> - }
> - }
> + super(message, cause);
> + }
>
> }
Please go ahead and commit this as a separate patch too.
Cheers,
Omair
--
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95 0056 F286 F14F 6648 4681
More information about the distro-pkg-dev
mailing list