[tor-bugs] #9199 [BridgeDB]: Rethink the logging of BridgeDB
Tor Bug Tracker & Wiki
blackhole at torproject.org
Tue Sep 3 22:33:57 UTC 2013
#9199: Rethink the logging of BridgeDB
--------------------------+----------------------------
Reporter: asn | Owner: isis
Type: task | Status: needs_revision
Priority: normal | Milestone:
Component: BridgeDB | Version:
Resolution: | Keywords:
Actual Points: | Parent ID:
Points: |
--------------------------+----------------------------
Comment (by sysrqb):
Replying to [comment:23 isis]:
> Replying to [comment:18 sysrqb]:
> > - 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")
> > }}}
>
> Sure. Can we change these to `'\t'`s instead?
>
That's fine with me
> > - 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)
> > }}}
>
> Yep, so long as we don't do string concatenation within calls to the
logger. See
> {{{% pydoc twisted.python.log.msg}}}
>
Sure, just remind me when I forget.
> > - 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.
> > }}}
>
>
> Yeah, the indentation was not right either. It should be:
>
> {{{
> :type _stuff: It must be one of :type:None,
> :class:`twisted.python.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.
>
> These get formatted ''much'' nicer when the Sphinx docs are built.
Yup. It looks like trac ate the indentation I added in my suggestion, too.
> >
> > - You don't actually return the level in setLevel(): (log.py:153)
> > {{{
> > + :rtype: int
> > + :returns: An integer from :attr:`log.LOG_LEVEL`.
> > }}}
>
> It doesn't need to return. It is like an `@property.setter()`, but on a
module global variable.
>
Right, I think my point was only that the docstring says it returns an
int, but the method never **return**s, so the return value will always be
None.
> > - 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)
> > }}}
...
> Hmm. This is the same as the `level`. Basically, this is telling people
to do (for example, in `Main.py`):
> {{{
> import os
> from bridgedb import log
>
> log.level = log.LOG_LEVELS['ERROR']
> log.folder = os.path.expanduser('~/foo-logs')
>
> log.msg("foo")
> }}}
...
> Because the `msg()` function would then look ''within its own scope''
for `level` (or `folder`, if it were used in that function), and it would
find the one at the top of the `log.py` file, set to `LOG_LEVEL['WARN']`,
and use that instead of the one we set in the scope of the parent caller.
Python scoping is a more than a bit of madness.
Yup, I understood this (sorry to make you provide an example), I just
think it's a bit confusing/kludgey, but I don't immediately see a better
way to do this and I'm ok with using this.
> > - Hm, BridgeDBLogPublisher().start() and stop() are not complementary,
I fear this can be confusing.
> >
>
> Hmm, you're probably right. Maybe the `stop()` should be named
`shutdown()`, or something else instead?
>
Sure, that sounds fine. Reading through this now, I wonder if `start()` is
what is bothering me. `stop()` does, indeed, stop all logging, however
`start()` can start logging (if to stdout) but usually does absolutely
nothing. I don't know if `begin()` is a better name. I don't know, I guess
go with `stop()` -> `shutdown()`, unless you come up with a better one.
> > - 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.
>
> For sure. It's definitely an unusual bit of code. Perhaps this whole
"import this ''entire'' module, not functions and classes from it" should
go in the module docstring, and there should be some sort of inline
comments or something above the NameError trick.
Sounds perfect
>
> Replying to [comment:19 cypherpunks]:
> > One thing that I forgot to add, how do you feel about merging
Util.logSafely() into log.py? The only reason I created Util.py was
because there was no other place to put a logging-related function.
>
>
> Yes, I assumed this should be done after everything is merged.
Sounds good!
>
> Replying to [comment:20 sysrqb]:
> >also also, I think you also need
> >{{{
> >diff --git a/lib/bridgedb/Main.py b/lib/bridgedb/Main.py
> >index 1f84091..729e4b0 100644
> >--- a/lib/bridgedb/Main.py
> >+++ b/lib/bridgedb/Main.py
> >@@ -293,6 +293,8 @@ def startup(cfg):
> > :ivar logdir: The directory to store logfiles in. Defaults to
rundir/log/.
> > """
> > rundir = getattr(cfg, 'RUN_IN_DIR', '~/run')
> >+ if rundir.startswith('~'):
> >+ rundir = os.path.expanduser(rundir)
> > os.chdir(rundir)
> > beginLogging(cfg, rundir)
> >}}}
> >or similar, else python cries when you use os.chdir('~/run').
> >
>
>
> Yeah, I do, you're right. Though I ''think'' that this change went into
the config branch for #9277.
>
Ah ha! Cool, that looks fine.
> I am refactoring. Somewhere though, something happened and my test of
the next rebase of this failed to log anything. I should probably write
some unittests to see what's failing.
:( sounds like a plan
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9199#comment:24>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list