Skip to content

Conversation

@ctrueden
Copy link
Member

As discussed with @fjug in Dresden:

  1. The LogService now sports a spiffy callback mechanism, so that you can register listeners for log events.
  2. You can now create any number of Logger channels keyed by name, of which the LogService itself functions as the "default" one, preserving the convenience of the previous paradigm.
  3. The ConsoleService now forwards all stdout and stderr messages to special channels of the LogService named "stdout" and "stderr" respectively.

@fjug Please review and test with your logging UI. I will merge once you sign off on it.

@ctrueden
Copy link
Member Author

ctrueden commented Dec 22, 2016

One thing I did not do, and which needs testing: We want the ability to filter stdout and stderr console events based on the producer (i.e., generating class/package). We need to inspect the stack trace to figure that out. The LogService already has this feature, so probably, nothing new needs to be implemented. We can simply use the stdout and stderr log channels. However, the callingClass() method of AbstractLogService almost certainly needs tweaking. We want to ignore anything in org.scijava.log and org.scijava.console completely. But we also don't want the org.scijava.log package to reference org.scijava.console at all. Consider how best to achieve this...

And one other needed improvement: the ConsoleService should be smart enough to recognize when an exception comes down the pipeline of stderr, and transform it into the associated exception of the emitted log message. Kinda nuts, but super useful to reconstruct failures. Definitely needs unit testing, though!

@ctrueden
Copy link
Member Author

ctrueden commented Dec 22, 2016

Remaining action items:

  • @fjug try the new API. Update his Swing UI logging panels to: 1) not use logback explicitly at all; instead: 2) use the above; 3) report back on what works, what doesn't, etc.
  • Iterate on the design. Think about how to fix org.scijava.ui.console package to instead be org.scijava.ui.log. Deprecate the old classes.
  • Once it's ready: migrate Swing UI logging panels into scijava-ui-swing for everyone's profit.

We are going to add support for multiple Logger objects in the same
SciJava context. The LogService instance will itself be the default
Logger, but there will be others as well.
The default level went back and forth between INFO
and WARN a couple of times, but it is currently INFO.
This change eliminates the various protected log methods of
AbstractLogService, in favor of a single method:

    alwaysLog(int level, Object message, Throwable t)

All other logging methods in the Logger interface now have default
implementations which funnel to this method, which should make it
simpler to implement this interface.

This change also introduces new methods for logging information at any
arbitrary log level:

    log(int level, Object message)
    log(int level, Throwable t)
    log(int level, Object message, Throwable t)

All level-specific logging methods now lean on these general-purpose log
methods, which do the level threshold check, and then call alwaysLog.
The logic in AbstractLogService is now generalized in a DefaultLogger
implementation which implements Logger. The AbstractLogService now
simply keeps an instance of DefaultLogger to which it delegates
everything, apart from calls to alwaysLog, which is marks abstract.

To better understand the motivation for this change, see next commits.
Now Logger extends the Named interface.

This will be useful shortly.
We sometimes need Logger instances other than the LogService itself.
All the logic of AbstractLogService is hereby migrated to a separate
DefaultLogger class, instances of which are created on demand in
response to LogService#channel(String) calls. Created Logger instances
are stored in a hash by name.

The AbstractLogService maintains a special instance of DefaultLogger
as a channel called "default", which receives the log events fed
to the LogService itself. Alternately, consuming code can call
LogService#channel(String) to obtain a different channel to which
information can be logged.
For performance reasons, we do a couple of tricky things:

1. We do not use a synchronize lock in the notifyListeners method;
   rather, we keep a cached copy of the listeners as an array reference
   and use that; see also 033a921.

2. In contrast to normal Java convention, we do not wrap the message
   details in a LogEvent. This tactic avoids the overhead of creating a
   new object every time a message is logged.

With this API, it is now possible for interested parties to receive
notification of any log messages of interest.
The LogLevel#prefix(int) method hasn't been released yet,
so we can still modify the behavior before it goes gold. ;-)
I usually hate to do this, since it hurts startup time of a context.
However, we really need to register as a listener for console events
up front, so they can be forwarded to dedicated logger channels of
the LogService (see next commit).
Specifically, we publish stdout messages to a dedicated "stdout" logger
channel, and stderr messages to a dedicated "stderr" logger channel.

Note that because console messages can contain partial lines, we use a
simple heuristic to avoid publish stupid and/or empty log messages:

1) We accumulate console messages to a buffer;
2) Whenever we see a newline, we flush it;
3) Whenever the message ends in a newline, we trim one.

See next commit for a test which illustrates why this works OK.
We now forward console messages to special logger channels of
the LogService. This test ensures that that actually happens.
@maarzt
Copy link
Contributor

maarzt commented Feb 23, 2017

Hey @ctrueden, I'm sorry for starting a fork of this branch before contributing to this discussion. It was simply the workflow I used for the plot-service branch. But discussions are important so let's start:

@maarzt
Copy link
Contributor

maarzt commented Feb 23, 2017

At first, I think we should clarify some user stories this branch is dedicated to:

  1. User story: While using ImageJ / Fiji log messages should be easy accessible in a central logging window. The user should no be disturbed by multiple log, console and third party log windows popping up.

  2. User story: Plugin A uses the functionality of plugin B. Plugin A does not want plugin B to open a logging window. But plugin A still wants plugin B's log messages to be visible in a central logging window or in the logging panel of plugin A.

@maarzt
Copy link
Contributor

maarzt commented Feb 23, 2017

What does not work:

  • I cannot write a logging window that listens to all logging channels. Currently this requires the logging window to register for all logging channels. The problem: This process needs to be repeated again and again, as new logging channels might be added. I see two possible solutions: 1) Having a notification when new channels are added. 2) Providing a listener interface, that is fed with the log messages of all channels (This needs an additional parameter, to figure out, which channel a log message belongs to.)

  • I think we shouldn't forward stdout and stderr to logging channels. Logs and streams are different concepts: logs are organized in messages, streams are not. Output to streams is pre formatted and needs a mono spaced font to be displayed correctly. Logs are not pre formatted and can be displayed with any font.
    I think two different Listener interfaces for two different concepts was already very good: Listen to log messages by registering to LogService, and listen to stdout/err by registering to ConsoleService.

  • User story: I want all channels, log messages to be forwarded to stdout/stderr as I want to see them in my terminal or IDE. But I don't want my plugin that listens to the ConsoleService to get the log messages, as it already gets these messages by listening to LogService.
    Currently there is no way to write log messages to stdout/stderr while not forwarding them to the listener of ConsoleService. Currently only the messages of the default channel are forwarded to stdout/stderr.

  • It would be useful to have an upper limit for log levels. This would simplify algorithms that process log messages depending on their log channel. We should consider making LogLevel an enum.

@maarzt maarzt mentioned this pull request Feb 28, 2017
@ctrueden
Copy link
Member Author

ctrueden commented May 5, 2017

Closing in favor of #255.

@ctrueden ctrueden closed this May 5, 2017
@ctrueden ctrueden deleted the log-revamp branch May 5, 2017 19:28
@ctrueden ctrueden mentioned this pull request Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants