BIND 10 #555: C++ Logging - multiple destinations and configuration

[BIND 10 Development]

#555: C++ Logging - multiple destinations and configuration
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110531
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  logging                            |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  8.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => stephen


Comment:

 Hello

 I have some comments, but I believe none of them are serious, mostly
 details:
  * Is there any reason why the `LoggerManagerImpl` is in separate file?
 The class could be in the same C++ file as the `LoggerManager` class,
 which would mean that there's no public header file for the class and the
 calls to the class can be insined by the compiler if it sees it fit (which
 it should, as most of the methods in `LoggerManager` are only wrappers).
  * In several conversion functions, when it gets invalid input, it returns
 something anyway and logs a warning. Is there any reason for such
 behaviour? I believe it should rather throw an exception instead of
 silently ignoring invalid input (because one might do a typo, say „DEGUB“
 and keep hunting down why the hell it doesn't log much). Generally I
 believe the BIND10 is written with the idiom „If you have to fail, fail
 loudly and fail as soon as possible“. Moreover, when it says the value
 must be one of "DEBUG", "INFO", ….
  * If we ignore the first point for a moment, why is {{{void
 LoggerManagerImpl::processEnd() {}}}} in `logger_manager.cc`? Shouldn't it
 be in `logger_manager_impl.cc`?
  * In `LoggerManager::init`, the comment „Log using the logging facility
 root logger“ isn't right. It logs using a logger with name "log".
  * I don't understand this comment: „There are - sort and remove any
 duplicates“ ‒ what does the „There are“ mean?
  * The comment and code don't match:
 {{{#!C++
 +        // File successfully read, list the duplicates
 +        MessageReader::MessageIDCollection unknown =
 reader.getNotAdded();
 +        for (MessageReader::MessageIDCollection::const_iterator
 +            i = unknown.begin(); i != unknown.end(); ++i) {
 +            string message_id = boost::lexical_cast<string>(*i);
 +                logger.warn(MSG_IDNOTFND).arg(message_id);
 +        }
 }}}
  * When logging the exception, you use a switch to specify number of
 arguments:
 {{{#!C++
 switch (args.size()) {
     case 0:
         LOG_ERROR(logger, ident);
         break;
 }}}
    The current implementation actually supports a cycle to provide
 arbitrary number of arguments. It should AFAIK work if you save the result
 of logger.error to variable and then call .arg on it as many times as
 needed (to be implementation detail agnostic, replacing the variable with
 it's result each time, but it would currently work without it as well).
  * This comment is probably true, but it's misleading (it says what
 happens inside, but sounds like there would be no more logging after
 this).
 {{{
 +    /// Given a list of logger specifications, disables all current
 logging
 +    /// and resets the properties of each logger to that given.
 }}}
   Would it be better to say something like it replaces configuration and
 sets it to the new one?
  * `LoggerManager::init` says `This must be the first logging function
 called in the program.` What happens if it isn't?
    Like if some object is statically initialized and uses a function in
 it's constructor which does some logging, therefore
    it's called before main starts, will it crash, or will it just log into
 some default destination?
  * And, it would be nice to say that the file parameter might be NULL.
  * What does the second sentence mean?
 {{{
 /// Initializes the processing of a list of specifications by resetting
 all
 /// loggers to their defaults.  Note that loggers aren't removed as unless
 /// a previously-enabled logger is re-enabled, it becomes inactive.
 }}}
  * `logger_manager_impl.cc` talks in two comments at the beginning of the
 file about reset, but there's no reset near it.
  * `class UnknownLoggingDestination : public isc::Exception {` ‒ shouldn't
 it be in some header file? How can someone catch it?
  * This isn't too extensible. Maybe we don't need to do anything about it
 now, but in future, we might want to add plugins or something to add
 logging destinations:
 {{{
     switch (i->destination) {
         case OutputOption::DEST_CONSOLE:
 }}}
  * Console test ‒ is there a need to use temporary files? Pipes could be
 enough. Or, alternatively, both stdout and stderr could be redirected at
 once, requiring only one run of each testcase instead of two.
  * This might be wrong, there's no setting of the sentinel char and no
 strncpy:
 {{{
     // Get prefix.  Note that in all copies, strncpy does not guarantee
     // a null-terminated string, hence the explict setting of the last
     // character to NULL. ‒ kde?
 }}}
  * Well, the compiler might warn because there's a race condition. Using
 more complicated code with the same problem to only silence the compiler
    seems wrong. Is it really needed to delete the file? Wouldn't closing
 be enough?
 {{{
     // The compiler warns against tmpnam() and suggests mkstemp instead.
     // Unfortunately, this creates the filename and opens it.  So we need
 to
     // close and delete the file before returning the name.  Also, the
 name
     // is based on the template supplied and the name of the temporary
     // directory may vary between systems.  So translate TMPDIR and if
 that
     // does not exist, use /tmp.
 }}}
  * What is the reason of this class? And even if you want to group tests
 together with TEST_F instead of TEST, what is the reason for the empty
 constructor and destructor?
 {{{
 class LoggerSpecificationTest : public ::testing::Test {
 public:
     LoggerSpecificationTest()
     {}
     ~LoggerSpecificationTest()
     {}
 };
 }}}
  * The same for `OutputOptionTest`.

-- 
Ticket URL: <http://bind10.isc.org/ticket/555#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development