BIND 10 #176: python logging framework

[BIND 10 Development]

#176: python logging framework
---------------------+------------------------------------------------------
 Reporter:  jreed    |        Owner:  zzchen_pku                                 
     Type:  task     |       Status:  reviewing                                  
 Priority:  major    |    Milestone:  04. 2nd Incremental Release: Early Adopters
Component:  logging  |   Resolution:                                             
 Keywords:           |    Sensitive:  0                                          
---------------------+------------------------------------------------------
Changes (by shane):

  * owner:  shane => zzchen_pku


Comment:

 In general, I think this is a good start and should be merged.

 For the next release, we need to make sure we have more documentation, and
 probably a bit of discussion on a couple of the points below.

 ----

 I see some `print()` in the main function. This is probably okay, but I
 was thinking:

   * Maybe some of these should also go to the logging system.
   * Maybe these should go to stderr instead of stdout.

 In !XfroutServer's `__init__()` function, we see:

 {{{
     self._log = None
       ...
     self._log = isc.log.ModuleLogger(self._config_data.get('log_name'),
 self._config_data.get('log_file'),
 self._config_data.get('severity'), self._config_data.get('versions'),
                                          self._config_data.get('ma
 }}}

 No need to set it to None the first time! :)

 In !UnixSockServer's `shutdown()` method, don't leave the commented-out
 `pass` statement in there.

 I think the default logging should be to something other than
 /var/log/xfrout.log. Probably it should go to something like
 @@LOCALSTATEDIR@@, except defined for logging, if there is such a thing...

 ----

 In log.py:

 In the !ModuleLogger class, with the `__init__()` function, we need to
 document that to disable logging set log_file=''.

 Typo: Ture -> True

 Also, for now it is fine, but I think maybe we should consider the design
 of how the !ModuleLogger is created. Perhaps it would be easier to simply
 say something like:

 {{{
     self._log.update_config(self._config_data))
 }}}

 And have the logging stuff use the `.get()` functions for you. I'm
 thinking this would be simpler, and actually make it easier to update the
 logging. Perhaps we can make this a backlog ticket, or just put it in a
 TODO file?

 There are times when we need to only log to `syslog()`, for example.


 In add_syslog_handler, there is no way to set facility, so the comment
 should just say "LOG_USER is used", rather than "If facility is not
 specified".


 I'm not sure why we need the !NullHandler by default... if it really just
 sends to nowhere, maybe we should leave it out completely? Or does it do
 something?

 ----

 I had to change the tests to something other than /var/log to run. This is
 kind of related to the comment above about /var/log - but for testing I
 just made it go to /tmp. This is not necessarily the best solution, but it
 worked.

 It might be nice to send the output to someplace other than `stdout` when
 running the tests. This could be done by something like:

 {{{
 def setUp(self):
       ...
     self.save_stdout = sys.stdout
     sys.stdout = open(os.devnull, "w")

       ...
 def tearDown(self):
     sys.stdout = self.save_stdout
       ...
 }}}

 This will make "pytest" output a little easier to read.

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