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&grou... 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:
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&grou...
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.
Well, Mark was apparently too busy too. I've assigned this to myself and am making progress with the review.
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 :-)
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/)
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.
I will update the docstrings as per your feedback.
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.
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.
- 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?
No, you're right - using "a" and "w" should work. I'll change the code to lose the "+".
- 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?
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.
- 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.
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.
- 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).
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.
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.
I will update the docstrings as per your feedback.
Great! (When can we see a new release on http://www.red-dove.com/python_logging.html ?)
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.
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.
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.
- 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?
No, you're right - using "a" and "w" should work. I'll change the code to lose the "+".
OK.
- 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?
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.
Yes, this seems how log rotation is generally done. (Please remove the commented-out old code.)
- 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.
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.
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.
- 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.
Especially since this is slated to go into 2.3 only. :-)
- 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).
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.
The length header might still be useful just to be format-compatible with the TCP variant though.
- 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.
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:
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.
I will update the docstrings as per your feedback.
Great! (When can we see a new release on http://www.red-dove.com/python_logging.html ?)
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...
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.
[stuff snipped]
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!
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
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.
[stuff snipped]
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!
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".
Sounds good to me. I hope that whoever felt strongly about this (Martin von Loewis?) agrees.
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 :-)
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/)
Guido van Rossum guido@python.org writes:
Sounds good to me. I hope that whoever felt strongly about this (Martin von Loewis?) agrees.
I don't think I ever voiced an opinion on logging.
Regards, Martin
- 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?
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.
Yes, this seems how log rotation is generally done. (Please
remove
the commented-out old code.)
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:
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.
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
Chris McDonough wrote:
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.
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).
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:
Chris McDonough wrote:
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.
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.
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
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.
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/)
-
barry＠zope.com -
Chris McDonough -
Guido van Rossum -
martin＠v.loewis.de -
Vinay Sajip