Skip to content

Commit 6b2af82

Browse files
committed
feat(metrics): Add metrics when the bounce check blocks an email
This change also includes a change to bounces.js that isn't used in the metrics along with tests because it was part of a footgun that I ran into while working on this patch. Fixes FXA-10964
1 parent 16deb2e commit 6b2af82

5 files changed

Lines changed: 134 additions & 5 deletions

File tree

packages/fxa-auth-server/lib/bounces.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ module.exports = (config, db) => {
7171
tally.count++;
7272
}
7373
});
74+
return tallies;
7475
}
7576

7677
function disabled() {

packages/fxa-auth-server/lib/senders/email.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ const UTM_PREFIX = 'fx-';
2828
const X_SES_CONFIGURATION_SET = 'X-SES-CONFIGURATION-SET';
2929
const X_SES_MESSAGE_TAGS = 'X-SES-MESSAGE-TAGS';
3030

31-
module.exports = function (log, config, bounces) {
31+
module.exports = function (log, config, bounces, statsd) {
3232
const oauthClientInfo = require('./oauth_client_info')(log, config);
3333
const verificationReminders = require('../verification-reminders')(
3434
log,
@@ -461,6 +461,9 @@ module.exports = function (log, config, bounces) {
461461
try {
462462
await bounces.check(to, template);
463463
} catch (err) {
464+
const tags = { template, error: err.errno };
465+
statsd.increment('email.bounce.limit', tags);
466+
464467
log.error('email.bounce.limit', {
465468
err: err.message,
466469
errno: err.errno,

packages/fxa-auth-server/lib/senders/index.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ module.exports = async (
1414
sender // This is only used in tests
1515
) => {
1616
const defaultLanguage = config.i18n.defaultLanguage;
17-
const Mailer = createMailer(log, config, bounces);
17+
const Mailer = createMailer(log, config, bounces, statsd);
1818

1919
async function createSenders() {
2020
return {

packages/fxa-auth-server/test/local/bounces.js

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,4 +190,34 @@ describe('bounces', () => {
190190
await bounces.check(EMAIL, 'recovery');
191191
assert.equal(db.emailBounces.callCount, 0);
192192
});
193+
194+
it('get results from bounce checks', async () => {
195+
const conf = Object.assign({}, config);
196+
const latestBounce = Date.now() - 20000;
197+
conf.smtp = {
198+
bounces: {
199+
enabled: true,
200+
complaint: {
201+
0: 5000,
202+
1: 50000,
203+
},
204+
},
205+
};
206+
const db = {
207+
emailBounces: sinon.spy(() =>
208+
Promise.resolve([
209+
{
210+
bounceType: BOUNCE_TYPE_COMPLAINT,
211+
createdAt: latestBounce,
212+
},
213+
])
214+
),
215+
};
216+
return createBounces(conf, db)
217+
.check(EMAIL)
218+
.then((tallies) => {
219+
assert.equal(tallies[BOUNCE_TYPE_COMPLAINT].count, 1);
220+
assert.equal(tallies[BOUNCE_TYPE_COMPLAINT].latest, latestBounce);
221+
});
222+
});
193223
});

packages/fxa-auth-server/test/local/senders/emails.ts

Lines changed: 98 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ import {
1717
MOCK_DEVICE_OS,
1818
MOCK_DEVICE_OS_VERSION,
1919
} from '../../../lib/senders/emails/partials/userDevice/mocks';
20+
import AppError from '../../../lib/error';
21+
import { AUTH_SERVER_ERRNOS } from 'fxa-shared/lib/errors';
2022

2123
const moment = require('moment-timezone');
2224
const config = require(`${ROOT_DIR}/config`).default.getProperties();
@@ -3213,6 +3215,96 @@ describe('mailer constructor:', () => {
32133215
});
32143216
});
32153217

3218+
describe('mailer bounce throws exceptions', () => {
3219+
let mailer: Record<any, any>, mockStatsd: any;
3220+
3221+
before(async () => {
3222+
mockStatsd = mocks.mockStatsd();
3223+
mailer = await setup(
3224+
mocks.mockLog(),
3225+
config,
3226+
{},
3227+
'en',
3228+
null,
3229+
{
3230+
check: async () => {
3231+
throw AppError.emailComplaint(10);
3232+
},
3233+
},
3234+
mockStatsd
3235+
);
3236+
3237+
mailer.localize = () => ({});
3238+
});
3239+
3240+
it('email bounce exceptions increment stats', () => {
3241+
const message = {
3242+
3243+
flowId: 'wibble',
3244+
subject: 'subject',
3245+
template: 'inactive-first-email',
3246+
uid: 'foo',
3247+
};
3248+
3249+
// We shouldn't get to this call, so we fail it.
3250+
sinon.stub(mailer.mailer, 'sendMail').callsFake((_config, cb: any) => {
3251+
cb(new Error('Fail'));
3252+
});
3253+
3254+
return mailer.send(message).then(() => {
3255+
const spiedStatsd = mockStatsd.increment.getCalls()[0];
3256+
3257+
assert.equal(spiedStatsd.args[0], 'email.bounce.limit');
3258+
assert.equal(spiedStatsd.args[1].template, 'inactive-first-email');
3259+
assert.equal(
3260+
spiedStatsd.args[1].error,
3261+
AUTH_SERVER_ERRNOS.BOUNCE_COMPLAINT
3262+
);
3263+
});
3264+
});
3265+
});
3266+
3267+
describe('mailer bounces succeed', () => {
3268+
let mailer: Record<any, any>, mockStatsd: any;
3269+
const mockLog = mocks.mockLog();
3270+
3271+
before(async () => {
3272+
mockStatsd = mocks.mockStatsd();
3273+
mailer = await setup(
3274+
mockLog,
3275+
config,
3276+
{},
3277+
'en',
3278+
null,
3279+
{
3280+
check: async () => Promise.resolve(),
3281+
},
3282+
mockStatsd
3283+
);
3284+
3285+
mailer.localize = () => ({});
3286+
sinon.stub(mailer.mailer, 'sendMail').callsFake((_config, cb: any) => {
3287+
cb(null, { resp: 'ok' });
3288+
});
3289+
});
3290+
3291+
it('email bounce check sends mail', () => {
3292+
const message = {
3293+
3294+
flowId: 'wibble',
3295+
subject: 'subject',
3296+
template: 'inactive-first-email',
3297+
uid: 'foo',
3298+
};
3299+
3300+
return mailer.send(message).then((resp) => {
3301+
// assert that the log in the final 'sendMail' function is called.
3302+
const emailEventLog = mockLog.debug.getCalls()[1];
3303+
assert.equal(emailEventLog.args[0], 'mailer.send.1');
3304+
});
3305+
});
3306+
});
3307+
32163308
function sesMessageTagsHeaderValue(templateName: string, serviceName?: any) {
32173309
return `messageType=fxa-${templateName}, app=fxa, service=${
32183310
serviceName || 'fxa-auth-server'
@@ -3284,14 +3376,17 @@ async function setup(
32843376
config: Record<any, any>,
32853377
mocks: any,
32863378
locale = 'en',
3287-
sender: any = null
3379+
sender: any = null,
3380+
bounces: any = null,
3381+
statsd: any = null
32883382
) {
32893383
const Mailer = proxyquire(`${ROOT_DIR}/lib/senders/email`, mocks)(
32903384
log,
32913385
config,
3292-
{
3386+
bounces || {
32933387
check: () => Promise.resolve(),
3294-
}
3388+
},
3389+
statsd
32953390
);
32963391
return new Mailer(config.smtp, sender);
32973392
}

0 commit comments

Comments
 (0)