1016534 - pulsebuildmonitor silently fails if no logger is provided

Status: RESOLVED FIXED

(Webtools :: Pulse, defect)

Description

[Mark Côté [:mcote]]

Seems that worker threads silently die if you leave the 'logger' arg of start_pulse_monitor() at is default value of None.  This is a problem because PulseBuildMonitor does not always check if self.logger is not None before attempting to use it, and since it's in its own thread, the exceptions disappear.

Assuming we want to keep the behaviour of not doing any logging if logger is None, the fix is just to put more checks in--although a more foolproof fix would be to create some sort of NullLogger if logger is passed in as None.

Comment 1

[Mark Côté [:mcote]]

Hm further weirdness: if self.logger exists, PulseBuildMonitor not only logs exceptions but also prints them to stderr via traceback.print_exc().  If self.logger isn't set, then it doesn't print them to stderr.  That doesn't make a lot of sense to me.

Comment 2

[Mark Côté [:mcote]]

Attachment: Fix logging when no logger provided

Spreading some pulse reviews around. :)

To avoid a bunch of "if self.logger" calls, which clearly can be easily forgotten, if logger is passed in as None, PulseBuildMonitor now uses a logger with name 'pulsebuildmonitor' with a NullHandler.

I also removed the print_exc() calls, since if desired the client can always pass in a logger with a StreamHandler.

Finally I removed the unused json/simplejson import.

Comment 3

[Mark Côté [:mcote]]

Attachment: Fix logging when no logger provided

Forgot to remove the traceback import, which is no longer used.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8429717 [details] [diff] [review]
Fix logging when no logger provided

Review of attachment 8429717 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, and it makes sense. Why not also updating the version number in setup.py? Are other changes coming up?

Maybe you should also ask jgriffin for review, given that I'm not an official reviewer for this project.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8430096 [details] [diff] [review]
Fix logging when no logger provided

Review of attachment 8430096 [details] [diff] [review]:
-----------------------------------------------------------------

As what I said on the other patch.

Comment 6

[Mark Côté [:mcote]]

Yeah, I want to (separately) bump the version requirement of mozillapulse so that ssl is on by default.  And I may also wait until we change the exchange and queue naming conventions.

Comment 7

[Mark Côté [:mcote]]

jgriffin says he trusts you. :)

http://hg.mozilla.org/automation/pulsebuildmonitor/rev/6bedbff3258e