Improved email error logging (#16184)

no issue

Logs errors to Sentry and adds error codes.

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({
+                let ghostError = new errors.EmailError({
-                    err: error
+                    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;

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({

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}`);
+            if (!err.code || err.code !== 'BULK_EMAIL_SEND_FAILED') {
-            logging.error(err);
+                // 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',

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

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);