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

feat(metrics): metrics flow for iframeless flow #6227

Merged
merged 2 commits into from May 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions app/scripts/lib/metrics.js
Expand Up @@ -206,6 +206,7 @@ define(function (require, exports, module) {
}

const flowModel = new Flow({
metrics: this,
sentryMetrics: this._sentryMetrics,
window: this._window
});
Expand Down Expand Up @@ -458,6 +459,18 @@ define(function (require, exports, module) {
}
},

/**
* Marks some event already logged in metrics memory.
*
* Used in conjunction with `logEventOnce` when we know that some event was already logged elsewhere.
* Helps avoid event duplication.
*
* @param {String} eventName
*/
markEventLogged: function (eventName) {
this._eventMemory[eventName] = true;
},

/**
* Start a timer
*
Expand Down
131 changes: 68 additions & 63 deletions app/scripts/models/flow.js
Expand Up @@ -11,79 +11,84 @@
* If unavailable there, fetch from data attributes in the DOM.
*/

define(function (require, exports, module) {
'use strict';

const $ = require('jquery');
const AuthErrors = require('../lib/auth-errors');
const Backbone = require('backbone');
const Cocktail = require('cocktail');
const ErrorUtils = require('../lib/error-utils');
const ResumeTokenMixin = require('./mixins/resume-token');
const SearchParamMixin = require('./mixins/search-param');
const vat = require('../lib/vat');
const $ = require('jquery');
const AuthErrors = require('../lib/auth-errors');
const Backbone = require('backbone');
const Cocktail = require('cocktail');
const ErrorUtils = require('../lib/error-utils');
const ResumeTokenMixin = require('./mixins/resume-token');
const SearchParamMixin = require('./mixins/search-param');
const vat = require('../lib/vat');
const Url = require('../lib/url');

var Model = Backbone.Model.extend({
initialize (options) {
options = options || {};
var Model = Backbone.Model.extend({
initialize (options) {
options = options || {};

this.sentryMetrics = options.sentryMetrics;
this.window = options.window || window;

// We should either get both fields from the resume token, or neither.
// Getting one without the other is an error.
this.populateFromStringifiedResumeToken(this.getSearchParam('resume'));
if (this.has('flowId')) {
if (! this.has('flowBegin')) {
this.logError(AuthErrors.toMissingResumeTokenPropertyError('flowBegin'));
}
} else if (this.has('flowBegin')) {
this.logError(AuthErrors.toMissingResumeTokenPropertyError('flowId'));
} else {
this.populateFromDataAttribute('flowId');
this.populateFromDataAttribute('flowBegin');
this.sentryMetrics = options.sentryMetrics;
this.window = options.window || window;
this.metrics = options.metrics;
const urlParams = Url.searchParams(this.window.location.search);
// We should either get both fields from the resume token, or neither.
// Getting one without the other is an error.
this.populateFromStringifiedResumeToken(this.getSearchParam('resume'));
if (this.has('flowId')) {
if (! this.has('flowBegin')) {
this.logError(AuthErrors.toMissingResumeTokenPropertyError('flowBegin'));
}
},
} else if (this.has('flowBegin')) {
this.logError(AuthErrors.toMissingResumeTokenPropertyError('flowId'));
} else if (urlParams.flow_begin_time && urlParams.flow_id) {
this.set('flowId', urlParams.flow_id);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should flow_begin_time and flow_id be checked for validity? The schema below is even used when populating from the data attributes.

this.set('flowBegin', urlParams.flow_begin_time);
// if the urlParams are set that means flow.begin was already logged on the server-side
// therefore we mark flow.begin logged
this.metrics.markEventLogged('flow.begin');
} else {
this.populateFromDataAttribute('flowId');
this.populateFromDataAttribute('flowBegin');
}
},

defaults: {
flowBegin: null,
flowId: null
},
defaults: {
flowBegin: null,
flowId: null
},

populateFromDataAttribute (attribute) {
var data = $(this.window.document.body).data(attribute);
if (! data) {
this.logError(AuthErrors.toMissingDataAttributeError(attribute));
populateFromDataAttribute (attribute) {
var data = $(this.window.document.body).data(attribute);
if (! data) {
this.logError(AuthErrors.toMissingDataAttributeError(attribute));
} else {
const result = this.resumeTokenSchema[attribute].validate(data);
if (result.error) {
this.logError(AuthErrors.toInvalidDataAttributeError(attribute));
} else {
const result = this.resumeTokenSchema[attribute].validate(data);
if (result.error) {
this.logError(AuthErrors.toInvalidDataAttributeError(attribute));
} else {
this.set(attribute, result.value);
}
this.set(attribute, result.value);
}
},
}
},

logError (error) {
return ErrorUtils.captureError(error, this.sentryMetrics);
},
logError (error) {
return ErrorUtils.captureError(error, this.sentryMetrics);
},

resumeTokenFields: ['flowId', 'flowBegin'],
resumeTokenFields: ['flowId', 'flowBegin'],

resumeTokenSchema: {
flowBegin: vat.number().test(function (value) {
// Integers only
return value === Math.round(value);
}),
flowId: vat.hex().len(64)
}
});
resumeTokenSchema: {
flowBegin: vat.number().test(function (value) {
// Integers only
return value === Math.round(value);
}),
flowId: vat.hex().len(64)
}
});

Cocktail.mixin(
Model,
ResumeTokenMixin,
SearchParamMixin
);
Cocktail.mixin(
Model,
ResumeTokenMixin,
SearchParamMixin
);

module.exports = Model;
});
module.exports = Model;
12 changes: 12 additions & 0 deletions app/tests/spec/lib/metrics.js
Expand Up @@ -240,6 +240,18 @@ define(function (require, exports, module) {
});
});

describe('markEventLogged', function () {
it('does not log an event if marked logged', function () {
metrics.markEventLogged('event2');
metrics.logEventOnce('event1');
metrics.logEventOnce('event2');

const filteredData = metrics.getFilteredData();
assert.equal(filteredData.events.length, 1);
assert.equal(filteredData.events[0].type, 'event1');
});
});

describe('startTimer/stopTimer', function () {
it('adds a timer to output data', function () {
metrics.startTimer('timer1');
Expand Down
26 changes: 26 additions & 0 deletions app/tests/spec/models/flow.js
Expand Up @@ -21,18 +21,23 @@ define(function (require, exports, module) {
var flow;
var sentryMetricsMock;
var windowMock;
var metricsMock;

beforeEach(function () {
sentryMetricsMock = {
captureException: sinon.spy()
};
metricsMock = {
markEventLogged: sinon.spy()
};
windowMock = new WindowMock();
$(windowMock.document.body).removeData('flowId').removeAttr('data-flow-id');
$(windowMock.document.body).removeData('flowBegin').removeAttr('data-flow-begin');
});

function createFlow () {
flow = new Flow({
metrics: metricsMock,
sentryMetrics: sentryMetricsMock,
window: windowMock
});
Expand Down Expand Up @@ -76,6 +81,27 @@ define(function (require, exports, module) {
assert.equal(flow.get('flowBegin'), 42);
});

it('fetches from query parameters, if available', function () {
$(windowMock.document.body).attr('data-flow-id', BODY_FLOW_ID);
$(windowMock.document.body).attr('data-flow-begin', '42');

const QUERY_FLOW_BEGIN = '55';
const QUERY_FLOW_ID = 'A1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103';

windowMock.location.search = Url.objToSearchString({
/*eslint-disable camelcase*/
flow_begin_time: QUERY_FLOW_BEGIN,
flow_id: QUERY_FLOW_ID
/*eslint-enable camelcase*/
});

createFlow();

assert.equal(flow.get('flowId'), QUERY_FLOW_ID);
assert.equal(flow.get('flowBegin'), QUERY_FLOW_BEGIN);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this test also assert that markEventLogged is called correctly? (or if not here then somewhere)

assert.ok(metricsMock.markEventLogged.calledOnce);
});

it('logs an error when the resume token contains `flowId` but not `flowBegin`', function () {
windowMock.location.search = Url.objToSearchString({
resume: ResumeToken.stringify({ flowId: RESUME_FLOW_ID })
Expand Down
13 changes: 13 additions & 0 deletions server/bin/fxa-content-server.js
Expand Up @@ -166,6 +166,19 @@ function makeApp() {
// it's a four-oh-four not found.
app.use(fourOhFour);

// Handler for CORS errors
app.use((err, req, res, next) => {
if (err.message === 'CORS Error') {
res.status(401).json({
error: 'Unauthorized',
message: 'CORS Error',
statusCode: 401
});
} else {
next(err);
}
});

// The error handler must be before any other error middleware
app.use(raven.ravenModule.errorHandler());

Expand Down
1 change: 1 addition & 0 deletions server/config/local.json-dist
Expand Up @@ -21,6 +21,7 @@
"enabled": true
},
"static_directory": "app",
"allowed_metrics_flow_cors_origins": ["http://127.0.0.1:8001"],
"allowed_parent_origins": ["http://127.0.0.1:8080"],
"csp": {
"enabled": true,
Expand Down
6 changes: 6 additions & 0 deletions server/lib/configuration.js
Expand Up @@ -18,6 +18,12 @@ const conf = module.exports = convict({
doc: 'context query parameters allowed to embed FxA within an IFRAME',
format: Array
},
allowed_metrics_flow_cors_origins: {
default: [],
doc: 'Origins that are allowed to request the /metrics-flow endpoint',
env: 'ALLOWED_METRICS_FLOW_ORIGINS',
format: Array
},
allowed_parent_origins: {
default: [],
doc: 'Origins that are allowed to embed FxA within an IFRAME',
Expand Down
7 changes: 6 additions & 1 deletion server/lib/flow-event.js
Expand Up @@ -108,7 +108,7 @@ const PERFORMANCE_TIMINGS = [

const AUTH_VIEWS = new Set([ 'enter-email', 'force-auth', 'signin', 'signup' ]);

module.exports = (req, metrics, requestReceivedTime) => {
const metricsRequest = (req, metrics, requestReceivedTime) => {
if (IS_DISABLED || ! isValidFlowData(metrics, requestReceivedTime)) {
return;
}
Expand Down Expand Up @@ -292,3 +292,8 @@ function optionallySetFallbackData (eventData, key, fallback) {
eventData[key] = limitLength(fallback);
}
}

module.exports = {
logFlowEvent,
metricsRequest
};
1 change: 1 addition & 0 deletions server/lib/routes.js
Expand Up @@ -45,6 +45,7 @@ module.exports = function (config, i18n) {
require('./routes/get-lbheartbeat')(),
require('./routes/get-openid-configuration')(config),
require('./routes/get-version.json'),
require('./routes/get-metrics-flow')(config),
require('./routes/post-metrics')(),
require('./routes/post-metrics-errors')(),
require('./routes/redirect-complete-to-verified')(),
Expand Down
56 changes: 56 additions & 0 deletions server/lib/routes/get-metrics-flow.js
@@ -0,0 +1,56 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

'use strict';

const flowMetrics = require('../flow-metrics');
const logFlowEvent = require('../flow-event').logFlowEvent;
const logger = require('../logging/log')('server.get-metrics-flow');

module.exports = function (config) {
const FLOW_ID_KEY = config.get('flow_id_key');
const FLOW_EVENT_NAME = 'flow.begin';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philbooth what flow event name should we use here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flow.begin is correct (there's a whole bunch of post-processing that's keyed on it, so it's not really a choice at this point tbh)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also need to remove app/scripts/views/mixins/flow-begin-mixin.js from the client-side of course. Probably you already realised that, just making sure.

Any places that were mixing it in can mix in flow-events-mixin.js instead now, I think, and everything should just magically work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, We cannot just remove flow-begin from client-side because there are still iframe versions of the flow. So we either not record a begin if there was a query flow sent or some other solution

Copy link
Contributor

@philbooth philbooth May 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point, lol. Yeah, maybe add some condition to the begin mixin so it doesn't emit the event if the query params are set?

const ALLOWED_CORS_ORIGINS = config.get('allowed_metrics_flow_cors_origins');
const CORS_OPTIONS = {
methods: 'GET',
origin: function corsOrigin(origin, callback) {
if (ALLOWED_CORS_ORIGINS.includes(origin)) {
callback(null, true);
} else {
logger.info('request.metrics-flow.bad-origin', origin);
callback(new Error('CORS Error'));
}
},
preflightContinue: false
};

const route = {};
route.method = 'get';
route.path = '/metrics-flow';
route.cors = CORS_OPTIONS;

route.process = function (req, res) {
const flowEventData = flowMetrics.create(FLOW_ID_KEY, req.headers['user-agent']);
const flowBeingTime = flowEventData.flowBeginTime;
const flowId = flowEventData.flowId;
const metricsData = req.query || {};

metricsData.flowId = flowId;

logFlowEvent({
flowTime: flowBeingTime,
time: flowBeingTime,
type: FLOW_EVENT_NAME
}, metricsData, req);

// charset must be set on json responses.
res.charset = 'utf-8';
res.json({
flowBeginTime: flowEventData.flowBeginTime,
flowId: flowId
});
};

return route;
};
4 changes: 2 additions & 2 deletions server/lib/routes/post-metrics.js
Expand Up @@ -6,7 +6,7 @@

const _ = require('lodash');
const config = require('../configuration');
const flowEvent = require('../flow-event');
const flowMetricsRequest = require('../flow-event').metricsRequest;
const GACollector = require('../ga-collector');
const joi = require('joi');
const logger = require('../logging/log')('server.post-metrics');
Expand Down Expand Up @@ -174,7 +174,7 @@ module.exports = function () {
}
ga.write(metrics);

flowEvent(req, metrics, requestReceivedTime);
flowMetricsRequest(req, metrics, requestReceivedTime);
});
}
};
Expand Down
2 changes: 1 addition & 1 deletion tests/server/flow-event.js
Expand Up @@ -44,7 +44,7 @@ registerSuite('flow-event', {
'./amplitude': mocks.amplitude,
'./configuration': mocks.config,
'./flow-metrics': mocks.flowMetrics
});
}).metricsRequest;
},

afterEach: function() {
Expand Down