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

[BIND 10 Development]

#555: C++ Logging - multiple destinations and configuration
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110614
                   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 stephen):

 * owner:  stephen => vorner


Comment:

 Thanks for the very comprehensive review:

 > Is there any reason why the !LoggerManagerImpl is in separate file?...
 You are right about optimisation, but in this case the code has been put
 in a separate file because it very cleanly separates the underlying
 implementation from the interface.  If we need to replace log4cplus, then
 (theoretically) we should only need to replaced the xxx_impl.* files and
 nothing else.  The extra overhead caused by lack of optimisation should be
 negligible compared with the processing done by the various functions.

 > In several conversion functions, when it gets invalid input, it returns
 something anyway and logs a warning. Is there any reason for such
 behaviour?
 It is for robustness - I didn't want BIND10 falling over if someone made a
 spelling mistake in the configuration file.  (Also, it follows the Jon
 Postel dictum: "Be conservative in what you send and liberal in what you
 accept".) However, in some places no warning was logged and the error
 silently ignored.  In these places I've logged a message before coercing
 the value. (Throwing an exception was an option, but is many cases is
 likely to result in program termination.  Logging - at least to the
 console - is enabled early on in the program and so is used where
 possible.)

 > 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?
 It should - it was put there temporarily during testing and never got
 moved. (In fact, being empty it has been moved to logger_manager_impl.h.)


 > In LoggerManager::init, the comment „Log using the logging facility root
 logger“ isn't right. It logs using a logger with name "log".
 An example of where the code was updated but not the relevant comment.
 Removed the word "root" - it logs using the logging facility logger.

 > I don't understand this comment: „There are - sort and remove any
 duplicates“ ‒ what does the „There are“ mean?
 It referred to the previous comment ("Check if there were any duplicate
 message IDs"), but the comment has been clarified.

 > The comment and code don't match:
 > !// File successfully read, list the duplicates
 Comment has been updated.


 > When logging the exception, you use a switch to specify number of
 arguments:
 > :
 > 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).
 The original was a direct replacement for the time when a varadic argument
 list was used.  The method you suggest works (although I had to add the
 assignment operator to the Formatter class) and has been implemented.  In
 doing this I noticed that there is no check that a message is generated if
 there is a problem opening a local message file, so one was added.  The
 test also checks that the above code formats the message correctly.

 > 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?
 Yes, and this has been done.


 > 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?
 The consequences have been noted in the header of the init() functions in
 !LoggerManager and !LoggerManagerImpl.  There will be no crash, just
 messages written to stderr that log4cplus has not been initialized.

 > And, it would be nice to say that the file parameter might be NULL.
 Done, although it is recommended that this not be the case.

 > What does the second sentence mean?
 The comment has been updated.

 > logger_manager_impl.cc talks in two comments at the beginning of the
 file about reset, but there's no reset near it.
 One of the comment blocks has been removed.  The reset talked about at
 that point is in the call to log4cplus's
 getDefaultHierarchy().resetConfiguration()

 > class !UnknownLoggingDestination : public isc::Exception { ‒ shouldn't
 it be in some header file? How can someone catch it?
 Well noticed.  The definition of the exception has been moved to the
 public header file logger_manager.h

 > 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:
 Agreed, but I too think we should defer it.

 > 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.
 I suggest leave it as is now it has been written.  Each test checks one
 particular item and is very simple.  I don't think the added complexity is
 worth changing it.

 > This might be wrong, there's no setting of the sentinel char and no
 strncpy:
 It was - comment removed.


 > 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
 I think that the race condition is such that two calls to tmpnam at
 exactly the same time might generate the same name.  By creating the file
 in the process, the condition is avoided.  I'm not sure if deleting the
 file re-introduces the race condition, but I've taken that out of the
 !SpecificationForFileLogger class.  Deleting the file before the test now
 happens in just one place, where the ability to create the log file if it
 does not exist is checked.  In other places I've followed your suggestion
 - the file is not deleted and messages are appended to it.


 > 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?
 > :
 > The same for !OutputOptionTest.
 They were just place holders.  However, you are right - I've removed them
 and converted the TEST_Fs to TESTs

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