In my opinion this has been a problem for a very long time, and it would be better to have a correction that fits the majority of cases with a small code change now rather than wait for the perfect log system. These changes don't preclude a better logger and way of filtering these out in the future. self.noisy may be viewed as a hack, but honestly it's worked well for me and it's easy to understand what it does. If the compromise is the 4021 patch (wrapping with "if self.noisy"), with noisy defaulting to True, I think that's a good solution today without precluding the perfect solution when someone wants to build it. -J On Mon, Jan 24, 2011 at 11:09 AM, <exarkun@twistedmatrix.com> wrote:
On 05:24 pm, jasonjwwilliams@gmail.com wrote:
If there's no objections the rest of today, I'll make sure the 4021 patch still applies and see what could be done as a test.
I think it would be sad to lose the port starting up log messages. Sure, they're a nuisance if you start and stop 50k ports per second. However, they're just what you want if you have a boring server that starts one or two ports in the entire lifetime of your application.
For clients, the common case is to start and stop many connectors over the lifetime of an application, so it sounds slightly more tolerable to disable these by default. However, even for clients I've found these messages useful. For example they've revealed bugs in buildbot's reconnection code.
So I doubt it's a pure win to disable these by default (just like it's not a pure win to have them enabled by default).
As long as someone's going to touch this code, it seems like it would be better to just implement structured logging. This doesn't even have to mean changes to LogPublisher. It could mean replacing:
log.msg("%s starting on %s"%(self.protocol.__class__, self._realPortNumber))
with:
log.msg( event_source=self, event_type="start", protocol=self.protocol, port_number=self._realPortNumber)
(and documenting and testing). It may well be nice to have LogPublisher be able to dispatch these more efficiently, but that's just an optimization.
This still has the effect of disabling the messages by default. And (for now) it's even harder to turn them back on (you need a log observer that can report about these events, instead of just setting a flag). But it's better in the long run, right?
Jean-Paul
-J
On Mon, Jan 24, 2011 at 3:40 AM, Phil Mayers <p.mayers@imperial.ac.uk> wrote:
On 01/24/2011 02:43 AM, Glyph Lefkowitz wrote:
Personally I'd say 'false'. This is technically a change in behavior, but I don't think that we should make guarantees about emitted log messages. Practically speaking, I've never seen any code which would care about an unstructured log message. Anyone else object to changing it?
I'm strongly in favour of setting noisy to False on all factory objects; I find their logging tedious, and have a bunch of monkeypatch code in most of my projects to import the modules and set the class variable, to I don't have to subclass just to do that.
The structured logging proposal sounds interesting and I'm potentially willing to give it a go. However, it seems like it would be a lot of work and that it would be very likely to sit in Trac review limbo for a while (getting ever-harder to merge the branch back in).
(This is not intended as a criticism - merely an observation that even tickets for simple problems with patches take a while to get reviewed, as manpower is of course a precious and scarce resource)
Do you have any thoughts on how it could be broken down into smaller chunks?
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python