From f7ce4d02679871f722a574d2f0f5bc880c764451 Mon Sep 17 00:00:00 2001 From: Phil Booth Date: Mon, 29 Jan 2018 17:33:33 +0000 Subject: [PATCH 1/2] fix(metrics): ensure amplitude events always have a metrics context https://github.com/mozilla/fxa-auth-server/pull/2267 r=vbudhram --- lib/metrics/events.js | 11 ++++++++- test/local/metrics/events.js | 44 ++++++++++++++++++++++++------------ 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/lib/metrics/events.js b/lib/metrics/events.js index d3c2d36c..ca57c2ab 100644 --- a/lib/metrics/events.js +++ b/lib/metrics/events.js @@ -83,6 +83,7 @@ module.exports = (log, config) => { } const request = this + let isFlowCompleteSignal = false return P.resolve().then(() => { if (ACTIVITY_EVENTS.has(event)) { @@ -101,10 +102,18 @@ module.exports = (log, config) => { return emitFlowEvent(event, request, data) }) + .then(metricsContext => { + if (metricsContext) { + isFlowCompleteSignal = event === metricsContext.flowCompleteSignal + return metricsContext + } + + return request.gatherMetricsContext({}) + }) .then(metricsContext => { return amplitude(event, request, data, metricsContext) .then(() => { - if (metricsContext && event === metricsContext.flowCompleteSignal) { + if (isFlowCompleteSignal) { log.flowEvent(Object.assign({}, metricsContext, { event: 'flow.complete' })) return amplitude('flow.complete', request, data, metricsContext) .then(() => request.clearMetricsContext()) diff --git a/test/local/metrics/events.js b/test/local/metrics/events.js index f4094b55..5fec6e61 100644 --- a/test/local/metrics/events.js +++ b/test/local/metrics/events.js @@ -80,7 +80,7 @@ describe('metrics/events', () => { return events.emit.call(request, 'device.created', data) .then(() => { assert.equal(log.activityEvent.callCount, 1, 'log.activityEvent was called once') - const args = log.activityEvent.args[0] + let args = log.activityEvent.args[0] assert.equal(args.length, 1, 'log.activityEvent was passed one argument') assert.deepEqual(args[0], { event: 'device.created', @@ -89,8 +89,12 @@ describe('metrics/events', () => { uid: 'baz' }, 'argument was event data') + assert.equal(metricsContext.gather.callCount, 1, 'metricsContext.gather was called once') + args = metricsContext.gather.args[0] + assert.equal(args.length, 1, 'metricsContext.gather was passed one argument') + assert.deepEqual(args[0], {}, 'metricsContext.gather was passed an empty object') + assert.equal(log.amplitudeEvent.callCount, 0, 'log.amplitudeEvent was not called') - assert.equal(metricsContext.gather.callCount, 0, 'metricsContext.gather was not called') assert.equal(log.flowEvent.callCount, 0, 'log.flowEvent was not called') assert.equal(metricsContext.clear.callCount, 0, 'metricsContext.clear was not called') assert.equal(log.error.callCount, 0, 'log.error was not called') @@ -116,8 +120,9 @@ describe('metrics/events', () => { service: 'bar' }, 'argument was event data') + assert.equal(metricsContext.gather.callCount, 1, 'metricsContext.gather was called once') + assert.equal(log.amplitudeEvent.callCount, 0, 'log.amplitudeEvent was not called') - assert.equal(metricsContext.gather.callCount, 0, 'metricsContext.gather was not called') assert.equal(log.flowEvent.callCount, 0, 'log.flowEvent was not called') assert.equal(metricsContext.clear.callCount, 0, 'metricsContext.clear was not called') assert.equal(log.error.callCount, 0, 'log.error was not called') @@ -138,8 +143,9 @@ describe('metrics/events', () => { userAgent: 'test user-agent' }, 'argument was event data') + assert.equal(metricsContext.gather.callCount, 1, 'metricsContext.gather was called once') + assert.equal(log.amplitudeEvent.callCount, 0, 'log.amplitudeEvent was not called') - assert.equal(metricsContext.gather.callCount, 0, 'metricsContext.gather was not called') assert.equal(log.flowEvent.callCount, 0, 'log.flowEvent was not called') assert.equal(metricsContext.clear.callCount, 0, 'metricsContext.clear was not called') assert.equal(log.error.callCount, 0, 'log.error was not called') @@ -172,9 +178,14 @@ describe('metrics/events', () => { return events.emit.call(request, 'account.reminder') .then(() => { assert.equal(metricsContext.gather.callCount, 1, 'metricsContext.gather was called once') + let args = metricsContext.gather.args[0] + assert.equal(args.length, 1, 'metricsContext.gather was passed one argument') + assert.equal(args[0].event, 'account.reminder', 'metricsContext.gather was passed event') + assert.equal(args[0].locale, request.app.locale, 'metricsContext.gather was passed locale') + assert.equal(args[0].userAgent, request.headers['user-agent'], 'metricsContext.gather was passed user agent') assert.equal(log.flowEvent.callCount, 1, 'log.flowEvent was called once') - const args = log.flowEvent.args[0] + args = log.flowEvent.args[0] assert.equal(args.length, 1, 'log.flowEvent was passed one argument') assert.deepEqual(args[0], { event: 'account.reminder', @@ -481,9 +492,10 @@ describe('metrics/events', () => { badRequest: true }, 'argument was correct') + assert.equal(metricsContext.gather.callCount, 1, 'metricsContext.gather was called once') + assert.equal(log.activityEvent.callCount, 0, 'log.activityEvent was not called') assert.equal(log.amplitudeEvent.callCount, 0, 'log.amplitudeEvent was not called') - assert.equal(metricsContext.gather.callCount, 0, 'metricsContext.gather was not called') assert.equal(log.flowEvent.callCount, 0, 'log.flowEvent was not called') assert.equal(metricsContext.clear.callCount, 0, 'metricsContext.clear was not called') }) @@ -597,15 +609,16 @@ describe('metrics/events', () => { }) it('.emit with content-server account.signed event', () => { - const metricsContext = mocks.mockMetricsContext() + const flowBeginTime = Date.now() - 1 + const metricsContext = mocks.mockMetricsContext({ + gather: sinon.spy(() => ({ + device_id: 'foo', + flow_id: 'bar', + flowBeginTime + })) + }) const request = mocks.mockRequest({ metricsContext, - payload: { - metricsContext: { - flowId: 'bar', - flowBeginTime: Date.now() - 1 - } - }, query: { service: 'content-server' } @@ -620,6 +633,8 @@ describe('metrics/events', () => { assert.equal(log.amplitudeEvent.callCount, 1, 'log.amplitudeEvent was called once') assert.equal(log.amplitudeEvent.args[0].length, 1, 'log.amplitudeEvent was passed one argument') assert.equal(log.amplitudeEvent.args[0][0].event_type, 'fxa_activity - cert_signed', 'log.amplitudeEvent was passed correct event_type') + assert.equal(log.amplitudeEvent.args[0][0].device_id, 'foo', 'log.amplitudeEvent was passed correct device_id') + assert.equal(log.amplitudeEvent.args[0][0].session_id, flowBeginTime, 'log.amplitudeEvent was passed correct session_id') assert.deepEqual(log.amplitudeEvent.args[0][0].event_properties, { service: undefined, oauth_client_id: undefined @@ -631,7 +646,8 @@ describe('metrics/events', () => { ua_version: request.app.ua.browserVersion }, 'log.amplitudeEvent was passed correct user properties') - assert.equal(metricsContext.gather.callCount, 0, 'metricsContext.gather was not called') + assert.equal(metricsContext.gather.callCount, 1, 'metricsContext.gather was called once') + assert.equal(log.flowEvent.callCount, 0, 'log.flowEvent was not called') assert.equal(metricsContext.clear.callCount, 0, 'metricsContext.clear was not called') assert.equal(log.error.callCount, 0, 'log.error was not called') From e19697b799047fb59c21e9b1ccf2944bfb29def3 Mon Sep 17 00:00:00 2001 From: Phil Booth Date: Mon, 29 Jan 2018 17:34:18 +0000 Subject: [PATCH 2/2] Release v1.104.1 --- CHANGELOG.md | 10 ++++++++++ npm-shrinkwrap.json | 2 +- package.json | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2097a6af..a04fb192 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ + +## [1.104.1](https://github.com/mozilla/fxa-auth-server/compare/v1.104.0...v1.104.1) (2018-01-29) + + +### Bug Fixes + +* **metrics:** ensure amplitude events always have a metrics context ([f7ce4d0](https://github.com/mozilla/fxa-auth-server/commit/f7ce4d0)) + + + # [1.104.0](https://github.com/mozilla/fxa-auth-server/compare/v1.103.0...v1.104.0) (2018-01-24) diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index d6aa4779..7e48a029 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -1,6 +1,6 @@ { "name": "fxa-auth-server", - "version": "1.104.0", + "version": "1.104.1", "dependencies": { "acorn": { "version": "5.0.3", diff --git a/package.json b/package.json index 754245bc..35cbe735 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "fxa-auth-server", - "version": "1.104.0", + "version": "1.104.1", "description": "Firefox Accounts, an identity provider for Mozilla cloud services", "bin": { "fxa-auth": "./bin/key_server.js"