Skip to content

Commit 8410863

Browse files
committed
feat(auth): Wrap amplitude in config to enable/disable
Because: - We want to better understand what downstream consumers are of amplitude - And we need to understand what duplicate logging might not exist This Commit: - Adds a guard clause to the amplitude logging to return early if disabled - Adds tests to make sure it works Closes: FXA-13374
1 parent d82cf8c commit 8410863

3 files changed

Lines changed: 45 additions & 16 deletions

File tree

packages/fxa-auth-server/config/index.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,12 @@ const convictConf = convict({
297297
},
298298
},
299299
amplitude: {
300+
enabled: {
301+
default: true,
302+
doc: 'Enable Amplitude event logging',
303+
env: 'AMPLITUDE_ENABLED',
304+
format: Boolean,
305+
},
300306
schemaValidation: {
301307
default: true,
302308
doc: 'Validate events against a JSON schema',

packages/fxa-auth-server/lib/metrics/amplitude.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,9 @@ module.exports = (log, config) => {
211211
data = {},
212212
metricsContext = {}
213213
) {
214+
if (config.amplitude && config.amplitude.enabled === false) {
215+
return;
216+
}
214217
const statsd = Container.get(StatsD);
215218
if (!eventType || !request) {
216219
log.error('amplitude.badArgument', {

packages/fxa-auth-server/lib/metrics/amplitude.spec.ts

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
65
import { Container } from 'typedi';
76
import { StatsD } from 'hot-shots';
87

@@ -24,6 +23,7 @@ const amplitudeModule = require('./amplitude');
2423
const mockAmplitudeConfig = {
2524
schemaValidation: true,
2625
rawEvents: false,
26+
enabled: true,
2727
};
2828

2929
const DAY = 1000 * 60 * 60 * 24;
@@ -69,11 +69,42 @@ describe('metrics/amplitude', () => {
6969
});
7070
});
7171

72+
afterEach(() => {
73+
mockAmplitudeConfig.enabled = true;
74+
});
75+
7276
it('interface is correct', () => {
7377
expect(typeof amplitude).toBe('function');
7478
expect(amplitude.length).toBe(2);
7579
});
7680

81+
describe('disabling', () => {
82+
it('does not log events when disabled', async () => {
83+
// disable config and re-instantiate amplitude for just this test
84+
mockAmplitudeConfig.enabled = false;
85+
amplitude = amplitudeModule(log, {
86+
amplitude: mockAmplitudeConfig,
87+
oauth: {
88+
clientIds: {
89+
0: 'amo',
90+
1: 'pocket',
91+
},
92+
},
93+
verificationReminders: {
94+
firstInterval: 1000,
95+
secondInterval: 2000,
96+
thirdInterval: 3000,
97+
},
98+
});
99+
100+
return amplitude('account.created', mocks.mockRequest({})).then(() => {
101+
// could check other things, but this is the important one that
102+
// we want to disable when config.enabled is false
103+
expect(log.amplitudeEvent.callCount).toBe(0);
104+
});
105+
});
106+
});
107+
77108
// -----------------------------------------------------------------------
78109
// empty event argument
79110
// -----------------------------------------------------------------------
@@ -198,10 +229,7 @@ describe('metrics/amplitude', () => {
198229
1,
199230
'amplitude.event.raw'
200231
);
201-
expect(statsd.increment).toHaveBeenNthCalledWith(
202-
2,
203-
'amplitude.event'
204-
);
232+
expect(statsd.increment).toHaveBeenNthCalledWith(2, 'amplitude.event');
205233
});
206234
});
207235

@@ -401,9 +429,7 @@ describe('metrics/amplitude', () => {
401429
expect(args[0].user_properties['$append']).toEqual({
402430
fxa_services_used: 'undefined_oauth',
403431
});
404-
expect(args[0].user_properties.sync_active_devices_day).toBe(
405-
undefined
406-
);
432+
expect(args[0].user_properties.sync_active_devices_day).toBe(undefined);
407433
expect(args[0].user_properties.sync_active_devices_week).toBe(
408434
undefined
409435
);
@@ -834,10 +860,7 @@ describe('metrics/amplitude', () => {
834860

835861
describe(`email.${template}.sent`, () => {
836862
beforeEach(() => {
837-
return amplitude(
838-
`email.${template}.sent`,
839-
mocks.mockRequest({})
840-
);
863+
return amplitude(`email.${template}.sent`, mocks.mockRequest({}));
841864
});
842865

843866
it('did not call log.error', () => {
@@ -898,10 +921,7 @@ describe('metrics/amplitude', () => {
898921
it('incremented amplitude dropped', () => {
899922
const statsd = Container.get(StatsD) as { increment: jest.Mock };
900923
expect(statsd.increment).toHaveBeenCalledTimes(2);
901-
expect(statsd.increment).toHaveBeenNthCalledWith(
902-
1,
903-
'amplitude.event'
904-
);
924+
expect(statsd.increment).toHaveBeenNthCalledWith(1, 'amplitude.event');
905925
expect(statsd.increment).toHaveBeenNthCalledWith(
906926
2,
907927
'amplitude.event.dropped'

0 commit comments

Comments
 (0)