832415 - [PulseGuardian] Email errors to admins

Status: RESOLVED FIXED

(Webtools :: Pulse, defect, P3)

Description

[Mark Côté [:mcote]]

Every so often pulse barfs and we don't notice it for a little while.  Sometimes the server stays down, and sometimes it appears that the client silently dies or fails to reconnect.

It would be nice to have a little web service that keeps a connection open to pulse and listens to, say, builds and tests on m-i.  It would display the time it received the last message, and send out a warning email if nothing is heard after X minutes.  Or it could monitor the tinderbox builds on ftp.m.o and warn if one shows up there but isn't reported by pulse after Y minutes.

Comment 1

[Mark Côté [:mcote]]

Assigning to nobody for now, since this doesn't have to be implemented by jgriffin necessarily. :)

Comment 2

[Mark Côté [:mcote]]

We should solve this in PulseGuardian by having admins (when we add that feature) receive copies of all error messages (e.g. could not contact pulse) as well be CCed on all queue warning and deletion emails.

Comment 3

[abhi12ravi]

Could you please assign this bug to me?

Comment 4

[Mark Côté [:mcote]]

Unassigning due to lack of activity.  abhi12ravi, you are more than welcome to submit a patch, but in the mean time I'm opening it up for other contributors.

Comment 5

[Allan [:asilva]]

Hi, I am new here, I am looking for the "First Bug" that needs python skills.
It is possible to try fix it?



-
Translated from portuguese with help of Google Translate

Comment 6

[Mark Côté [:mcote]]

(In reply to allan.tavares from comment #5)
> Hi, I am new here, I am looking for the "First Bug" that needs python skills.
> It is possible to try fix it?

Sure!  See the link in the "User Story" section above to get started.  Feel free to ask questions here if they're specifically about fixing this, or if you have general questions about setting up PulseGuardian, please email me or ask in #pulse on irc.mozilla.org.

Comment 7

[abhi12ravi]

Hi Mark,

Sorry to keep you waiting for so long. I am currently involved in other initiatives at Mozilla and therefore I am unable to focus my time and energy over this bug. Please re-assign it to someone who might be interested.

Sorry and thank you!

Comment 8

[Allan [:asilva]]

Hi Mark,

I read the HACKING.md file and I need to confirm some steps:
1. Install RabbitMQ;
2. Fork https://github.com/mozilla/pulseguardian
3. Work locally, make changes, test etc.
4. Send a Pull Request when finish.

Is this workflow correct?

-
Note: Translated from portuguese with help of Google Translate

Comment 9

[Mark Côté [:mcote]]

Yes, that works!

Comment 10

[Allan [:asilva]]

Hi Mark,
Finally, I finish PulseGuardian and RabbitMQ installation! :)
Now I need understand what to do.

When you say:
-"Sometimes the server stays down, and sometimes it appears that the client silently dies or fails to reconnect."
	-By server, are you referring to RabbitMQ and by client referring to guardian daemon?

Comment 11

[Mark Côté [:mcote]]

Ah, this bug has changed a bit, so comment 0 is now a bit misleading.  It was written before we had PulseGuardian.  So, to update what I said, when PulseGuardian has an error connecting to Pulse, we should email the admin as well as logging it.

Comment 12

[Allan [:asilva]]

Hi Mark, I made a PR for this bug:
https://github.com/mozilla/pulseguardian/pull/16

Comment 13

[Mark Côté [:mcote]]

Thanks for the pull request!  I'm taking a look now.

One thing I noticed is that you are only sending connection errors to admins.  This bug was originally about also sending queue warnings.  However, I've since reconsidered that, and I think it makes sense to only send errors.  I've updated the bug summary accordingly. :)

Comment 14

[Mark Côté [:mcote]]

Okay I left some comments. :)

After you address them, could you actually create an attachment on this bug, paste in the pull request URL, and set the review flag to "?" and the requestee to me?  It helps me track review requests better.  Thanks!

Comment 15

[Allan [:asilva]]

Attachment: PR for review

Hi Mark, I reviewed all considerations.
I think that everything or almost everything is OK.
Thanks

PR: https://github.com/mozilla/pulseguardian/pull/16

Comment 16

[Mark Côté [:mcote]]

Comment on attachment 8755215 [details] [review]
PR for review

This is looking really good!  I left a few comments, just small things.

However, while testing it, I see why you are sometimes getting a TypeError.  This is actually a bug in PulseManagementAPI._api_request().  We actually catch exceptions there, and if there are too many, we return None.  That value is then passed to PulseGuardian.clear_deleted_queues(), which is not expecting a None type, and so it raises a TypeError.

Now that we have your better retry logic, though, we don't need the retry loop in _api_request().  Could you take that out?  I think at that point you won't need the special case for catching TypeErrors anymore (the proper ConnectionError will be raised), and so you can remove it (although please test to make sure; don't take my word for it. :)

Finally, also please try to keep lines to less than 80 characters, and don't leave trailing spaces on lines (a good editor should point out when you have extra space at the end of a line).

Thanks very much for your help!

Comment 17

[Allan [:asilva]]

(In reply to Mark Côté [:mcote] from comment #16)
> Comment on attachment 8755215 [details] [review]
> PR for review
> 
> This is looking really good!  I left a few comments, just small things.
> 
> However, while testing it, I see why you are sometimes getting a TypeError. 
> This is actually a bug in PulseManagementAPI._api_request().  We actually
> catch exceptions there, and if there are too many, we return None.  That
> value is then passed to PulseGuardian.clear_deleted_queues(), which is not
> expecting a None type, and so it raises a TypeError.
> 
> Now that we have your better retry logic, though, we don't need the retry
> loop in _api_request().  Could you take that out?  I think at that point you
> won't need the special case for catching TypeErrors anymore (the proper
> ConnectionError will be raised), and so you can remove it (although please
> test to make sure; don't take my word for it. :)
> 
> Finally, also please try to keep lines to less than 80 characters, and don't
> leave trailing spaces on lines (a good editor should point out when you have
> extra space at the end of a line).
> 
> Thanks very much for your help!

Hi Mark, take a look again in my changes, please. Contact me if need more changes.

#16 https://github.com/mozilla/pulseguardian/pull/16

Comment 18

[Mark Côté [:mcote]]

Thanks!  There were a few more little nits (including more trailing whitespace; please look out for that in future patches) but nothing big.  I fixed them up myself (only took a couple minutes), and I'm rebasing your patch on master now, since a few things have changed since you started working on it.  I'll be landing it later today or tomorrow.  Thanks very much!

Comment 19

[Mark Côté [:mcote]]

https://github.com/mozilla/pulseguardian/commit/0650b6f10eb735069411c510986196c3fcaf03ab

Comment 20

[Allan [:asilva]]

OK, Mark, thanks for feedback. Some points I interpreted the wrong way (I'm not good in English) :) 
I look your merged code and I understood better.