Review of Jose Armando Garcia Sancio's std.log

[Jose Armando Garcia]

On Thu, Feb 16, 2012 at 7:21 AM, Jonathan M Davis <jmdavisProg at gmx.com> wrote:
> On Monday, February 13, 2012 16:50:04 David Nadlinger wrote:
>> Please post all feedback in this thread, and remember: Although
>> comprehensive reviews are obviously appreciated, short comments are very
>> welcome as well!
>
> Why does vlog even exist? It's a needless complication IMHO. Let the log
> levels manage what does and doesn't get logged. I see no reason to add the
> concept of verbosity on top of that. It's a needless complication.
>

Needless? Man, you really make some "strange" comments. I believe that
this is better served by you asking for the motivation for vlog
instead of make a useless comment like the one above. The reason why
vlog exist is to provide users with the ability to enable/disable
logging at a package or module level. Lets say that for a particular
load you are seeing that module X is not behaving correctly. The next
time you run your application (or you can do this at runtime if you
have coded your application correctly) you can enable verbose logging
in this module by using the --v and --vmodule command line options. I
have tried to explain this in the properties
Configuration.maxVerboseLevel and Configuration.verboseFilter. Let me
know if it still no clear and if I should expand the documentation.

> Also, _please_ add a debug level. Personally, I'd argue for simply copying
> syslog's levels and matching them, since ideally any logging on Linux would be
> going to syslog anyway. But there are good reasons to have messages beyond
> info. I sure wouldn't want _all_ messages which don't indicate a problem in
> the app to be marked as info. For instance, what if I want to have info
> displayed in release mode but want greater verbosity in debug mode?

debug info("More information in debug mode");

I think it is very helpful that instead of just suggestion more level
we try to see how we can use this module and the power of D to do what
you want. If we find that it is not possible or clunky then we can
talk about adding more functionality.

> I'd need
> another log level which isn't there. Using the concept of verbosity to try and
> handle this is a needless complication. syslog has
>
> #define LOG_EMERG       0       /* system is unusable */
> #define LOG_ALERT       1       /* action must be taken immediately */
> #define LOG_CRIT        2       /* critical conditions */
> #define LOG_ERR         3       /* error conditions */
> #define LOG_WARNING     4       /* warning conditions */
> #define LOG_NOTICE      5       /* normal but significant condition */
> #define LOG_INFO        6       /* informational */
> #define LOG_DEBUG       7       /* debug-level messages */
>
> And I'd like to at least see notice and debug added.
>
> While we're at it, what's the point of dfatal? Why on earth would a _fatal_
> condition not be fatal if it were in release mode if it were fatal in debug
> mode? Is it fatal or not? It seems to me like another needless complication.
>

Barely a complication. dfatal looks as follow:

debug alias log!(Severity.fatal) dfatal; /// ditto
else alias log!(Severity.critical) dfatal; /// ditto

> If you're going to have write, then have writef, not format. Then it's
> actually consistent with our normal I/O functions.

Good suggestion. Will do.

> Also, do writef and format
> automatically append a newline? If so, then they should be writeln and
> writefln.
>

Technically, no. The module was abstracted with a frontend and a
backend. At a very high-level the front does filtering based on
compile time and run time configuration options. The backend is
basically responsible for persisting messages. It just happens that
the only backend implementation that we have writes to a human
consumable file hence the newline but it is possible that users are
going to want to store this data in persistent storage where each
logging event is not separated by a newline.

> Rich is a horrible name IMHO. It says nothing about what it actually is or
> does. I'm not sure what a good name would be (BoolMessage?, LogResult?), but
> Rich by itself is very confusing and utterly uninformative.
>
Yeah. I don't like Rich. Let me think about a better name. Thanks for
the suggestions!

> And why does Configuration's logger property throw if you set it after a
> logging call has been made. Is it really that inconceivable that someone would
> swap out loggers at runtime? Or is the idea that you'd swap out the
> Configuration?
>
The idea is not to swap out the Configuration but to instead be able
to reset each property. There is no technical reason why you can't
replace the Logger. I just didn't think this is something the users
would want to do in practice. Again this goes beyond technical reason
is more of the operational consequence if you allow this. For example:

1. You started logging to /tmp/application/...log...
2. You swapped the logger at runtime to start logging to syslog

You have a bunch of important data in /tmp/application/...log... what
are you going to do with it? Think of this like a stream flowing into
a lake and at some point you want to route the stream to the ocean.
What is the state of the lake after the routing change?

I would also like to add that this is a restriction that we can remove
in the future. I am honestly a little hesitant to remove it now
without giving it a little bit of more thought. Thoughts?

Thanks!
-Jose

> - Jonathan M Davis