Logging

[phelixbtc]

Replaces all print outputs by parallel output to file and console.

This might need some testing.

[phelixbtc]

I'd like to get this merged as soon as possible as it will otherwise cause additional work due to merge conflicts.

[JeremyRand]

I'll try to review within the next couple days. (Semester starts tomorrow and I'm in the process of moving back to the campus today.)

[JeremyRand]

Temporary NACK until we figure out whether the monkey patch is a good idea.

[phelixbtc]

Unfortunately by default standard log function behaves very differently than the print function. In general it is more comfortable to use a modified version. Particularly in this case the alternative is to dig through all the code and modify each output to the cumbersome standard log argument format. Even if you would volunteer to do it I would oppose as it is simply less efficient both to write and read.

I updated the mylogging module to be less hacky. Unfortunately pylint still does not get it. I suggest to disable it's warning E1205 ("logging-too-many-args") as it is plain wrong here.

[JeremyRand]

@phelixbtc The reason why Heartbleed happened was because OpenSSL used code that wasn't understandable by static analyzers. I would be strongly against merging code that can't be understood by static analyzers.

What do you mean by "it is simply less efficient both to write and read"? Are you talking about humans editing it, or computers executing it?

[phelixbtc]

@JeremyRand I was talking about humans.

In this case Pylint is wrong. Also the warning is only relevant to this specific issue. IMHO it legit in this case to deactivate it. The purpose of Pylint as I see it is to help development not stall it by having developers follow every nitpick.

I would like to go ahead and merge. If you have a suggestion on how to improve please do so in another PR.

[JeremyRand]

I'll get back to you on this by tomorrow. (Maybe sooner.)

[JeremyRand]

Do the changes to _log affect performance at all? It looks like it will do the string concatenation even if the logging level isn't met, which seems like a waste of resources. (I'm not sure of this.) Running a profiler on this would make me much more comfortable with it.

I think I will accept the change to PyLint configuration, on the grounds that it is unlikely that the error reported will show up anywhere else in our code.

I'll continue reviewing and get back to you soon.

[phelixbtc]

Good point about the performance implications. But the _log function is only called after the level is determined. See this snippet from the logging module:

if self.isEnabledFor(INFO):
    self._log(INFO, msg, args, **kwargs)

Regarding wrapping: 80 chars is a bit narrow for Python IMHO. With a couple of indention levels readability gets pretty bad because lines become too narrow. There are plenty of instances with way longer lines in the rest of the code. As long as we have not yet decided on a global line width maximum I would like to keep it like this. But it may be helpful to decide on this (in another discussion).

[phelixbtc]

@JeremyRand I'd really like to go forward.

[JeremyRand]

Given that I've been too busy to review and I don't want to hold this up any further, I'm okay with merging if it works for you. Note that I haven't fully reviewed the code.