BIND 10 #558: Disable log4cxx dependency in logging API

[BIND 10 Development]

#558: Disable log4cxx dependency in logging API
-------------------------------------+-------------------------------------
                 Reporter:  stephen  |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:  major    |            Milestone:  R-Team-
                Component:  logging  |  Sprint-20110222
                 Keywords:           |           Resolution:
Estimated Number of Hours:  2.0      |            Sensitive:  0
                Billable?:  1        |  Add Hours to Ticket:  0
                Internal?:  0        |          Total Hours:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  jelte => jinmei


Comment:

 > As for !RootLoggerName?, the original problem seems to be solved. But I
 now wonder whether it has to be a class.
 > :
 It doesn't have to be a class - I've changed it to a pair of functions.


 > is it really guaranteed that the symbol for the !MessageInitializer?
 object remains?
 > :
 It worked in the tests with Ubuntu and OS/X, but I guess that you are
 right and that it is not guaranteed.  The aim is that anyone creating a
 message file does not need to modify a global initialization function to
 pull in the symbols; just including the file in the link (or in a library
 that is included in the link process) is enough to make the symbols
 available.


 > Even if this approach is guaranteed to work, I don't see the need for
 defining the static
 > variable in the header file (which is in general not a good practice
 because a copy of that
 > object will be created in all translation units that include this header
 file). My experiment
 > indicates it should be sufficient if we create another .cc that has the
 following code:
 >
 >      namespace {
 >      const isc::log::!MessageInstantiator initiator(
 >          &isc::log::messagedef_cc_Tue_Feb__8_18_01_54_2011);
 >      }
 If there is no reference to "initiator" in the code, what causes the
 linker to pull in this file? (But see below.)


 > Not directly related to the main topic, but I'm concerned about the
 static MessageID variables
 > defined in messagedef.h (such as MSG_DUPLNS), too. Each translation unit
 that includes
 > messagedef.h will have a copy of MSG_xxx variables, and, depending on
 the compiler/linker
 > optimization there may be copies of the defined strings (such as
 "DUPLNS"). Is there any reason
 > that we cannot declare these MSG_xxx as extern in messagedef.h and
 define only a single
 > instance of each of them in messagedef.cc? In my quick experience it
 seems to be possible.
 > This way we can ensure each MSG_xxx symbol is unique within the program,
 and we can possibly
 > compare these IDs by addresses, which should be much faster than
 comparison as strings.
 This was an option I suggested at the end of my comments on ticket #559.
 Since both you and Jelte are happy with the idea, I've modified the code
 to do that.

 It also solves the problem of the referencing of the !MessageInitializer -
 that can be in the same file as the definitions of the the message
 symbols.  Since the file will be included at link time to resolve the
 message symbol references, the !MessageInitializer object will also be
 included.

 > I've made some trivial editorial/style fixes directly on the branch and
 pushed the changes.
 OK

 > I don't (yet) fully understand the expected usage, but is each app (e.g.
 b10-auth) and (some
 > of) BIND 10 library (e.g. libdns++) is expected to create its own
 message definition and
 > register it via !MessageInitializer?? If so, are these specific
 definitions expected to override
 > the default MSG_xxx defined in lib/log/messagedef.h or expected to be
 different sets of IDs? If
 > it's the latter, should we worry about the current default definition
 (DUPLNS, etc) can cause
 > conflict?
 We can do it either way - I was leaning more to the idea that each
 facility would define its own symbols in a local file. (If we include an
 explanation of each message in the message file - the aim being that a
 "system messages" manual can created for the user - a single file could
 get very large.)

 Of course, the problem with this is that the intended usage is that each
 message ID (not the symbol used to reference it - these can be in
 different namespaces) should be unique in the program.  Splitting the
 message IDs between files runs the risk of using the same one twice.  This
 is difficult to detect at compile time, but not at run-time.  When the
 !MessageInitializer object loads a message file into the global
 dictionary, it records any duplicate IDs found.  This list is output when
 the logger system is initialized (in the init() function in
 logger_support.cc).  So providing the server logs are checked during
 tests, it should be possible to identify duplicate IDS.

 (as an aside, since message declaration (.h) and definition (.cc) files
 are generated from the message file, I see only the message file being
 checked into git.  If the BIND10 build procedure were to build the message
 compiler first, it could run through the BIND10 source tree and generate
 the declaration and definition files from each message file before
 starting the build.)

 > It seems the return value of RootLoggerName::getName() could be 'const
 string&', and it
 > would be a bit (or much, depending on the std::string implementation)
 faster.
 >
 > same comment applies to MessageDictionary::getText()
 Done.


 > do we have performance consideration? Logger::xxx() methods seem to
 involve expensive string
 > comparison and many object copies and may have substantial performance
 impact.
 Isn't this premature optimisation? :-)

 Seriously, with the backing out of log4cxx, this implementation is a quick
 fix to get an API in place (so we can start adding logging to the code) in
 time for the next release.  At the F2F meeting in Prague, we need to
 decide if we are going to write our own logger implementation or choose
 some other package.

 However, to address the concerns for now, I've tried to improve
 performance.  Most of the string comparisons now only occur if options are
 set for a specific logger.  If all loggers use the default (root) options,
 there should only be a few integer comparisons.


 > why does MessageDictionary::globalDictionary() return a pointer instead
 of a reference?
 Historical accident, now changed.

 > in logger.cc, I'd try to avoid relying on macro.
 > :
 Fair enough.  I was trying to put as much as possible in the
 implementation-independent logger.cc.  But I've followed your suggestion
 and removed the macros, putting more into the implementation.

 > minor editorial nit: some of the copyright years may be incorrect.
 Shouldn't all these be 2011?
 Changed for files created in 2011. (And as a side-effect, I've updated my
 new file template.)

-- 
Ticket URL: <https://bind10.isc.org/ticket/558#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development