PR#91: Add support to email admins in case of failure - sync-to-jira

sync-to-jira

#91 Add support to email admins in case of failure

Merged 4 years ago by sidpremkumar. Opened 4 years ago by sidpremkumar.

sidpremkumar/sync-to-jira failure-notice into develop

Adding support for email to be sent with traceback in case of failure

sidpremkumar • 4 years ago

064bff8

sync2jira/downstream.py

file modified

+11 -3

		`@@ -227,6 +227,10 @@`
		`break`
		`if not found:`
		`log.warning(' Could not find JIRA user for username %s' % issue.downstream.get('owner'))`
		`+ if not ret:`
		`+ message = ' No owner could be found for username %s' % issue.downstream.get('owner')`
		`+ log.warning(message.strip())`
		`+ raise ValueError(message)`

		`user = {'name': ret[0].displayName, 'email': ret[0].emailAddress}`

		`@@ -238,9 +242,9 @@`
		`admins = []`
		`admin_template = []`
		`for admin in config['sync2jira']['admins']:`
		`- ret = client.search_users(admin)`
		`+ ret = client.search_users(admin.keys()[0])`
		`if len(ret) > 1:`
		`- log.warning(' Found multiple users for admin %s' % issue.downstream.get('owner'))`
		`+ log.warning(' Found multiple users for admin %s' % admin.keys()[0])`
		`found = False`
		`for person in ret:`
		`if person.key == issue.downstream.get('owner'):`
		`@@ -248,7 +252,11 @@`
		`found = True`
		`break`
		`if not found:`
		`- log.warning(' Could not find JIRA user for admin %s' % issue.downstream.get('owner'))`
		`+ log.warning(' Could not find JIRA user for admin %s' % admin.keys()[0])`
		`+ if not ret:`
		`+ message = ' No admin could be found for username %s' % admin.keys()[0]`
		`+ log.warning(message.strip())`
		`+ raise ValueError(message)`
		`admins.append(ret[0].emailAddress)`
		`admin_template.append({'name': ret[0].displayName, 'email': ret[0].emailAddress})`

sync2jira/failure_template.jinja

file added

		`@@ -0,0 +1,7 @@`
		`+ <html>`
		`+ <body style="font-family: Arial; font-size: 14px;">`
		`+ <p>Looks like Sync2Jira has failed!</p>`
		`+ <p>Here is the full traceback:</p>`
		`+ <code>{{ traceback }}</code>`
		`+ </body>`
		`+ </html>`
		`\ No newline at end of file`

sync2jira/mailer.py

file modified

+2 -1

		`@@ -31,7 +31,8 @@`
		`msg["Subject"] = subject`
		`msg["From"] = sender`
		`msg["To"] = ", ".join(recipients)`
		`- msg['Cc'] = ", ".join(cc)`
		`+ if cc:`
		`+ msg['Cc'] = ", ".join(cc)`
		`server = smtplib.SMTP(_cfg["server"])`
		`part = MIMEText(text, 'html', 'utf-8')`
		`msg.attach(part)`

sync2jira/main.py

file modified

