Review of Jose Armando Garcia Sancio's std.log

[Jose Armando Garcia]

On Fri, Mar 2, 2012 at 12:56 AM, Brad Roberts <braddr at puremagic.com> wrote:
> 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:
>
Thanks for looking at the documentation.

> 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.
>
I think this depends on your point of view. One way to look at
critical("critical") is as a replacement to enforce() that logs. We
could extends critical in a way similar to enforce where you can
specify the exception that should be thrown. David, would this help
with the issues you express earlier?

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

This distinction is not a limitation of the logger API. It is a
limitation of the configuration API. In the future we can add the
option to enable error, warning, info at the module level without
breaking existing code.

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

If you want to change the log level at runtime you can just:

std.log.config.minSeverity = Severity.info;

No need to call parseCommandLine. Everything that parseCommandLine
does can be implemented by a client user. Really parseCommandLine
doesn't need to be part of the Configuration class. It is just there
for grouping and documentation.

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

My motivation for using one char is that it is easier to parse by both
computers and humans. It is also makes the framework faster since it
writes less bytes. I think we can extends this and fix this once we
have custom line formatted.

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

Yes, it is. It is probably to conservative in this regard. I need to
go back and do a lot performances improvements.

> 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.
>
This was brought up before. There is no technical reason for this and
I will remove it.

Thanks!

> Later,
> Brad