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

On 2/13/12 4:50 PM, David Nadlinger wrote:
> https://github.com/jsancio/phobos/commit/d114420e0791c704f6899d81a0293cbd3cc8e6f5
>
> Docs: http://jsancio.github.com/phobos/phobos/std_log.html

A few small points from a first pass through the docs:

Spelling nits:
 - potion -> position
 - enviroment -> environment
 - arguements -> arguments
 - explictly -> explicitly
 - fileter -> filter
 - persiste -> persist
 - genereated -> generated
 - brakets -> brackets
 - paramater -> parameter
 - compuration -> computation

The introductory section needs some copy editing, e.g.:
 - In the first paragraph, every sentence starts with »The module«
 - disabled/enabled -> disable/enable
 - »In the other« -> »in the order«

You define the Severity enum members starting with fatal as 0. Why not the other way round – so that severityA < severityB would do what you (or at least I) would expect?

Include a cross-reference to log() in the Severity comment?

Maybe clarify the meaning of »can be logged« (i.e. the severity level is not completely disabled) in the LogFilter docs?

Are the explicit to!string calls in the first LogFilter example required?

config.logger: First line is missing a full stop, »The default value a FileLogger.«, »change and configure« (where is the difference?)

»Create a configuration object based on the passed parameter.« is slightly misleading, because parseCommandLine() doesn't actually create an object.


I'll post a more in-depth review later.

David

On Monday, February 13, 2012 17:49:49 David Nadlinger wrote:
> You define the Severity enum members starting with fatal as 0. Why not the other way round – so that severityA < severityB would do what you (or at least I) would expect?

syslog defines 0 (LOG_EMERG) as the strongest and 7 (LOG_DEBUG) as the weakest. The greater the number, the more logging output, you're going to see. So, he's following syslog in that respect, though he doesn't have as many log levels. He should probably add at least debug. He also aliases them to module-level symbols for some reason, which is a big no-no IMHO.

- Jonathan M Davis

I'd like to see a simple example of how to specify the filename of the log file.

A first quick observation:

I vote for a debug severity level. Then make that default to the template parameter for log:

template log(Severity severity = Severity.debug)

That would make it nice for good old print debugging.

log("This is a dbg message");

/Jonas

On Feb 13, 2012, at 12:44 PM, jdrewsen wrote:

> A first quick observation:
> 
> I vote for a debug severity level. Then make that default to the template parameter for log:
> 
> template log(Severity severity = Severity.debug)
> 
> That would make it nice for good old print debugging.


I think that's what vlog is for, it just isn't particularly well documented.

Log levels "debug" and maybe also "trace" would be useful, but I see that vlog(n)() is meant for that purpose. I would just prefer explicit names instead of just numbers.

Is there a compelling reason why formatted logging is not the default? I find that most logging calls in practice use formatted output, and the only overhead would be searching once through the format string in the case of format placeholders.

A predefined logger for OutputDebugString on Windows would be useful - or maybe it could be used instead of stdout at least for non-console applications.

One kind of log writer that I have in my code is one that outputs a nicely formatted HTML file with built-in JavaScript to be able to filter messages by priority or module. Maybe this is too much for a standard library implementation though.

Support for multiple log writers can be useful (e.g. logging to a file + logging to stdout or to a log control inside of the running application). Of course, one can also simply write a "MultiDispatchLogger"...

A format option to log the thread name instead of just the ID.

On 14 February 2012 10:17, Sönke Ludwig <ludwig@informatik.uni-luebeck.de> wrote:
> Log levels "debug" and maybe also "trace" would be useful, but I see that
> vlog(n)() is meant for that purpose. I would just prefer explicit names
> instead of just numbers.
>
> Is there a compelling reason why formatted logging is not the default? I find that most logging calls in practice use formatted output, and the only overhead would be searching once through the format string in the case of format placeholders.
>
> A predefined logger for OutputDebugString on Windows would be useful - or maybe it could be used instead of stdout at least for non-console applications.
>
> One kind of log writer that I have in my code is one that outputs a nicely formatted HTML file with built-in JavaScript to be able to filter messages by priority or module. Maybe this is too much for a standard library implementation though.
>
> Support for multiple log writers can be useful (e.g. logging to a file + logging to stdout or to a log control inside of the running application). Of course, one can also simply write a "MultiDispatchLogger"...
>
> A format option to log the thread name instead of just the ID.