+40 -6

		`@@ -24,16 +24,21 @@`

		`import logging`
		`import warnings`
		`+ import traceback`

		`import fedmsg`
		`import fedmsg.config`
		`+ import jinja2`
		`+ import jira`

		`import sync2jira.upstream as u`
		`import sync2jira.downstream as d`
		`+ from sync2jira.mailer import send_mail`


		`log = logging.getLogger('sync2jira.main')`
		`remote_link_title = "Upstream issue"`
		`+ failure_email_subject = "Sync2Jira Has Failed!"`

		`handlers = {`
		`# Example: https://apps.fedoraproject.org/datagrepper/id?id=2016-895ed21e-5d53-4fde-86ac-64dab36a14ad&is_raw=true&size=extra-large`
		`@@ -184,13 +189,42 @@`
		`logging.basicConfig(level=logging.INFO)`
		`warnings.simplefilter("ignore")`

		`- if config['sync2jira'].get('initialize'):`
		`- initialize(config)`
		`-`
		`try:`
		`- listen(config)`
		`- except KeyboardInterrupt:`
		`- pass`
		`+ if config['sync2jira'].get('initialize'):`
		`+ initialize(config)`
		`+`
		`+ try:`
		`+ listen(config)`
		`+ except KeyboardInterrupt:`
		`+ pass`
		`+ except:`
		`+ report_failure(config)`
		`+ raise`
		`+`
		`+`
		`+ def report_failure(config):`
		`+ """`
		`+ Helper function to alert admins in case of failure.`
		`+`
		`+`
		`+ :param Dict config: Config dict for JIRA`
		`+ """`
		`+ # Email our admins with the traceback`
		`+ templateLoader = jinja2.FileSystemLoader(searchpath='sync2jira/')`
		`+ templateEnv = jinja2.Environment(loader=templateLoader)`
		`+ template = templateEnv.get_template('failure_template.jinja')`
		`+ html_text = template.render(traceback=traceback.format_exc())`
		`+`
		`+ # Get admin information`
		`+ admins = []`
		`+ for admin in config['sync2jira']['admins']:`
		`+ admins.append(admin.values()[0])`
		`+`
		`+ # Send mail`
		`+ send_mail(recipients=admins,`
		`+ cc=None,`
		`+ subject=failure_email_subject,`
		`+ text=html_text)`


		`def list_managed():`

sidpremkumar commented 4 years ago

If Syns2Jira fails, who gets alerted. This PR aims to fix that by looking up the admins (through the default JIRA instance) and sends them an email with the full traceback when Sync2Jira fails!

ralph commented on line 43 of sync2jira/main.py 4 years ago

Take everything in this except block and move it to its own function called report_failure or something so that this main function can remain high level with no low-level details.

Also, at the end of the except block, include a bare raise statement which will re-raise the exception so it gets printed in the logs and kills the process with a non-zero exit code.

rebased onto bef4c974c8ad74c79b1d68754a2c04f1e3f46b0a

4 years ago

ralph commented on line 7 of sync2jira/downstream.py 4 years ago

Could this be:

message = '  No owner could be found for username %s' % issue.downstream.get('owner')
log.warning(message.strip())
raise ValueError(message)

... so that the reason propagates to the error email?

ralph commented on line 73 of sync2jira/main.py 4 years ago

I don't see where issue gets assigned here. Some error in refactoring?

ralph commented on line 86 of sync2jira/main.py 4 years ago

I also don't see where admin_template gets used below here. Perhaps it isn't needed?

If it isn't needed, perhaps you can dispense with the whole setup here to query Jira to find more info about the admins in the first place. You could just keep the list of admins in the config as a list of emails, and email them directly. It would be much more simple code in this error case.

What would happen if sync2jira failed in the inner loop because there was suddenly some problem trying to access jira itself. The email code here would fail as well, and we wouldn't be notified.

Better to streamline this function. WDYT?

rebased onto 7948851f17e9c474bd5ef5113bf81bb59f97ef75

4 years ago

sidpremkumar commented 4 years ago

Could this be:
message = ' No owner could be found for username %s' % issue.downstream.get('owner')
log.warning(message.strip())
raise ValueError(message)

... so that the reason propagates to the error email?

Yes, I like this better.

I don't see where issue gets assigned here. Some error in refactoring?

Yeah needs to be removed.

I also don't see where admin_template gets used below here. Perhaps it isn't needed?
If it isn't needed, perhaps you can dispense with the whole setup here to query Jira to find more info about the admins in the first place. You could just keep the list of admins in the config as a list of emails, and email them directly. It would be much more simple code in this error case.
What would happen if sync2jira failed in the inner loop because there was suddenly some problem trying to access jira itself. The email code here would fail as well, and we wouldn't be notified.
Better to streamline this function. WDYT?

I agree its easier. But I wanted to keep it so that when we create the duplicate email we can have the admins names so they're easier to get in touch with. Maybe have both (username + email), that way we can just use email for when we are sending the failure notice? Take a look at the most recent PR and let me know what you think.

ralph commented on line 59 of sync2jira/main.py 4 years ago

Looks better, but this whole section with jira_instance and client can probably be removed too?

rebased onto 5e8b5dbbf7cc10a237d66397867dfa6f71109217

4 years ago

sidpremkumar commented 4 years ago