From 6c5ca6d3555c21b556b3b372bfd820017bc6c608 Mon Sep 17 00:00:00 2001 From: Muhammad Farhan Date: Tue, 26 Mar 2019 02:08:55 +0530 Subject: [PATCH] fix(metrics): add independent config to disable amplitude/flow events --- server/lib/amplitude.js | 6 ++++-- server/lib/configuration.js | 14 +++++++++++++ server/lib/flow-event.js | 5 ++--- tests/server/amplitude.js | 40 ++++++++++++++++++++++++++++++++----- tests/server/flow-event.js | 34 +++++++++++++++++++++++++++---- 5 files changed, 85 insertions(+), 14 deletions(-) diff --git a/server/lib/amplitude.js b/server/lib/amplitude.js index 63b0659ac8..7fea1acbbc 100644 --- a/server/lib/amplitude.js +++ b/server/lib/amplitude.js @@ -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 = { @@ -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; } diff --git a/server/lib/configuration.js b/server/lib/configuration.js index 8600639d12..17fdb3886e 100644 --- a/server/lib/configuration.js +++ b/server/lib/configuration.js @@ -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', @@ -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', diff --git a/server/lib/flow-event.js b/server/lib/flow-event.js index 2fedd2a9cd..8da2c67318 100644 --- a/server/lib/flow-event.js +++ b/server/lib/flow-event.js @@ -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})$/; @@ -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 @@ -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; } diff --git a/tests/server/amplitude.js b/tests/server/amplitude.js index 51e3d7618e..18c431666f 100644 --- a/tests/server/amplitude.js +++ b/tests/server/amplitude.js @@ -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 @@ -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(() => {}); }, @@ -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')); diff --git a/tests/server/flow-event.js b/tests/server/flow-event.js index cc52363438..e6b814e09f 100644 --- a/tests/server/flow-event.js +++ b/tests/server/flow-event.js @@ -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(); @@ -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;