[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:                
----------------------+-----------------------------------------------------
Changes (by asn):

  * status:  needs_review => needs_revision


Comment:

 Replying to [comment:16 isis]:
 > Replying to [comment:13 asn]:
 > > hey isis,
 >
 > Hi asn, thanks for the review!
 >
 > > your branch contains non-logging-related changes. Can you put these in
 a different branch and trac ticket, so that we only talk about logging-
 related changes here. Examples:
 > > 03e79f9, 9aaf152, 96a01f2, 7db2fc8, 540dcf2ec02, a811d3a
 >
 > > Replying to isis:
 > >
 > > >    Ah, forgot. This also rewrites the Conf() configuration class
 (the old one was an old-style class, and would have had serialization
 problems), and with that #6127 is fixed too.
 >
 > > Can you put this in another branch and trac ticket?
 >
 > The config and extra fixes are now #9277, which is in this branch:
 https://github.com/isislovecruft/bridgedb/compare/fix;9277-config
 >
 > Replying to [comment:15 asn]:
 > > Also, can you name your module logging so that you don't need to do
 the rewriting in 07dc793..44544c8?
 >
 > Done. :)
 >
 > Here: https://github.com/isislovecruft/bridgedb/compare/feature;9199
 -improved-logging-r8

 Quick review of log.py:

 - Run your files over pylint. It finds lots of small mistakes.  For
 example, you are using `file` as a parameter, when it's a python reserved
 name. Also you have some unused imports.

 - I don't like `isinstance()` use in normal code. It makes dynamic-typed
 code even more confusing. I would prefer if types were well-defined. For
 example, I would prefer if `setLevel` were two functions; one for integers
 and one for strings; instead of a single function that tries to do both.

 - Why are you using `isinstance()` in the `__init__` of
 `BridgeDBFileLogObserver`? What else would `daily` be if it's not `bool`?
 Why don't you pass a default value of `10**6` to `max_size` (it's `None`
 now), instead of doing the `isinstance()` trick to insert a default value?

 - `_emit_with_level` seems to do more things than ''Prepend basic ISO-8601
 timestamps to log messages...''.

 - I don't understand why you have to override `LogPublisher` with your own
 class. I'm not even sure what all these methods are. I can't find any
 public documentation for `LogPublisher` on the twisted docs. What are
 functions `mute`, `showwarning` etc. used for? Why do you have to override
 them?

 - What's the deal with the `except NameError` trick in the end of the
 file? Do you want that block to only execute once? Can't you do this with
 a `is_initialized` flag for log.py? It's easier to understand IMO.

 - `warn()` docs reference `category`, `filename`, etc. which don't exist.
 Does this happen elsewhere too?

 - `1139de21bb` has a wrong commit message. You can fix this with an empty
 `git commit --squash` that I can autosquash when I merge your branch. It's
 fine if you don't fix it too.

 Other than that, the patch looks reasonable.

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