From c99f40957e27af178d499a39d217c149d206b6fa Mon Sep 17 00:00:00 2001 From: Naz Gargol Date: Fri, 15 Nov 2019 18:25:33 +0700 Subject: [PATCH] Improved mega error handling (#11393) no issue - Increased default mailgun retry limit to 5 - Handling retry logic closer to SDK layer gives less future manual handling - Allowed failing request to be passed through to the caller - To be able to handle failed requests more gracefully in the future we need all available error information to be given to the caller - The previous method with `Promise.all` would have rejected a whole batch without providing details on each specific batch. - Limited data returned with a failed message to batch values - Added better error handling on mega layer - Added new column to store failed batch info - Added reference to mailgan error docs - Refactored batch emailer to respond with instances of an object - It's hard to reason about the response type of bulk mailer when multiple object types can be returned - This gives more clarity and ability to check with `instanceof` check --- core/server/data/schema/schema.js | 1 + core/server/services/bulk-email/index.js | 94 ++++++++++++++++------ core/server/services/bulk-email/mailgun.js | 3 +- core/server/services/mega/mega.js | 46 +++++++++-- core/server/translations/en.json | 5 ++ 5 files changed, 116 insertions(+), 33 deletions(-) diff --git a/core/server/data/schema/schema.js b/core/server/data/schema/schema.js index 12489af828..1259c33ae9 100644 --- a/core/server/data/schema/schema.js +++ b/core/server/data/schema/schema.js @@ -389,6 +389,7 @@ module.exports = { validations: {isIn: [['pending', 'submitting', 'submitted', 'failed']]} }, error: {type: 'string', maxlength: 2000, nullable: true}, + error_data: {type: 'text', maxlength: 1000000000, fieldtype: 'long', nullable: true}, meta: {type: 'text', maxlength: 65535, nullable: true}, stats: {type: 'text', maxlength: 65535, nullable: true}, email_count: {type: 'integer', nullable: false, unsigned: true, defaultTo: 0}, diff --git a/core/server/services/bulk-email/index.js b/core/server/services/bulk-email/index.js index 2ee900e2d2..6f959b4b51 100644 --- a/core/server/services/bulk-email/index.js +++ b/core/server/services/bulk-email/index.js @@ -4,6 +4,29 @@ const mailgunProvider = require('./mailgun'); const configService = require('../../config'); const settingsCache = require('../settings/cache'); +/** + * An object representing batch request result + * @typedef { Object } BatchResultBase + * @property { string } data - data that is returned from Mailgun or one which Mailgun was called with + */ +class BatchResultBase { +} + +class SuccessfulBatch extends BatchResultBase { + constructor(data) { + super(); + this.data = data; + } +} + +class FailedBatch extends BatchResultBase { + constructor(error, data) { + super(); + this.error = error; + this.data = data; + } +} + /** * An email address * @typedef { string } EmailAddress @@ -17,11 +40,13 @@ const settingsCache = require('../settings/cache'); */ module.exports = { + SuccessfulBatch, + FailedBatch, /** * @param {Email} message - The message to send * @param {[EmailAddress]} recipients - the recipients to send the email to * @param {[object]} recipientData - list of data keyed by email to inject into the email - * @returns {Promise>} An array of promises representing the success of the batch email sending + * @returns {Promise>} An array of promises representing the success of the batch email sending */ async send(message, recipients, recipientData = {}) { let BATCH_SIZE = 1000; @@ -36,33 +61,52 @@ module.exports = { BATCH_SIZE = 2; } - try { - const chunkedRecipients = _.chunk(recipients, BATCH_SIZE); - const blogTitle = settingsCache.get('title'); - fromAddress = blogTitle ? `${blogTitle}<${fromAddress}>` : fromAddress; - return Promise.map(chunkedRecipients, (toAddresses) => { - const recipientVariables = {}; - toAddresses.forEach((email) => { - recipientVariables[email] = recipientData[email]; - }); - const messageData = Object.assign({}, message, { - to: toAddresses, - from: fromAddress, - 'recipient-variables': recipientVariables - }); - const bulkEmailConfig = configService.get('bulkEmail'); + const blogTitle = settingsCache.get('title'); + fromAddress = blogTitle ? `${blogTitle}<${fromAddress}>` : fromAddress; - if (bulkEmailConfig && bulkEmailConfig.mailgun && bulkEmailConfig.mailgun.tag) { - Object.assign(messageData, { - 'o:tag': [bulkEmailConfig.mailgun.tag, 'bulk-email'] - }); - } + const chunkedRecipients = _.chunk(recipients, BATCH_SIZE); - return mailgunInstance.messages().send(messageData); + return Promise.mapSeries(chunkedRecipients, (toAddresses) => { + const recipientVariables = {}; + toAddresses.forEach((email) => { + recipientVariables[email] = recipientData[email]; }); - } catch (err) { - common.logging.error({err}); - } + + const batchData = { + to: toAddresses, + from: fromAddress, + 'recipient-variables': recipientVariables + }; + + const bulkEmailConfig = configService.get('bulkEmail'); + + if (bulkEmailConfig && bulkEmailConfig.mailgun && bulkEmailConfig.mailgun.tag) { + Object.assign(batchData, { + 'o:tag': [bulkEmailConfig.mailgun.tag, 'bulk-email'] + }); + } + + const messageData = Object.assign({}, message, batchData); + + return new Promise((resolve) => { + mailgunInstance.messages().send(messageData, (error, body) => { + if (error) { + // NOTE: logging an error here only but actual handling should happen in more sophisticated batch retry handler + // REF: possible mailgun errors https://documentation.mailgun.com/en/latest/api-intro.html#errors + common.logging.warn(new common.errors.GhostError({ + err: error, + context: common.i18n.t('errors.services.mega.requestFailed.error') + })); + + // NOTE: these are generated variables, so can be regenerated when retry is done + const data = _.omit(batchData, ['recipient-variables']); + resolve(new FailedBatch(error, data)); + } else { + resolve(new SuccessfulBatch(body)); + } + }); + }); + }); } }; diff --git a/core/server/services/bulk-email/mailgun.js b/core/server/services/bulk-email/mailgun.js index ebb707c477..e4ecec63d7 100644 --- a/core/server/services/bulk-email/mailgun.js +++ b/core/server/services/bulk-email/mailgun.js @@ -13,7 +13,8 @@ function createMailgun(config) { protocol: baseUrl.protocol, host: baseUrl.host, port: baseUrl.port, - endpoint: baseUrl.pathname + endpoint: baseUrl.pathname, + retry: 5 }); } diff --git a/core/server/services/mega/mega.js b/core/server/services/mega/mega.js index 61b1a0ab18..92925bc48a 100644 --- a/core/server/services/mega/mega.js +++ b/core/server/services/mega/mega.js @@ -186,14 +186,46 @@ async function listener(emailModel, options) { id: emailModel.id }); - const meta = await sendEmail(post, members); + let meta = []; + let error; - await models.Email.edit({ - status: 'submitted', - meta: JSON.stringify(meta) - }, { - id: emailModel.id - }); + try { + // NOTE: meta can contains an array which can be a mix of successful and error responses + // needs filtering and saving objects of {error, batchData} form to separate property + meta = await sendEmail(post, members); + } catch (err) { + common.logging.error(new common.errors.GhostError({ + err: err, + context: common.i18n.t('errors.services.mega.requestFailed.error') + })); + error = err.message; + } + + const successes = meta.filter(response => (response instanceof bulkEmailService.SuccessfulBatch)); + const failures = meta.filter(response => (response instanceof bulkEmailService.FailedBatch)); + const batchStatus = successes.length ? 'submitted' : 'failed'; + + if (!error && failures.length) { + error = failures[0].error.message; + } + + if (error && error.length > 2000) { + error = error.substring(0, 2000); + } + + try { + // CASE: the batch partially succeeded + await models.Email.edit({ + status: batchStatus, + meta: JSON.stringify(successes), + error: error, + error_data: JSON.stringify(failures) // NOTE:need to discuss how we store this + }, { + id: emailModel.id + }); + } catch (err) { + common.logging.error(err); + } } function listen() { diff --git a/core/server/translations/en.json b/core/server/translations/en.json index e3c895ce81..fb475e312c 100644 --- a/core/server/translations/en.json +++ b/core/server/translations/en.json @@ -519,6 +519,11 @@ }, "loader": "Error trying to load YAML setting for {setting} from '{path}'.", "ensureSettings": "Error trying to access settings files in {path}." + }, + "mega": { + "requestFailed": { + "error" : "The email service was unable to send an email batch." + } } }, "errors": {