Review of Jose Armando Garcia Sancio's std.log

[Brad Roberts]

On 2/13/2012 7:50 AM, David Nadlinger wrote:
> There are several modules in the review queue right now, and to get things going, I have volunteered to manage the
> review of Jose's std.log proposal. Barring any objections, the review period starts now and ends in three weeks, on
> March 6th, followed by a week of voting.
> 
> ---
> Code: https://github.com/jsancio/phobos/commit/d114420e0791c704f6899d81a0293cbd3cc8e6f5
> Docs: http://jsancio.github.com/phobos/phobos/std_log.html
> 
> Known remaining issues:
>  - Proof-reading of the docs is required.
>  - Not yet fully tested on Windows.
> 
> Depends on: https://github.com/D-Programming-Language/druntime/pull/141 (will be part of 2.058)
> ---
> 
> Earlier drafts of this library were discussed last year, just search the NG and ML archives for "std.log".
> 
> I think getting this right is vitally important so that we can avoid an abundance of partly incompatible logging
> libraries like in Java. Thus, I'd warmly encourage everyone to actively try out the module or compare it with any
> logging solution you might already be using in your project.
> 
> Please post all feedback in this thread, and remember: Although comprehensive reviews are obviously appreciated, short
> comments are very welcome as well!
> 
> David

My 2 cents from a fairly quick scan of the docs:

1) I'm of the opinion that it should be possible to strip all log code from an app without changing it's behavior.
Having log levels that change execution flow is evil.  It's it the same class of bad practices as assert expressions
having side effects, imho.

2) Apps over a certain size (that tends to not be all that big, a few 10's of thousands of lines) tend to start to need
module based logging.  This proposal includes a set of log levels that have no concept of modularity and another
separate set that do.  The distinction seems arbitrary and limiting.

3) The conditional stuff seems cute, but I can't recall ever wanting it in anything I've done before.

4) I don't see an obvious facility for changing log parameters at runtime, unless the intent is to build a paramstring
array as if it came from command line parameters and calling parseCommandLine again.  use case: long running application
that has a configuration api or notices a config file has been updated or whatever.  Fairly common behavior.

5) The logger severity symbols part should allow more than single character tokens.  IMHO, the default should be the
full name of the severity, not just the first character.

6) Is the log system thread safe?  I see that it's at least thread aware, but what guarantees are about log entry atomicity?

7) The logger setter docs says it's an error to change the logger after a log message has been emitted.  That's going to
hurt.  It's not at all uncommon for an app to want to log some set of errors before it's potentially had enough time to
execute the code that configures the logger.  use case: reading a config file to get the logger configuration, but can't
process the config file properly.

Later,
Brad