diff --git a/ghost/core/core/server/services/bulk-email/bulk-email-processor.js b/ghost/core/core/server/services/bulk-email/bulk-email-processor.js index e8493039fd..7f94480ad8 100644 --- a/ghost/core/core/server/services/bulk-email/bulk-email-processor.js +++ b/ghost/core/core/server/services/bulk-email/bulk-email-processor.js @@ -196,8 +196,11 @@ module.exports = { // log any error that didn't come from the provider which would have already logged it if (!error.code || error.code !== 'BULK_EMAIL_SEND_FAILED') { - let ghostError = new errors.InternalServerError({ - err: error + let ghostError = new errors.EmailError({ + err: error, + code: 'BULK_EMAIL_SEND_FAILED', + message: `Error sending email batch ${emailBatchId}`, + context: error.message }); sentry.captureException(ghostError); logging.error(ghostError); @@ -274,7 +277,7 @@ module.exports = { }); sentry.captureException(ghostError); - logging.warn(ghostError); + logging.error(ghostError); debug(`failed to send message (${Date.now() - startTime}ms)`); throw ghostError; diff --git a/ghost/core/core/server/services/email-service/wrapper.js b/ghost/core/core/server/services/email-service/wrapper.js index 9b2cfcaf62..9bf3efe442 100644 --- a/ghost/core/core/server/services/email-service/wrapper.js +++ b/ghost/core/core/server/services/email-service/wrapper.js @@ -88,7 +88,8 @@ class EmailServiceWrapper { jobsService, emailSegmenter, emailRenderer, - db + db, + sentry }); this.service = new EmailService({ diff --git a/ghost/email-service/lib/batch-sending-service.js b/ghost/email-service/lib/batch-sending-service.js index f9dfa35633..fca0cc3a22 100644 --- a/ghost/email-service/lib/batch-sending-service.js +++ b/ghost/email-service/lib/batch-sending-service.js @@ -29,6 +29,7 @@ class BatchSendingService { #jobsService; #models; #db; + #sentry; /** * @param {Object} dependencies @@ -42,6 +43,7 @@ class BatchSendingService { * @param {Email} dependencies.models.Email * @param {object} dependencies.models.Member * @param {object} dependencies.db + * @param {object} [dependencies.sentry] */ constructor({ emailRenderer, @@ -49,7 +51,8 @@ class BatchSendingService { jobsService, emailSegmenter, models, - db + db, + sentry }) { this.#emailRenderer = emailRenderer; this.#sendingService = sendingService; @@ -57,6 +60,7 @@ class BatchSendingService { this.#emailSegmenter = emailSegmenter; this.#models = models; this.#db = db; + this.#sentry = sentry; } /** @@ -97,7 +101,17 @@ class BatchSendingService { error: null }, {patch: true, autoRefresh: false}); } catch (e) { - logging.error(`Error sending email ${email.id}: ${e.message}`); + const ghostError = new errors.EmailError({ + err: e, + code: 'BULK_EMAIL_SEND_FAILED', + message: `Error sending email ${email.id}` + }); + + logging.error(ghostError); + if (this.#sentry) { + // Log the original error to Sentry + this.#sentry.captureException(e); + } // Edge case: Store error in email model (that are not caught by the batch) await email.save({ @@ -324,8 +338,21 @@ class BatchSendingService { }, {patch: true, require: false, autoRefresh: false}); succeeded = true; } catch (err) { - logging.error(`Error sending email batch ${batch.id}`); - logging.error(err); + if (!err.code || err.code !== 'BULK_EMAIL_SEND_FAILED') { + // BULK_EMAIL_SEND_FAILED are already logged in mailgun-email-provider + const ghostError = new errors.EmailError({ + err, + code: 'BULK_EMAIL_SEND_FAILED', + message: `Error sending email batch ${batch.id}`, + context: err.message + }); + + logging.error(ghostError); + if (this.#sentry) { + // Log the original error to Sentry + this.#sentry.captureException(err); + } + } await batch.save({ status: 'failed', diff --git a/ghost/email-service/lib/mailgun-email-provider.js b/ghost/email-service/lib/mailgun-email-provider.js index 7712d0f32e..a26b0e40a3 100644 --- a/ghost/email-service/lib/mailgun-email-provider.js +++ b/ghost/email-service/lib/mailgun-email-provider.js @@ -168,7 +168,7 @@ class MailgunEmailProvider { }); } - logging.warn(ghostError); + logging.error(ghostError); debug(`failed to send message (${Date.now() - startTime}ms)`); // log error to custom error handler. ex sentry diff --git a/ghost/email-service/test/batch-sending-service.test.js b/ghost/email-service/test/batch-sending-service.test.js index 49df4d0199..1fdae467e2 100644 --- a/ghost/email-service/test/batch-sending-service.test.js +++ b/ghost/email-service/test/batch-sending-service.test.js @@ -125,8 +125,12 @@ describe('Batch Sending Service', function () { status: 'pending' } }); + const captureException = sinon.stub(); const service = new BatchSendingService({ - models: {Email} + models: {Email}, + sentry: { + captureException + } }); let emailModel; let afterEmailModel; @@ -141,6 +145,16 @@ describe('Batch Sending Service', function () { assert.equal(result, undefined); sinon.assert.calledOnce(errorLog); sinon.assert.calledOnce(sendEmail); + sinon.assert.calledOnce(captureException); + + // Check error code + const error = errorLog.firstCall.args[0]; + assert.equal(error.code, 'BULK_EMAIL_SEND_FAILED'); + + // Check error + const sentryError = captureException.firstCall.args[0]; + assert.equal(sentryError.message, ''); + assert.equal(emailModel.status, 'submitting', 'The email status is submitting while sending'); assert.equal(afterEmailModel.get('status'), 'failed', 'The email status is failed after sending'); assert.equal(afterEmailModel.get('error'), 'Something went wrong while sending the email'); @@ -621,7 +635,7 @@ describe('Batch Sending Service', function () { }); assert.equal(result, false); - sinon.assert.calledTwice(errorLog); + sinon.assert.calledOnce(errorLog); sinon.assert.calledOnce(sendingService.send); sinon.assert.calledOnce(findOne); @@ -632,6 +646,54 @@ describe('Batch Sending Service', function () { assert.equal(batch.get('error_data'), null); }); + it('Does log error to Sentry', async function () { + const EmailBatch = createModelClass({ + findOne: { + status: 'pending', + member_segment: null + } + }); + const sendingService = { + send: sinon.stub().rejects(new Error('Test error')) + }; + + const findOne = sinon.spy(EmailBatch, 'findOne'); + const captureException = sinon.stub(); + const service = new BatchSendingService({ + models: {EmailBatch, EmailRecipient}, + sendingService, + sentry: { + captureException + } + }); + + const result = await service.sendBatch({ + email: createModel({}), + batch: createModel({}), + post: createModel({}), + newsletter: createModel({}) + }); + + assert.equal(result, false); + sinon.assert.calledOnce(errorLog); + sinon.assert.calledOnce(sendingService.send); + sinon.assert.calledOnce(captureException); + const sentryExeption = captureException.firstCall.args[0]; + assert.equal(sentryExeption.message, 'Test error'); + + const loggedExeption = errorLog.firstCall.args[0]; + assert.match(loggedExeption.message, /Error sending email batch/); + assert.equal(loggedExeption.context, 'Test error'); + assert.equal(loggedExeption.code, 'BULK_EMAIL_SEND_FAILED'); + + sinon.assert.calledOnce(findOne); + const batch = await findOne.firstCall.returnValue; + assert.equal(batch.get('status'), 'failed'); + assert.equal(batch.get('error_status_code'), null); + assert.equal(batch.get('error_message'), 'Test error'); + assert.equal(batch.get('error_data'), null); + }); + it('Does save EmailError', async function () { const EmailBatch = createModelClass({ findOne: { @@ -664,7 +726,7 @@ describe('Batch Sending Service', function () { }); assert.equal(result, false); - sinon.assert.calledTwice(errorLog); + sinon.assert.notCalled(errorLog); sinon.assert.calledOnce(sendingService.send); sinon.assert.calledOnce(findOne);