Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Commit

Permalink
fix(metrics): add independent config to disable amplitude/flow events
Browse files Browse the repository at this point in the history
  • Loading branch information
farhan787 committed Mar 29, 2019
1 parent eeef718 commit 6c5ca6d
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 14 deletions.
6 changes: 4 additions & 2 deletions server/lib/amplitude.js
Expand Up @@ -17,8 +17,10 @@
const { GROUPS, initialize } = require('fxa-shared/metrics/amplitude');
const logger = require('./logging/log')();
const ua = require('./user-agent');
const config = require('./configuration');

const SERVICES = require('./configuration').get('oauth_client_id_map');
const SERVICES = config.get('oauth_client_id_map');
const amplitude = config.get('amplitude');

// Maps view name to email type
const EMAIL_TYPES = {
Expand Down Expand Up @@ -133,7 +135,7 @@ const transform = initialize(SERVICES, EVENTS, FUZZY_EVENTS);
module.exports = receiveEvent;

function receiveEvent (event, request, data) {
if (! event || ! request || ! data) {
if (amplitude.disabled || ! event || ! request || ! data) {
return;
}

Expand Down
14 changes: 14 additions & 0 deletions server/lib/configuration.js
Expand Up @@ -30,6 +30,14 @@ const conf = module.exports = convict({
env: 'ALLOWED_PARENT_ORIGINS',
format: Array
},
amplitude: {
disabled: {
default: false,
doc: 'Disable amplitude events',
env: 'AMPLITUDE_DISABLED',
format: Boolean
}
},
are_dist_resources: {
default: false,
doc: 'Check if the resources are under the /dist directory',
Expand Down Expand Up @@ -217,6 +225,12 @@ const conf = module.exports = convict({
env: 'FLOW_ID_KEY',
format: String
},
flow_metrics_disabled: {
default: false,
doc: 'Disable flow metrics output to stderr',
env: 'FLOW_METRICS_DISABLED',
format: Boolean
},
fxa_client_configuration: {
max_age: {
default: '1 day',
Expand Down
5 changes: 2 additions & 3 deletions server/lib/flow-event.js
Expand Up @@ -29,6 +29,7 @@ const VERSION = 1;
const FLOW_BEGIN_EVENT = 'flow.begin';
const FLOW_ID_KEY = config.get('flow_id_key');
const FLOW_ID_EXPIRY = config.get('flow_id_expiry');
const FLOW_METRICS_DISABLED = config.get('flow_metrics_disabled');

const ENTRYPOINT_PATTERN = /^[\w.-]+$/;
const SERVICE_PATTERN = /^(sync|content-server|none|[0-9a-f]{16})$/;
Expand All @@ -44,8 +45,6 @@ const VALID_FLOW_EVENT_PROPERTIES = [

const UTM_PATTERN = /^[\w.%-]+$/;

const IS_DISABLED = config.get('client_metrics').stderr_collector_disabled;

const PERFORMANCE_TIMINGS = [
// These timings are only an approximation, to be used as extra signals
// when looking for correlations in the flow data. They're not perfect
Expand Down Expand Up @@ -84,7 +83,7 @@ const PERFORMANCE_TIMINGS = [
const AUTH_VIEWS = new Set([ 'enter-email', 'force-auth', 'signin', 'signup' ]);

const metricsRequest = (req, metrics, requestReceivedTime) => {
if (IS_DISABLED || ! isValidFlowData(metrics, requestReceivedTime)) {
if (FLOW_METRICS_DISABLED || ! isValidFlowData(metrics, requestReceivedTime)) {
return;
}

Expand Down
40 changes: 35 additions & 5 deletions tests/server/amplitude.js
Expand Up @@ -11,13 +11,21 @@ const pkg = require('../../package.json');
const logger = {
info: sinon.spy()
};
const amplitudeConfig = {
disabled: false
};

const amplitude = proxyquire(path.resolve('server/lib/amplitude'), {
'./configuration': {
get () {
return {
'0': 'amo',
'1': 'pocket'
};
get (name) {
if (name === 'oauth_client_id_map') {
return {
'0': 'amo',
'1': 'pocket'
};
} else if (name === 'amplitude') {
return amplitudeConfig;
}
}
},
'./logging/log': () => logger
Expand All @@ -26,6 +34,7 @@ const APP_VERSION = /^[0-9]+\.([0-9]+)\./.exec(pkg.version)[1];

registerSuite('amplitude', {
beforeEach: function() {
amplitudeConfig.disabled = false;
sinon.stub(process.stderr, 'write').callsFake(() => {});
},

Expand All @@ -46,6 +55,27 @@ registerSuite('amplitude', {
assert.lengthOf(amplitude, 3);
},

'disable writing amplitude events': {
'logger.info was not called': () => {
amplitudeConfig.disabled = true;
amplitude({
time: 'a',
type: 'flow.signin_from.bar'
}, {
connection: {},
headers: {
'x-forwarded-for': '63.245.221.32'
}
}, {
flowBeginTime: 'b',
flowId: 'c',
uid: 'd'
});

assert.equal(logger.info.callCount, 0);
}
},

'does not throw if arguments are missing': () => {
assert.doesNotThrow(amplitude);
assert.doesNotThrow(() => amplitude('foo'));
Expand Down
34 changes: 30 additions & 4 deletions tests/server/flow-event.js
Expand Up @@ -13,11 +13,9 @@ registerSuite('flow-event', {
beforeEach: function() {
config = {
/*eslint-disable camelcase*/
client_metrics: {
stderr_collector_disabled: false
},
flow_id_expiry: 7200000,
flow_id_key: 'foo'
flow_id_key: 'foo',
flow_metrics_disabled: false
/*eslint-enable camelcase*/
};
sandbox = sinon.sandbox.create();
Expand Down Expand Up @@ -60,6 +58,34 @@ registerSuite('flow-event', {
assert.lengthOf(flowEvent, 3);
},

'call flowEvent with flow_metrics_disabled true': {
beforeEach() {
/*eslint-disable camelcase*/
config.flow_metrics_disabled = true;
/*eslint-enable camelcase*/

const timeSinceFlowBegin = 1000;
flowMetricsValidateResult = true;
setup({
events: [
{offset: 5, type: 'wibble'},
{offset: 5, type: 'flow.begin'},
{offset: 5.9, type: 'screen.signup'},
{offset: timeSinceFlowBegin, type: 'flow.signup.good-offset-now'},
{offset: timeSinceFlowBegin + 1, type: 'flow.signup.bad-offset-future'},
{offset: timeSinceFlowBegin - config.flow_id_expiry - 1, type: 'flow.signup.bad-offset-expired'},
{offset: timeSinceFlowBegin - config.flow_id_expiry, type: 'flow.signup.good-offset-oldest'},
{offset: 500, type: 'flow.timing.foo.1'},
{offset: 500, type: 'flow.timing.bar.baz.2'}
],
}, timeSinceFlowBegin);
},

'process.stderr.write was not called': () => {
assert.equal(process.stderr.write.callCount, 0);
}
},

'call flowEvent with valid flow data': {
beforeEach() {
const timeSinceFlowBegin = 1000;
Expand Down

0 comments on commit 6c5ca6d

Please sign in to comment.