[boost] [Log] Review Results

Hi,

here are my review results. I have organized it in two parts. A
relatively short one answering the formal review questions and a second
part that lists a lot of additional details. Attached is a short
program which contains some of the tests I did.

- Should the library be accepted?
=================================
   YES, but some things should be fixed first (see also details below):

     - Compiler warnings: I get tons of them. Zero should be the goal.
     - Completeness of documentation: Some things are simply missing and
       can only be found in the code
     - Compile time for the library
     - A lot of details are listed below. Of course, I would like to see
       all of them addressed, but my vote does not depend on every single
       one of them.

- What is your evaluation of the design?
========================================
   Very flexible, very modularized. I have two concerns, both explained
   in more detail below.

   a) In some aspects, it is too flexible, I think. This makes it hard to
      document (which can be seen in the current documentation) and
      hard to test (the latter is just me guessing).
   b) It is easy to use it in the wrong way, which leads to exceptions
      and lost log records, possibly after hours of running, probably
      in some rare and potentially critical situation.
      It would be better to somehow prevent the wrong usage at compile
      time.

- What is your evaluation of the implementation?
================================================
   I looked at the code only briefly, but what I saw looked good to me
   (see below).

- What is your evaluation of the documentation?
===============================================
   Very nice examples, very well motivated tutorial. Good overview on
   details. Was good to read to get an idea of how the library can be
   used.
   But (at least for me) it was sometimes tough to combine the
   information given in examples to the things I want to do myself.
   Mostly this was because some information is missing (or too well
   hidden for me).

   I admit that I got lost on the last few pages, but that was probably
   due to me not actually experimenting with all of it.

   See below for specific remarks.

- What is your evaluation of the potential usefulness of the library?
=====================================================================
   Very useful! Logging is one of the core aspects of almost all
   software. And it is not nearly as easy as it seems at first glance.

   Thus, it is very useful to have a well designed, well documented,
   well tested and well supported log library.

- Did you try to use the library? With what compiler? Did you have any
======================================================================
problems?
=========
   I compiled the library (the version from the SVN trunk, because the
   packaged version from sourceforge did not compile on my system).

   I also did some experiments to see how easy or hard it was to transfer
   the information from the documentation to some "real" code. I stumbled
   over quite a few details, but am convinced that I could use the
   library, soon.

   The most annoying problem were the compile warnings I mentioned above.

   Ubuntu-8.04 64bit, gcc-4.2.4, boost-log from SVN trunk, boost-1.42

- How much effort did you put into your evaluation? A glance? A quick
=====================================================================
reading? In-depth study?
========================
   I spent several hours reading the documentation and several hours of
   experimenting (see details below), about 12 hours in total
   (which includes writing this review).

- Are you knowledgeable about the problem domain?
=================================================
   I wrote the log library which is being used by some 100 components in
   our company for about two years now. It is based on POCO library
   (pocoproject.org). Before writing it, I analyzed several log other
   libraries too, of course.
   So I would not consider myself an expert, but not really a newbie,
   either :-)


------------------------------------------------------------------------
MORE DETAILS
------------------------------------------------------------------------

Missing Features:
=================
Maybe I overlooked it, but I did not find the following:
   - Have several sinks share a formatter: The programs in my company
     typically produce several log files:
     - debug: Contains everything
     - warn: Contains everything which is at least a warning
     - error: Contains everything which is error or fatal
     Messages in all files are formatted in the same way. I understand
     that I could create several file sinks and each is given its
     own formatter. But that would mean that the same formatting is
     done several times.
     Is there a way to have several sinks with different filter but
     sharing the log records?

Compiling:
==========
   - Library Code:
     As mentioned in an earlier mail, it takes awfully long to compile
     the library, which is due to one file:

     formatter_parser.cpp

     The parsing is based on Boost.Spirit (classic) and takes 20 minutes
     to compile on my maschine (Ubuntu-8.04 64bit, gcc-4.2.4, Intel Quad
     Core, 2.5GHz)
     I don't know how much can be gained by switching to Spirit 2.1.
     But I am sure a way has to be found to accelerate the compilation.
     Otherwise, users will do what I did: Get nervous about the long
     compilation time and tend to press Ctrl-C before the work is done.

   - My Code:
     All of the code in our company is compiled with the following
     settings (gcc-4.2.4):
     -Wall -Wreorder -Wnon-virtual-dtor -Wno-non-template-friend
     -Woverloaded-virtual -Wsign-promo -Wextra -fvisibility=hidden

     We strive for code that compiles without warnings, but with
     boost.log, I get tons of warnings. What I have seen so far should
     be easy to get rid of. But I will not be able to use the
     boost.log library unless the warnings are taken care of.

