Code review comment for lp:~vishvananda/nova/log-fix

Revision history for this message
Vish Ishaya (vishvananda) wrote :

> You can wipe out all references to basicConfig in the log module. All the
> logging.basicConfig call does is set up handlers for the root logger if they
> don't exist yet. Since NovaRootLogger will always have handlers, it becomes a
> no-op without having to re-define it. So you can remove
> * The definition of basicConfig
> * Setting the logging.basicConfig function
> * The check in the top-level audit() function that calls basicConfig

done.

>
> By returning on the first call to 'error' in the NovaLogger.exception, you
> stop environment dumps.

good catch

>
> You should set the "nova" value of loggerDict to be the root logger as well,
> otherwise you'll get a new instance when you call logging.getLogger("nova").
> It will require fixing the parent links in other loggers, and possibly using
> the same locking mechanics (see logging). That will also let the loop in
> reset find the root logger.

Good call.

>
> Instead of checking the class of loggers in the 'reset' function, you should
> probably check if they have a 'setup_from_flags' function, since it allows
> better duck-typing. It would also keep you from making the explicit call to
> the root logger.

The explicit call was fixed by above. Not sure that i see the value of duck-typing here.

>
> I'd also vote for keeping the call to reset() away from the flags library, and
> put in in the binaries immediately after they call FLAGS(). Also, nova-api is
> changing soon so that it doesn't mangle flags (ttx has a branch).

I'm ok with this although i find it annoying that we have to put this code in multiple places. Perhaps a utils.py function called parse_flags that does both?

« Back to merge proposal