I agree that there needs to be an easy to use format-based logger,
even if its just logf template that calls format on the filter, since
writing `log!(info).format("my string %s", s);` looks bad compared to
`logf!info("my string %s", s);`

Looking through the docs, there needs to be a better indication of configuration in the primary example, since you don't even mention it, I thought it might not exist.

A built-in console logger should probably be available, one that writes to stdout at the very least, since I often need log messages for debugging, other loggers can be build on top of the Logger interface, separate from the library, but I imagine that many people would want a console logger and doing it in the standard library will prevent too much repeated work. It would also be an idea to make it the default, but at this point I can't really separate out my practices from what I think most people do.

A debug-level log would be nice, but I don't really care either way.

A built-in MultiDispatchLogger (or similar) would be nice, but I don't
think it really matters.

Otherwise, I think it all looks good; fairly simple to use and configure, ability to override the defaults if needed, ability to swap out backends at runtime. The docs need work, but docs always need work :P.

It might not mean much, but this gets my approval.

James Miller

On Monday, 13 February 2012 at 15:50:05 UTC, 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

Good work.

One suggestion. Instantiating a template for each log rather verbose for such common thing. I suggest:

(Just to demonstrate)
alias global_logger!sev_info info;
alias global_logger!sev_warning warning;
alias global_logger!sev_error error;
alias global_logger!sev_critical critical;
alias global_logger!sev_dfatal dfatal;
alias global_logger!sev_fatal fatal;

As we are pulling severity levels to global namespace anyway, this will save us some verbosity and the keyword "log".

On Mon, Feb 13, 2012 at 2:49 PM, David Nadlinger <see@klickverbot.at> wrote:
> On 2/13/12 4:50 PM, David Nadlinger wrote:
>>
>>
>> https://github.com/jsancio/phobos/commit/d114420e0791c704f6899d81a0293cbd3cc8e6f5
>>
>> Docs: http://jsancio.github.com/phobos/phobos/std_log.html
>
>
> A few small points from a first pass through the docs:
>
> Spelling nits:
>  - potion -> position
>  - enviroment -> environment
>  - arguements -> arguments
>  - explictly -> explicitly
>  - fileter -> filter
>  - persiste -> persist
>  - genereated -> generated
>  - brakets -> brackets
>  - paramater -> parameter
>  - compuration -> computation
>
Thanks. I cleaned this up. I'll pass the rest through a spellchecker.

> The introductory section needs some copy editing, e.g.:
>  - In the first paragraph, every sentence starts with »The module«
>  - disabled/enabled -> disable/enable
>  - »In the other« -> »in the order«
>

I fix it some what. Still not a big fan of the current introductory section. My current thinking for introductory section is to write something small that every developer should read. Probably something example driven. If they need details into how to configure the module then they can read the rest of the document. Thoughts?

> You define the Severity enum members starting with fatal as 0. Why not the other way round – so that severityA < severityB would do what you (or at least I) would expect?
>

Internally I wanted to use the same representation for severity as I would use for verbosity. I wanted 0 to represent the highest importance and increasing numbers to have lower severity/importance. This would allow users freely pick lower and lower verbose message as they develop their application without having to worry how the application is configured in the deployment/production environment.

> Include a cross-reference to log() in the Severity comment?
>
Done

> Maybe clarify the meaning of »can be logged« (i.e. the severity level is not completely disabled) in the LogFilter docs?
>
Thanks. This part of the document was some what confusing. It should be a little better now.

> Are the explicit to!string calls in the first LogFilter example required?
>
> config.logger: First line is missing a full stop, »The default value a FileLogger.«, »change and configure« (where is the difference?)
>
No, it is not. Fixed.

> »Create a configuration object based on the passed parameter.« is slightly misleading, because parseCommandLine() doesn't actually create an object.
>

Good catch. Fixed. I have been thinking of changing the signature of the Configuration classes to look more like a builder by using the "with..." pattern in the method signature. Thoughts?
>
> I'll post a more in-depth review later.
>
> David

Thanks a lot. Your comments were helpful.