From e81aa0642860fa47e2afd41a07af4db37332c900 Mon Sep 17 00:00:00 2001 From: Shane Tomlinson Date: Mon, 8 Sep 2014 13:27:04 +0100 Subject: [PATCH] chore(metrics): Namespace errors for metrics logging and other error logging updates. @kparlante asked for us to add a bit of context, like a screen name, to errors so that it is easier to decide where an error originated. This led down a bit of a rabbit hole of cleaning up the error code. * The different error types overlap in their errno's, this adds a namespace so that different errors are able to be selected from the metrics logs. Namespaces used: * auth-errors.js: 'auth' * oauth-errors.js: 'oauth' * profile-client.js: 'profile' * The `message` field of auth-errors.js.toError was never used, so this is repuprosed to pass in a `context`. * The format of an error event has changed to accommodate a context: 'error... * When a view's `logError` function is called, the screen name is set as the error's context. * base.js called metrics.js to fetch ids for screens and errors, then called metrics.js->logEvent. Updated to just call `logError` and `logScreen` * A couple of TestHelper functions were added: `isScreenLogged` and `isErrorLogged` fixes #1500 --- app/scripts/lib/auth-errors.js | 17 ++++++++++++-- app/scripts/lib/metrics.js | 36 +++++++++++++++++++++------- app/scripts/lib/oauth-errors.js | 3 ++- app/scripts/lib/profile-client.js | 3 ++- app/scripts/views/base.js | 20 ++++++++++++---- app/tests/lib/helpers.js | 14 ++++++++++- app/tests/spec/lib/auth-errors.js | 18 ++++++++++++++ app/tests/spec/lib/metrics.js | 31 ++++++++++++++++++++++-- app/tests/spec/views/base.js | 39 +++++++++++++++++-------------- app/tests/spec/views/form.js | 6 +---- 10 files changed, 145 insertions(+), 42 deletions(-) diff --git a/app/scripts/lib/auth-errors.js b/app/scripts/lib/auth-errors.js index 57613c17b..a98e7e995 100644 --- a/app/scripts/lib/auth-errors.js +++ b/app/scripts/lib/auth-errors.js @@ -97,6 +97,7 @@ function () { return { ERROR_TO_CODE: ERROR_TO_CODE, CODE_TO_MESSAGES: CODE_TO_MESSAGES, + NAMESPACE: 'auth', /** * Convert an error, a numeric code or string type to a message @@ -161,11 +162,23 @@ function () { }, /** - * Synthesize an error of the given type with the supplied message. + * Synthesize an error of the given type + * + * @param {String || Number || Object} type + * @param {String} [context] */ - toError: function (type, message) { + toError: function (type, context) { + var message = this.toMessage(type); + var err = new Error(message); err.errno = this.toCode(type); + err.namespace = this.NAMESPACE; + err.message = message; + + if (context) { + err.context = context; + } + return err; }, diff --git a/app/scripts/lib/metrics.js b/app/scripts/lib/metrics.js index fcc62a58a..4b0180a03 100644 --- a/app/scripts/lib/metrics.js +++ b/app/scripts/lib/metrics.js @@ -20,8 +20,9 @@ define([ 'jquery', 'speedTrap', 'lib/promise', - 'lib/url' -], function (_, Backbone, $, speedTrap, p, Url) { + 'lib/url', + 'lib/strings' +], function (_, Backbone, $, speedTrap, p, Url, Strings) { 'use strict'; // Speed trap is a singleton, convert it @@ -205,17 +206,36 @@ define([ }, /** - * Convert an error to an identifier + * Log an error. */ - errorToId: function (err, errors) { - return 'error.' + errors.toCode(err); + logError: function (error) { + this.logEvent(this.errorToId(error)); }, /** - * Convert a pathname from a URL to an identifier + * Convert an error to an identifier that can be used for logging. */ - pathToId: function (path) { - return 'screen.' + Url.pathToScreenName(path); + errorToId: function (error) { + var id = Strings.interpolate('error.%s.%s.%s', [ + error.context || 'unknown context', + error.namespace || 'unknown namespace', + error.errno || String(error) + ]); + return id; + }, + + /** + * Log a screen + */ + logScreen: function (screenName) { + this.logEvent(this.screenToId(screenName)); + }, + + /** + * Convert a screenName an identifier + */ + screenToId: function (screenName) { + return 'screen.' + screenName; }, /** diff --git a/app/scripts/lib/oauth-errors.js b/app/scripts/lib/oauth-errors.js index 8754cae5c..8ad7e7585 100644 --- a/app/scripts/lib/oauth-errors.js +++ b/app/scripts/lib/oauth-errors.js @@ -44,6 +44,7 @@ function (_, AuthErrors) { return _.extend({}, AuthErrors, { ERROR_TO_CODE: ERROR_TO_CODE, - CODE_TO_MESSAGES: CODE_TO_MESSAGES + CODE_TO_MESSAGES: CODE_TO_MESSAGES, + NAMESPACE: 'oauth' }); }); diff --git a/app/scripts/lib/profile-client.js b/app/scripts/lib/profile-client.js index 0f50943ca..6b461ff27 100644 --- a/app/scripts/lib/profile-client.js +++ b/app/scripts/lib/profile-client.js @@ -82,7 +82,8 @@ function ($, _, p, Session, ConfigLoader, OAuthClient, Assertion, AuthErrors) { var ProfileErrors = ProfileClient.Errors = _.extend({}, AuthErrors, { ERROR_TO_CODE: ERROR_TO_CODE, - CODE_TO_MESSAGES: CODE_TO_MESSAGES + CODE_TO_MESSAGES: CODE_TO_MESSAGES, + NAMESPACE: 'profile' }); return ProfileClient; diff --git a/app/scripts/views/base.js b/app/scripts/views/base.js index 142611477..171694b56 100644 --- a/app/scripts/views/base.js +++ b/app/scripts/views/base.js @@ -162,7 +162,7 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings, * authentication, but this could be extended to other types of * authorization as well. */ - isUserAuthorized: function() { + isUserAuthorized: function () { if (this.mustAuth) { return this.fxaClient.isSignedIn(Session.sessionToken); } @@ -381,7 +381,16 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings, } err.logged = true; - this.logEvent(this.metrics.errorToId(err, errors)); + if (typeof console !== 'undefined' && console) { + console.error(err.message || String(err)); + } + + this.metrics.logError(err); + }, + + getScreenName: function () { + var screenName = Url.pathToScreenName(this.window.location.pathname); + return screenName; }, _normalizeError: function (err, errors) { @@ -395,6 +404,10 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings, } } + if (! err.context) { + err.context = this.getScreenName(); + } + return err; }, @@ -402,8 +415,7 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings, * Log the current screen */ logScreen: function () { - var path = this.window.location.pathname; - this.logEvent(this.metrics.pathToId(path)); + this.metrics.logScreen(this.getScreenName()); }, /** diff --git a/app/tests/lib/helpers.js b/app/tests/lib/helpers.js index bf8ce9689..2ceaf82bb 100644 --- a/app/tests/lib/helpers.js +++ b/app/tests/lib/helpers.js @@ -99,6 +99,16 @@ define([ return false; } + function isErrorLogged(metrics, error) { + var eventName = metrics.errorToId(error); + return isEventLogged(metrics, eventName); + } + + function isScreenLogged(metrics, screenName) { + var eventName = metrics.screenToId(screenName); + return isEventLogged(metrics, eventName); + } + return { requiresFocus: requiresFocus, addFxaClientSpy: addFxaClientSpy, @@ -107,6 +117,8 @@ define([ createRandomHexString: createRandomHexString, createEmail: createEmail, emailToUser: emailToUser, - isEventLogged: isEventLogged + isEventLogged: isEventLogged, + isErrorLogged: isErrorLogged, + isScreenLogged: isScreenLogged }; }); diff --git a/app/tests/spec/lib/auth-errors.js b/app/tests/spec/lib/auth-errors.js index 030509b5f..03437d4aa 100644 --- a/app/tests/spec/lib/auth-errors.js +++ b/app/tests/spec/lib/auth-errors.js @@ -15,6 +15,24 @@ function (chai, AuthErrors) { var assert = chai.assert; describe('lib/auth-errors', function () { + describe('toError', function () { + it('converts a string to an Error object with expected fields', function () { + var err = AuthErrors.toError('INVALID_TOKEN'); + assert.isTrue(err instanceof Error); + + assert.equal(err.errno, 110); + assert.equal(err.namespace, 'auth'); + assert.equal(err.message, AuthErrors.toMessage('INVALID_TOKEN')); + }); + + it('sets `context` field of error if given', function () { + var err = AuthErrors.toError('INVALID_TOKEN', 'the context'); + assert.isTrue(err instanceof Error); + + assert.equal(err.context, 'the context'); + }); + }); + describe('toMessage', function () { it('converts a code to a message', function () { assert.equal(AuthErrors.toMessage(102), 'Unknown account'); diff --git a/app/tests/spec/lib/metrics.js b/app/tests/spec/lib/metrics.js index 9577e5969..c0684fc0c 100644 --- a/app/tests/spec/lib/metrics.js +++ b/app/tests/spec/lib/metrics.js @@ -8,9 +8,11 @@ define([ 'chai', 'jquery', 'lib/metrics', - '../../mocks/window' + 'lib/auth-errors', + '../../mocks/window', + '../../lib/helpers' ], -function (chai, $, Metrics, WindowMock) { +function (chai, $, Metrics, AuthErrors, WindowMock, TestHelpers) { 'use strict'; /*global describe, it*/ @@ -186,5 +188,30 @@ function (chai, $, Metrics, WindowMock) { }); }); }); + + describe('errorToId', function () { + it('converts an error into an id that can be used for logging', function () { + var error = AuthErrors.toError('UNEXPECTED_ERROR', 'signup'); + + var id = metrics.errorToId(error); + assert.equal(id, 'error.signup.auth.999'); + }); + }); + + describe('logError', function () { + it('logs an error', function () { + var error = AuthErrors.toError('UNEXPECTED_ERROR', 'signup'); + + metrics.logError(error); + assert.isTrue(TestHelpers.isErrorLogged(metrics, error)); + }); + }); + + describe('logScreen', function () { + it('logs the screen', function () { + metrics.logScreen('signup'); + assert.isTrue(TestHelpers.isScreenLogged(metrics, 'signup')); + }); + }); }); }); diff --git a/app/tests/spec/views/base.js b/app/tests/spec/views/base.js index ebb3d5ebd..37ae276f3 100644 --- a/app/tests/spec/views/base.js +++ b/app/tests/spec/views/base.js @@ -39,6 +39,7 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, var metrics; var fxaClient; var relier; + var screenName = 'screen'; beforeEach(function () { translator = new Translator('en-US', ['en-US']); @@ -49,6 +50,7 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, router = new RouterMock(); windowMock = new WindowMock(); + windowMock.location.pathname = '/' + screenName; ephemeralMessages = new EphemeralMessages(); metrics = new Metrics(); relier = new Relier(); @@ -223,11 +225,10 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, }); it('adds an entry into the event stream', function () { - var err = AuthErrors.toError('INVALID_TOKEN', 'bad token, man'); + var err = AuthErrors.toError('INVALID_TOKEN', screenName); view.displayError(err); - assert.isTrue(TestHelpers.isEventLogged(metrics, - metrics.errorToId('INVALID_TOKEN', AuthErrors))); + assert.isTrue(TestHelpers.isErrorLogged(metrics, err)); }); it('displays an `Unexpected Error` if no error passed in', function () { @@ -249,11 +250,11 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, }); it('adds an entry into the event stream', function () { - var err = AuthErrors.toError('INVALID_TOKEN', 'bad token, man'); + var err = AuthErrors.toError('INVALID_TOKEN', screenName); + view.displayError(err); - assert.isTrue(TestHelpers.isEventLogged(metrics, - metrics.errorToId('INVALID_TOKEN', AuthErrors))); + assert.isTrue(TestHelpers.isErrorLogged(metrics, err)); }); it('displays an `Unexpected Error` if no error passed in', function () { @@ -279,7 +280,7 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, describe('navigate', function () { it('navigates to a page', function (done) { router.on('navigate', function (newPage) { - wrapAssertion(function() { + wrapAssertion(function () { assert.equal(newPage, 'signin'); }, done); }); @@ -287,12 +288,12 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, }); it('logs an error if an error is passed in the options', function () { + var err = AuthErrors.toError('SESSION_EXPIRED', screenName); view.navigate('signin', { - error: AuthErrors.toError('SESSION_EXPIRED') + error: err }); - assert.isTrue(TestHelpers.isEventLogged(metrics, - metrics.errorToId('SESSION_EXPIRED', AuthErrors))); + assert.isTrue(TestHelpers.isErrorLogged(metrics, err)); }); }); @@ -332,7 +333,7 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, describe('BaseView.preventDefaultThen', function () { it('can take the name of a function as the name of the event handler', function (done) { view.eventHandler = function (event) { - wrapAssertion(function() { + wrapAssertion(function () { assert.isTrue(event.isDefaultPrevented()); }, done); }; @@ -343,7 +344,7 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, it('can take a function as the event handler', function (done) { function eventHandler(event) { - wrapAssertion(function() { + wrapAssertion(function () { assert.isTrue(event.isDefaultPrevented()); }, done); } @@ -365,7 +366,7 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, describe('BaseView.cancelEventThen', function () { it('can take the name of a function as the name of the event handler', function (done) { view.eventHandler = function (event) { - wrapAssertion(function() { + wrapAssertion(function () { assert.isTrue(event.isDefaultPrevented()); assert.isTrue(event.isPropagationStopped()); }, done); @@ -377,7 +378,7 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, it('can take a function as the event handler', function (done) { function eventHandler(event) { - wrapAssertion(function() { + wrapAssertion(function () { assert.isTrue(event.isDefaultPrevented()); assert.isTrue(event.isPropagationStopped()); }, done); @@ -440,9 +441,10 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, describe('logError', function () { it('logs an error to the event stream', function () { - view.logError(AuthErrors.toError('INVALID_TOKEN')); - assert.isTrue(TestHelpers.isEventLogged(metrics, - metrics.errorToId('INVALID_TOKEN', AuthErrors))); + var err = AuthErrors.toError('INVALID_TOKEN', screenName); + + view.logError(err); + assert.isTrue(TestHelpers.isErrorLogged(metrics, err)); }); it('does not log already logged errors', function () { @@ -460,7 +462,8 @@ function (chai, jQuery, sinon, BaseView, Translator, EphemeralMessages, Metrics, view.metrics.events.clear(); view.logError(); - assert.isTrue(TestHelpers.isEventLogged(metrics, metrics.errorToId('UNEXPECTED_ERROR', AuthErrors))); + var err = AuthErrors.toError('UNEXPECTED_ERROR', screenName); + assert.isTrue(TestHelpers.isErrorLogged(metrics, err)); }); it('prints a stack trace via console.trace to facilitate debugging if no error object is passed in', function () { diff --git a/app/tests/spec/views/form.js b/app/tests/spec/views/form.js index 75a8f8f1f..91aa7eb60 100644 --- a/app/tests/spec/views/form.js +++ b/app/tests/spec/views/form.js @@ -70,10 +70,6 @@ function (chai, $, p, FormView, Template, Constants, Metrics, AuthErrors, TestHe }); } - function testEventLogged(eventName) { - assert.isTrue(TestHelpers.isEventLogged(metrics, eventName)); - } - beforeEach(function () { metrics = new Metrics(); view = new View({ @@ -265,7 +261,7 @@ function (chai, $, p, FormView, Template, Constants, Metrics, AuthErrors, TestHe var err = AuthErrors.toError('EMAIL_REQUIRED'); view.on('validation_error', function () { TestHelpers.wrapAssertion(function () { - testEventLogged(metrics.errorToId(err, AuthErrors)); + assert.isTrue(TestHelpers.isErrorLogged(metrics, err)); }, done); }); view.showValidationError('#focusMe', err);