[tor-bugs] #9199 [BridgeDB]: Rethink the logging of BridgeDB

[Tor Bug Tracker & Wiki]

#9199: Rethink the logging of BridgeDB
----------------------+-----------------------------------------------------
 Reporter:  asn       |          Owner:  isis          
     Type:  task      |         Status:  needs_revision
 Priority:  normal    |      Milestone:                
Component:  BridgeDB  |        Version:                
 Keywords:            |         Parent:                
   Points:            |   Actualpoints:                
----------------------+-----------------------------------------------------

Comment(by sysrqb):

 From the top, first pass, nice job. (sorry, I can be a bit nit-picky)

 - Is it necessary to add the string sub twice? (Dist.py:372)
 {{{
 +                msg  = "Got a request for bridges from %r; we already" %
 safe
 +                msg += " sent a warning. Ignoring." % safe
 +                logging.info(msg)
 }}}

 - I think you might as well just reuse `safe` again (Dist.py:375)
 {{{
 +                raise IgnoreEmail("Client was warned" %
 Util.logSafely(emailaddress))
 }}}

 - Should these be a tuple? (HTTPServer.py:99)
 {{{
 +            logging.info("Valid recaptcha from %s. Parameters were %r"
 +                         % Util.logSafely(remote_ip), request.args)
 }}}

 - Can we keep the indent? It makes the log easier to read, (Main.py:233)
 {{{
 -                logging.debug("  Appending transport to running bridge")
 +                logging.debug("Appending transport to running bridge")
 }}}

 - Also the same for this one? (Bridges.py:592)
 {{{
 -                logging.warn("  Skipping extra or-address line "\
 -                             "from Bridge with ID %r" % ID)
 +                logging.warn(
 +                    "Skipping extra or-address line from Bridge with ID
 %r"
 +                    % ID)
 }}}

 - This was confusing to read, can it be reworded? (log.py:133)
 {{{
 +    :type _stuff: A :type:None, :class:`t.p.failure.Failure`, or
 +    :exc:Exception.
 }}}
 Maybe something like
 {{{
      :type _stuff: It must be one of :type:None,
      :class:`t.p.failure.Failure`, or :exc:Exception.
 }}}
 I think the colons were screwing up my parsing of it, so if you can make
 that easier it will be good.

 - You don't actually return the level in setLevel(): (log.py:153)
 {{{
 +    :rtype: int
 +    :returns: An integer from :attr:`log.LOG_LEVEL`.
 }}}

 - the log folder is a bit confusing :(
 {{{
 +        ## this should be set outside this file by doing:
 +        ## >>> import bridgedb.log as log
 +        ## >>> log.folder = './putlogshere'
 +        global folder
 +        self.folder = filepath.FilePath(folder)
 }}}
 That's funky, but that's ok
 {{{
 +            self.logfile = logfile.DailyLogFile(filename,
 self.folder.path)
 }}}
 But this (log.py:248) won't use the updated/correct dir, will it?

 - I'm not sure `daily` in BridgeDBFileLogObserver is correct.
 In Main.py:beginLogging() we have:
 {{{
 +    lstdout = getattr(conf, 'LOG_STDOUT', True)
 +    ldirect = os.path.join(rundir, getattr(conf, 'LOGDIR', 'log'))
 +
 +    logging.folder = ldirect
 +    logging.setLevel(conf.LOGLEVEL)
 +    logging.startLogging(logfile, lstdout)
 }}}
 Then in log.py:startLogging() we have:
 {{{
 +def startLogging(file=None, *args, **kwargs):
 ...
 +        fileobserver = BridgeDBFileLogObserver(file, *args,
 **kwargs).emit
 }}}
 And in BridgeDBFileLogObserver().__init__() we have:
 {{{
 +    def __init__(self, filename='bridgedb.log', daily=False,
 +                 max_size=None, max_files=None):
 ...
 +        if not isinstance(daily, bool): daily = True
 }}}
 So I may be misreading this, but it seems like we start out stating we
 want to log to stdout and end up defining daily rotation. I guess
 BridgeDBLogPublisher().___init___() is where the value of the LOG_STDOUT
 config option should go.
 {{{
 +    def __init__(self, log_to_stdout=True):
 ...
 +        self.log_to_stdout = log_to_stdout
 }}}

 - Hm, BridgeDBLogPublisher().start() and stop() are not complementary, I
 fear this can be confusing.

 - Similar to what asn mentioned, maybe docing the "try, except NameError"
 block will be helpful. It makes sense, but it's purpose may not be obvious
 to others.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9199#comment:18>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online