Debugging:
==========
   - I tried to add a timestamp to each written record. I used
     core->add_global_attribute("TimeStamp",
        boost::make_shared< attrs::local_clock >());
     backend->set_formatter(
          fmt::stream
          << fmt::date_time< std::tm >("TimeStamp")
          << ": [" << fmt::attr< severity_level >("Severity")
          << "] " << fmt::message()
          );
     This compiled, but when I ran my program, I got an exception message
     which did not really tell what was wrong:
     -----------------
     terminate called after throwing an instance of 
'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::log_mt_posix::missing_value> 
 >'
     what():  Requested attribute value not found
     -----------------
     I think, at least it should tell which attribute value was not
     found. But the real reason for the exception was that
     the types of the attribute and the attribute value did not match.
   - Is there a way to get to these exceptions earlier? As of now, in the
     example above, the exception is thrown when a logger is used. This
     is much too late. I need to initialize logging and then be sure
     that logging will work for the components that use it (within
     limits of course, e.g. filesystem being full).
     But mis-configurations like above must be detected before the
     real work is done in the components using the log system. For
     instance when constructing the logger.
   - Of course, best would be to prevent such stuff at compile time
     (no idea how, though)

Design:
=======
   - Flexibility: To be honest, in some aspects, I would have preferred
     to see less flexibility, for two reasons:
     a) the documentation effort rises and I suspect that the missing
        completeness of the documentation is partly a result of
        over-flexibility
     b) the testing effort rises (I guess). It will be hard to make sure
        that everything works as expected.
     An example is log record formatting via Boost.Lamda, Boost.Format,
     String Templates, Custom-Formatting, which is all shown by example
     in the tutorial, but the detailed description of Formatters does not
     even mention String Templates or Custom Formatting.
   - Easy to misuse: If your formatter requires a severity and the
     logger does not add one to the log record or adds a severity of the
     wrong type, using that logger results in an exception, see attached
     code.
     It is unlikely that this will happen in some part of the code which
     is executed frequently, because the developer would certainly
     notice. But what if someone would add a non-matching logger at some
     rarely executed but critical code section? Not only would the
     code fail with an exception, it would also fail to log the message!

     As of now, I would have to provide some factory method which
     produces loggers for anyone who needs one. And no logger is ever
     to be constructed otherwise. Just to make sure that everybody uses
     a logger that actually produces digestible log records.
     I can live with that, but I do not consider it a good solution.

Code Quality:
=============
   - Readability: I did not read through all the code, and none in very
     much detail, but the parts I looked at, looked well organized and
     readable.
   - Names:
     - namespace trivial: I admit that I don't like that, especially
       since its content is not exactly trivial. Easy, basic or default
       maybe, I don't know. But not trivial :-)

Documentation:
==============
Very nice examples, very well motivated tutorial. Good overview on 
details. Was good to read to get an idea of how the library can be used. 
But (at least for me) it was sometimes tough to combine the information 
given in examples to the things I want to do myself.

I admit that I got lost on the last few pages, but was probably due to
me not actually experimenting with all of it.

Some things should be looked into, though:

Build process:
   - it would be nice, if the default build configuration were documented
   - I wonder which build configuration will be used when the library is
     included in boost?

Tutorial:
   - Trivial logging with filters: The "Tip" that streaming expressions
     are not evaluated if the message is filtered should probably be a
     warning of potential hazard. It is certainly a good feature, but
     the semantics of the macros are not immediately obvious so I would
     give the "Tip" a more critical nature.
   - none of the code examples is complete. They are missing the
     namespace declarations documented in the introduction, at least.
     While brevity is good, it would be very helpful to have complete,
     compiling examples in the tutorial

Missing Documentation:
   - Not all include files are mentioned. This makes it unnecessarily
     tough to follow the examples.
     For instance, boost/log/utility/empty_deleter.hpp is required to
     use logging::empty_deleter. I could not find this information in
     all of the turorial and details.
     Another, maybe even more critical example: The BOOST_LOG macro
     is defined in boost/log/sources/record_ostream.hpp
     Again I did not find that information in the tutorial.
   - I understand that I can use my own set of severity levels. It is
     well documented how to filter messages using these severity levels.
     But I have no idea how to format messages with these severity
     levels? I do not want the number, I'd like a string, of course.
     I looked it up in the trivial.hpp, but it should be part of
     the documentation.
   - Maybe I missed it, but I did not find an overview of the log
     rotation options, including a description of the placeholders in
     strings.
     Specific question: Can I tell the log rotation to take place at
     midnight (regardless of the amount of data which is being logged)?

I would like to stress this last item one more time: The examples are
excellent, but they should be accompanied by more formal tables which
show the complete set of options, e.g. which placeholders can be used in 
a filename format or a list of public (protected?) member functions of a
logger. For instance, I found the logger::strm() member in examples only.