View mode: basic / threaded / horizontal-split · Log in · Help
February 13, 2012
Review of Jose Armando Garcia Sancio's std.log
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
February 13, 2012
Re: Review of Jose Armando Garcia Sancio's std.log
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
February 13, 2012
Re: Review of Jose Armando Garcia Sancio's std.log
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
February 13, 2012
Re: Review of Jose Armando Garcia Sancio's std.log
I'd like to see a simple example of how to specify the filename of the log file.
February 13, 2012
Re: Review of Jose Armando Garcia Sancio's std.log
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
February 13, 2012
Re: Review of Jose Armando Garcia Sancio's std.log
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.
February 13, 2012
Re: Review of Jose Armando Garcia Sancio's std.log
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.
February 13, 2012
Re: Review of Jose Armando Garcia Sancio's std.log
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
February 13, 2012
Re: Review of Jose Armando Garcia Sancio's std.log
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".
February 14, 2012
Re: Review of Jose Armando Garcia Sancio's std.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.
« First   ‹ Prev
1 2 3 4 5
Top | Discussion index | About this forum | D home