Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase information density of logs #85

Merged
merged 3 commits into from
Jul 16, 2013
Merged

Conversation

novas0x2a
Copy link
Contributor

I aggregate logs to syslog, and some aspects of how kazoo does logging make it difficult to follow; this pull focuses on improving the log messages and logger handling.

@hannosch
Copy link
Contributor

Could you split the Close/Ping change into a separate pull request? That one is much less controversial than the rest.

Your refactoring does remove one important optimization: Avoiding a large number of logging calls if the debug level isn't set. Usually applications won't have debug logging enabled, so it's quite nice to avoid all the extra logging function calls.

@novas0x2a
Copy link
Contributor Author

Done. The commits are still in this one because there are small changes that make them conflict, but I rebased this one on the other one, so if you accept the other one this commits will disappear from this one.

As for the timing aspect: yes, it's true that it's slightly more overhead (in fact, it's almost 4 times slower!) but we're talking about half a microsecond: https://gist.github.com/novas0x2a/43391b46b8bc7fede657.

@novas0x2a
Copy link
Contributor Author

An addendum, since I didn't really mention the motivation for that change; one of the nice things about just trusting the log configuration rather than caching the isEnabledFor value is you can change loglevels dynamically- if you notice that your long-running program is misbehaving, you just need to build in a hook to do logging.getLogger().setLevel(DEBUG) and suddenly all the verbose logging is on (you can even do things like logging.getLogger('kazoo.protocol.connection').setLevel(WARN) if you don't care about that part of the logs)

@novas0x2a
Copy link
Contributor Author

refreshed the pull, since it conflicted with merged changes.

We already have a logger associated with the client, and this makes it
easier to track which client is doing what when the logs are aggregated
on a log server.
This makes it easier to match requests and responses when the logs are
aggregated on a log server.
The getEffectiveLevel check is not actually necessary, the logger
already performs an isEnabledFor check when you use the
loglevel-specific methods (see Logger in python's logging module).

This also breaks DEBUG into two levels, INFO (for the request-response
cycle) and DEBUG (for the protocol-specific messages and pings).
bbangert added a commit that referenced this pull request Jul 16, 2013
Increase information density of logs
@bbangert bbangert merged commit b59091a into python-zk:master Jul 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants