PEP 282 Implementation

I've uploaded my logging module, the proposed implementation for PEP 282, for committer review, to the SourceForge patch manager: http://sourceforge.net/tracker/index.php?func=detail&aid=578494&group_id=547 0&atid=305470 I've assigned it to Mark Hammond as (a) he had posted some comments to Trent Mick's original PEP posting, and (b) Barry Warsaw advised not assigning to PythonLabs people on account of their current workload. The file logging.py is (apart from some test scripts) all that's supposed to go into Python 2.3. The file logging-0.4.6.tar.gz contains the module, an updated version of the PEP (which I mailed to Barry Warsaw on 26th June), numerous test/example scripts, TeX documentation etc. You can also refer to http://www.red-dove.com/python_logging.html Here's hoping for a speedy review :-) Regards, Vinay Sajip

"VS" == Vinay Sajip <vinay_sajip@red-dove.com> writes:
VS> The file logging.py is (apart from some test scripts) all VS> that's supposed to go into Python 2.3. The file VS> logging-0.4.6.tar.gz contains the module, an updated version VS> of the PEP (which I mailed to Barry Warsaw on 26th June), VS> numerous test/example scripts, TeX documentation etc. You can VS> also refer to PEP 282 update has been installed. One coment about the PEP: where `lvl' is used as an argument to methods and functions, I think we shouldn't be so cute. Please spell it out as `level'. -Barry

A month (!) ago, Vinay Sajip wrote:
Well, Mark was apparently too busy too. I've assigned this to myself and am making progress with the review.
Here's some feedback. In general the code looks good. Only one style nits: I prefer docstrings that have a one-line summary, then a blank line, and then a longer description. There's a lot of code there! Should it perhaps be broken up into different modules? Perhaps it should become a logging *package* with submodules that define the various filters and handlers. Some detailed questions: - Why does the FileHandler open the file with mode "a+" (and later with "w+")? The "+" makes the file readable, but I see no reason to read it. Am I missing? - setRollover(): the explanation isn't 100% clear. I *think* that you always write to "app.log", and when that's full, you rename it to app.log.1, and app.log.1 gets renamed to app.log.2, and so on, and then you start writing to a new app.log, right? - class SocketHandler: why set yourself up for buffer overflow by using only 2 bytes for the packet size? You can use the struct module to encode/decode this, BTW. I also wonder what the application for this is, BTW. - method send(): in Python 2.2 and later, you can use the sendall() socket method which takes care of this loop for you. - class DatagramHandler, method send(): I don't think UDP handles fragmented packets very well -- if you have to break the packet up, there's no guarantee that the receiver will see the parts in order (or even all of them). - fileConfig(): Is there documentation for the configuration file? That's it for now. --Guido van Rossum (home page: http://www.python.org/~guido/)

I will update the docstrings as per your feedback.
How strongly do you feel about this? I did think about doing this and in fact the first implementation of the module was as a package. I found this a little more cumbersome than the single-file solution, and reimplemented as logging.py. The module is a little on the large side but the single-file organization makes it a little easier to use.
No, you're right - using "a" and "w" should work. I'll change the code to lose the "+".
Yes. The original implementation was different - it just closed the current file and opened a new file app.log.n. The current implementation is slightly slower due to the need to rename several files, but the user can tell more easily which the latest log file is. I will update the setRollover() docstring to indicate more clearly how it works; I'm assuming that the current algorithm is deemed good enough.
I agree about the 2-byte limit. I can change it to use struct and an integer length. The application for encoding the length is simply to allow a socket-based server to handle multiple events sent by SocketHandler, in the event that the connection is kept open as long as possible and not shut down after every event.
- method send(): in Python 2.2 and later, you can use the sendall() socket method which takes care of this loop for you.
OK. I can update the code to use this in the case of 2.2 and later.
You're absolutely right - I wasn't thinking clearly enough about how UDP actually works. I will replace the loop with a single sendto() call.
- fileConfig(): Is there documentation for the configuration file?
There is some documentation in the python_logging.html file which is part of the distribution and also on the Web at http://www.red-dove.com/python_logging.html - it's in the form of comments in an annotated logconf.ini. I have not polished the documentation in this area as I'm not sure how much of the configuration stuff should be in the logging module itself. Feedback I've had indicates that at least some people object moderately strongly to having a particular configuration design forced on them. I'd appreciate views on this. Many thanks for the feedback, Vinay Sajip

I'm sorry that this seems to be a thread with one message per month! I'll try to be more responsive from now on, the big Zope projects that were keeping me busy have given me some slack time.
Great! (When can we see a new release on http://www.red-dove.com/python_logging.html ?)
I would feel much less strongly about this if several of the additional things could be moved to separate files without making it a package.
OK.
Yes, this seems how log rotation is generally done. (Please remove the commented-out old code.)
OK, please change it to a 4-byte length header. I understand why you need the length header; I'm just curious about the need for a socket server.
Especially since this is slated to go into 2.3 only. :-)
The length header might still be useful just to be format-compatible with the TCP variant though.
This is an example of something that I'd like to see relegated to a separate file. It really looks like fileConfig(), listen() and stopListening() are a separate feature bundle that looks like it is a specific example application rather than a core feature of the logging module. It certainly doesn't appear in PEP 282. Maybe the socket handler classes belong in the same category. Of course, the same can be said about all Handler subclasses except StreamHandler. Only StreamHandler is referenced by basicConfig(). Perhaps these should all (except StreamHandler) be moved to separate files? This sounds like a reason to make it a package. The main logging code could be in the __init__.py file -- there's no rule that says __init__.py should be empty or short! PS. In your comments you seem fond of the word "needful". I've rarely heard that word -- perhaps it is archaic or common only in India? --Guido van Rossum (home page: http://www.python.org/~guido/)

Guido van Rossum wrote:
Great.
I was waiting for your feedback about the packaging - the docstrings have been changed but I wanted to roll everything into the next release. Speaking of which...
[stuff snipped]
How about this suggestion? We could leave the core code in the existing module, "logging". This would include a minimal set of handlers, and all the Filters, and I think StreamHandler and FileHandler should be in here. All other handlers would live in "logging.handlers". As for configuration - basicConfig() could live in "logging" and any other configuration code in "logging.config". If the above seems a good idea, please let me know and I'll refactor accordingly - then the next release will (hopefully) be in the next 2-3 weeks.
PS. In your comments you seem fond of the word "needful". I've rarely heard that word -- perhaps it is archaic or common only in India?
I only found 2 uses of "needful" - in BufferingHandler and ConfigStreamHandler. It's the whole phrase "do the needful", which I think is peculiar to England but has its share of users on the subcontinent :-) Regards Vinay Sajip

Sounds good to me. I hope that whoever felt strongly about this (Martin von Loewis?) agrees.
Oh well. Shows how Americanized I am, despite my thoroughly European upbringing, after 7 years here. :-) --Guido van Rossum (home page: http://www.python.org/~guido/)

It would be helpful for the FileHandler class to define a method which just closes and reopens the current logfile (instead of actually rotating a set like-named logfiles). This would allow logfile rotation to be performed by a separate process (e.g. RedHat's logrotate). Sometimes it's better (and even necessary) to be able to use system-provided log rotation facilities instead of relying on the native rotation facilities. - C

Chris McDonough wrote:
I'm not sure whether this should be in the core functionality. I presume you don't mean an atomic "close and reopen" operation - rather, are you suggesting close the file, maybe rename it at the application level, then reopen? If so, then it's best handled entirely in the application level, through a subclass of FileHandler. This allows each application to consider issues such as what to do with events that occur between close and reopen (e.g. if multiple threads are running). Regards Vinay

No, this is using Unix functionality where once you have opened a file, if the file is renamed, you can continue to write to it and you will be writing to the renamed file. IOW the open file is connected to the inode, not the filename. Typically an application catches SIGHUP (though that has its share of problems!) and in response simply closes and reopens the file, using the original filename. The sysadmin uses this as follows: mv foo.log foo.log.1 kill -HUP `cat foo.pid` Having looked at it again, I think that this is definitely better than doing log rotation in the FileHandler. The rotation code in the log handler currently calls tell() after each record is emitted. This is expensive, and not needed if you use an external process to watch over the log files and rotate them. --Guido van Rossum (home page: http://www.python.org/~guido/)

Guido van Rossum wrote:
I think file rotation can definitely come out of the core, since there are many ways to do it and no one "right" way. I'll move the rotation stuff into a subclass of FileHandler called RotatingFileHandler (anyone think of a better name?). The FileHandler will remain in the core module; the subclass will be in the "logging.handlers" module. Vinay

Maybe this could be a different Handler subclass? I have to admit that I find log rotation borderline functionality for the logging module. Perhaps Chris' suggestion is sufficient. --Guido van Rossum (home page: http://www.python.org/~guido/)

"VS" == Vinay Sajip <vinay_sajip@red-dove.com> writes:
VS> The file logging.py is (apart from some test scripts) all VS> that's supposed to go into Python 2.3. The file VS> logging-0.4.6.tar.gz contains the module, an updated version VS> of the PEP (which I mailed to Barry Warsaw on 26th June), VS> numerous test/example scripts, TeX documentation etc. You can VS> also refer to PEP 282 update has been installed. One coment about the PEP: where `lvl' is used as an argument to methods and functions, I think we shouldn't be so cute. Please spell it out as `level'. -Barry

A month (!) ago, Vinay Sajip wrote:
Well, Mark was apparently too busy too. I've assigned this to myself and am making progress with the review.
Here's some feedback. In general the code looks good. Only one style nits: I prefer docstrings that have a one-line summary, then a blank line, and then a longer description. There's a lot of code there! Should it perhaps be broken up into different modules? Perhaps it should become a logging *package* with submodules that define the various filters and handlers. Some detailed questions: - Why does the FileHandler open the file with mode "a+" (and later with "w+")? The "+" makes the file readable, but I see no reason to read it. Am I missing? - setRollover(): the explanation isn't 100% clear. I *think* that you always write to "app.log", and when that's full, you rename it to app.log.1, and app.log.1 gets renamed to app.log.2, and so on, and then you start writing to a new app.log, right? - class SocketHandler: why set yourself up for buffer overflow by using only 2 bytes for the packet size? You can use the struct module to encode/decode this, BTW. I also wonder what the application for this is, BTW. - method send(): in Python 2.2 and later, you can use the sendall() socket method which takes care of this loop for you. - class DatagramHandler, method send(): I don't think UDP handles fragmented packets very well -- if you have to break the packet up, there's no guarantee that the receiver will see the parts in order (or even all of them). - fileConfig(): Is there documentation for the configuration file? That's it for now. --Guido van Rossum (home page: http://www.python.org/~guido/)

I will update the docstrings as per your feedback.
How strongly do you feel about this? I did think about doing this and in fact the first implementation of the module was as a package. I found this a little more cumbersome than the single-file solution, and reimplemented as logging.py. The module is a little on the large side but the single-file organization makes it a little easier to use.
No, you're right - using "a" and "w" should work. I'll change the code to lose the "+".
Yes. The original implementation was different - it just closed the current file and opened a new file app.log.n. The current implementation is slightly slower due to the need to rename several files, but the user can tell more easily which the latest log file is. I will update the setRollover() docstring to indicate more clearly how it works; I'm assuming that the current algorithm is deemed good enough.
I agree about the 2-byte limit. I can change it to use struct and an integer length. The application for encoding the length is simply to allow a socket-based server to handle multiple events sent by SocketHandler, in the event that the connection is kept open as long as possible and not shut down after every event.
- method send(): in Python 2.2 and later, you can use the sendall() socket method which takes care of this loop for you.
OK. I can update the code to use this in the case of 2.2 and later.
You're absolutely right - I wasn't thinking clearly enough about how UDP actually works. I will replace the loop with a single sendto() call.
- fileConfig(): Is there documentation for the configuration file?
There is some documentation in the python_logging.html file which is part of the distribution and also on the Web at http://www.red-dove.com/python_logging.html - it's in the form of comments in an annotated logconf.ini. I have not polished the documentation in this area as I'm not sure how much of the configuration stuff should be in the logging module itself. Feedback I've had indicates that at least some people object moderately strongly to having a particular configuration design forced on them. I'd appreciate views on this. Many thanks for the feedback, Vinay Sajip

I'm sorry that this seems to be a thread with one message per month! I'll try to be more responsive from now on, the big Zope projects that were keeping me busy have given me some slack time.
Great! (When can we see a new release on http://www.red-dove.com/python_logging.html ?)
I would feel much less strongly about this if several of the additional things could be moved to separate files without making it a package.
OK.
Yes, this seems how log rotation is generally done. (Please remove the commented-out old code.)
OK, please change it to a 4-byte length header. I understand why you need the length header; I'm just curious about the need for a socket server.
Especially since this is slated to go into 2.3 only. :-)
The length header might still be useful just to be format-compatible with the TCP variant though.
This is an example of something that I'd like to see relegated to a separate file. It really looks like fileConfig(), listen() and stopListening() are a separate feature bundle that looks like it is a specific example application rather than a core feature of the logging module. It certainly doesn't appear in PEP 282. Maybe the socket handler classes belong in the same category. Of course, the same can be said about all Handler subclasses except StreamHandler. Only StreamHandler is referenced by basicConfig(). Perhaps these should all (except StreamHandler) be moved to separate files? This sounds like a reason to make it a package. The main logging code could be in the __init__.py file -- there's no rule that says __init__.py should be empty or short! PS. In your comments you seem fond of the word "needful". I've rarely heard that word -- perhaps it is archaic or common only in India? --Guido van Rossum (home page: http://www.python.org/~guido/)

Guido van Rossum wrote:
Great.
I was waiting for your feedback about the packaging - the docstrings have been changed but I wanted to roll everything into the next release. Speaking of which...
[stuff snipped]
How about this suggestion? We could leave the core code in the existing module, "logging". This would include a minimal set of handlers, and all the Filters, and I think StreamHandler and FileHandler should be in here. All other handlers would live in "logging.handlers". As for configuration - basicConfig() could live in "logging" and any other configuration code in "logging.config". If the above seems a good idea, please let me know and I'll refactor accordingly - then the next release will (hopefully) be in the next 2-3 weeks.
PS. In your comments you seem fond of the word "needful". I've rarely heard that word -- perhaps it is archaic or common only in India?
I only found 2 uses of "needful" - in BufferingHandler and ConfigStreamHandler. It's the whole phrase "do the needful", which I think is peculiar to England but has its share of users on the subcontinent :-) Regards Vinay Sajip

Sounds good to me. I hope that whoever felt strongly about this (Martin von Loewis?) agrees.
Oh well. Shows how Americanized I am, despite my thoroughly European upbringing, after 7 years here. :-) --Guido van Rossum (home page: http://www.python.org/~guido/)

It would be helpful for the FileHandler class to define a method which just closes and reopens the current logfile (instead of actually rotating a set like-named logfiles). This would allow logfile rotation to be performed by a separate process (e.g. RedHat's logrotate). Sometimes it's better (and even necessary) to be able to use system-provided log rotation facilities instead of relying on the native rotation facilities. - C

Chris McDonough wrote:
I'm not sure whether this should be in the core functionality. I presume you don't mean an atomic "close and reopen" operation - rather, are you suggesting close the file, maybe rename it at the application level, then reopen? If so, then it's best handled entirely in the application level, through a subclass of FileHandler. This allows each application to consider issues such as what to do with events that occur between close and reopen (e.g. if multiple threads are running). Regards Vinay

No, this is using Unix functionality where once you have opened a file, if the file is renamed, you can continue to write to it and you will be writing to the renamed file. IOW the open file is connected to the inode, not the filename. Typically an application catches SIGHUP (though that has its share of problems!) and in response simply closes and reopens the file, using the original filename. The sysadmin uses this as follows: mv foo.log foo.log.1 kill -HUP `cat foo.pid` Having looked at it again, I think that this is definitely better than doing log rotation in the FileHandler. The rotation code in the log handler currently calls tell() after each record is emitted. This is expensive, and not needed if you use an external process to watch over the log files and rotate them. --Guido van Rossum (home page: http://www.python.org/~guido/)

Guido van Rossum wrote:
I think file rotation can definitely come out of the core, since there are many ways to do it and no one "right" way. I'll move the rotation stuff into a subclass of FileHandler called RotatingFileHandler (anyone think of a better name?). The FileHandler will remain in the core module; the subclass will be in the "logging.handlers" module. Vinay

Maybe this could be a different Handler subclass? I have to admit that I find log rotation borderline functionality for the logging module. Perhaps Chris' suggestion is sufficient. --Guido van Rossum (home page: http://www.python.org/~guido/)
participants (5)
-
barry@zope.com
-
Chris McDonough
-
Guido van Rossum
-
martin@v.loewis.de
-
Vinay Sajip