From dba18b1f70e36336bc8c16aa452ebff6043b048e Mon Sep 17 00:00:00 2001 From: dschom Date: Wed, 20 Sep 2023 16:23:55 -0700 Subject: [PATCH] bug(shared): Amplitude crashes on startup with unhandled error Because: - Content server was restarting due to unknown event group This Commit: - Wraps the mapping function with try catch to ensure it cannot result in an unhandled error - Adds optional statsd and logger params so that we can track potential problems outside of sentry. - Updates initializations to provide statsd and logger params to amplitude's initialization routine. --- .../fxa-auth-server/lib/metrics/amplitude.js | 6 +- .../server/lib/amplitude.js | 2 +- .../server/lib/amplitude.js | 4 +- packages/fxa-shared/metrics/amplitude.ts | 169 ++++++++++-------- packages/fxa-shared/test/metrics/amplitude.js | 2 +- 5 files changed, 102 insertions(+), 81 deletions(-) diff --git a/packages/fxa-auth-server/lib/metrics/amplitude.js b/packages/fxa-auth-server/lib/metrics/amplitude.js index d7101febe5f..a9573894802 100644 --- a/packages/fxa-auth-server/lib/metrics/amplitude.js +++ b/packages/fxa-auth-server/lib/metrics/amplitude.js @@ -14,6 +14,8 @@ const { Container } = require('typedi'); const { StatsD } = require('hot-shots'); +const config = require('../../config').default.getProperties(); +const logger = require('../log')(config.log.level, 'amplitude'); const { GROUPS, initialize } = require('fxa-shared/metrics/amplitude').amplitude; @@ -183,7 +185,9 @@ module.exports = (log, config) => { const transformEvent = initialize( config.oauth.clientIds, EVENTS, - FUZZY_EVENTS + FUZZY_EVENTS, + logger, + Container.has(StatsD) ? Container.get(StatsD) : undefined ); return receiveEvent; diff --git a/packages/fxa-content-server/server/lib/amplitude.js b/packages/fxa-content-server/server/lib/amplitude.js index 933f716c144..4648ed5b807 100644 --- a/packages/fxa-content-server/server/lib/amplitude.js +++ b/packages/fxa-content-server/server/lib/amplitude.js @@ -916,7 +916,7 @@ const FUZZY_EVENTS = new Map([ ], ]); -const transform = initialize(SERVICES, EVENTS, FUZZY_EVENTS); +const transform = initialize(SERVICES, EVENTS, FUZZY_EVENTS, logger, statsd); module.exports = receiveEvent; diff --git a/packages/fxa-payments-server/server/lib/amplitude.js b/packages/fxa-payments-server/server/lib/amplitude.js index d601910c568..c2f1a4ce7c9 100644 --- a/packages/fxa-payments-server/server/lib/amplitude.js +++ b/packages/fxa-payments-server/server/lib/amplitude.js @@ -47,7 +47,9 @@ const FUZZY_EVENTS = new Map([ const transform = initialize( config.get('oauth_client_id_map'), {}, - FUZZY_EVENTS + FUZZY_EVENTS, + log, + Container.has(StatsD) ? Container.get(StatsD) : undefined ); // TODO: remove eslint ignore in FXA-6950 diff --git a/packages/fxa-shared/metrics/amplitude.ts b/packages/fxa-shared/metrics/amplitude.ts index 058505bbdbb..9b08873a37c 100644 --- a/packages/fxa-shared/metrics/amplitude.ts +++ b/packages/fxa-shared/metrics/amplitude.ts @@ -6,6 +6,8 @@ import Ajv from 'ajv'; import pick from 'lodash.pick'; import { ParsedUserAgentProperties, ParsedUa, ParsedOs } from './user-agent'; import { Location } from '../connected-services/models/Location'; +import { ILogger } from '../log'; +import { StatsD } from 'hot-shots'; type AmplitudeEventGroup = typeof GROUPS; type AmplitudeEventGroupKey = keyof AmplitudeEventGroup; @@ -306,6 +308,8 @@ export const amplitude = { * but here `group` can be a string or a function. If it's * a function, it will be passed the matched `eventCategory` * as its argument and should return the group string. + * @param {StatsD} statsd An optional statsd client. + * @param {} * * @returns {Function} The mapper function. */ @@ -324,7 +328,9 @@ export const amplitude = { group: AmplitudeEventGroupKey | AmplitudeEventFuzzyEventGroupMapFn; event: string | AmplitudeEventFuzzyEventNameMapFn; } - > + >, + log?: ILogger, + statsd?: StatsD ) { /** * Map from a source event and it's associated data to an amplitude event. @@ -341,93 +347,98 @@ export const amplitude = { * ease by perusing the code. */ return (event: { [key: string]: any }, data: EventData) => { - if (!event || !data) { - return; - } - - let eventType = event.type; - let mapping = events[eventType]; - let eventCategory, eventTarget; - - if (!mapping) { - for (const [key, value] of fuzzyEvents.entries()) { - const match = key.exec(eventType); - if (match) { - mapping = value; + try { + if (!event || !data) { + return; + } - if (match.length >= 2) { - eventCategory = match[1]; - if (match.length === 3) { - eventTarget = match[2]; + let eventType = event.type; + let mapping = events[eventType]; + let eventCategory, eventTarget; + + if (!mapping) { + for (const [key, value] of fuzzyEvents.entries()) { + const match = key.exec(eventType); + if (match) { + mapping = value; + + if (match.length >= 2) { + eventCategory = match[1]; + if (match.length === 3) { + eventTarget = match[2]; + } } - } - break; + break; + } } } - } - if (mapping) { - eventType = mapping.event; - if (typeof eventType === 'function') { - eventType = eventType(eventCategory, eventTarget); - if (!eventType) { - return; + if (mapping) { + eventType = mapping.event; + if (typeof eventType === 'function') { + eventType = eventType(eventCategory, eventTarget); + if (!eventType) { + return; + } } - } - let eventGroup = mapping.group; - if (typeof eventGroup === 'function') { - eventGroup = eventGroup(eventCategory); - if (!eventGroup) { - return; + let eventGroup = mapping.group; + if (typeof eventGroup === 'function') { + eventGroup = eventGroup(eventCategory); + if (!eventGroup) { + return; + } } - } - let version; - try { - // @ts-ignore - version = /([0-9]+)\.([0-9]+)$/.exec(data.version)[0]; - } catch (err) {} - - // minimal data should be enabled for routes used by internal - // services like profile-server and token-server - if (mapping.minimal) { - data = { - uid: data.uid, - service: data.service, - version: data.version, - }; - } + let version; + try { + // @ts-ignore + version = /([0-9]+)\.([0-9]+)$/.exec(data.version)[0]; + } catch (err) {} + + // minimal data should be enabled for routes used by internal + // services like profile-server and token-server + if (mapping.minimal) { + data = { + uid: data.uid, + service: data.service, + version: data.version, + }; + } - return pruneUnsetValues({ - op: 'amplitudeEvent', - event_type: `${eventGroup} - ${eventType}`, - time: event.time, - user_id: data.uid, - device_id: data.deviceId, - session_id: data.flowBeginTime, - app_version: version, - language: data.lang, - country_code: data.countryCode, - country: data.country, - region: data.region, - os_name: data.os, - os_version: data.osVersion, - device_model: data.formFactor, - event_properties: mapEventProperties( - eventType, - eventGroup as string, - eventCategory as string, - eventTarget as string, - data - ), - user_properties: mapUserProperties( - eventGroup as string, - eventCategory as string, - data - ), - }); + return pruneUnsetValues({ + op: 'amplitudeEvent', + event_type: `${eventGroup} - ${eventType}`, + time: event.time, + user_id: data.uid, + device_id: data.deviceId, + session_id: data.flowBeginTime, + app_version: version, + language: data.lang, + country_code: data.countryCode, + country: data.country, + region: data.region, + os_name: data.os, + os_version: data.osVersion, + device_model: data.formFactor, + event_properties: mapEventProperties( + eventType, + eventGroup as string, + eventCategory as string, + eventTarget as string, + data + ), + user_properties: mapUserProperties( + eventGroup as string, + eventCategory as string, + data + ), + }); + } + } catch (err) { + statsd?.increment('fxa.amplitude.transform.error'); + log?.error(err); } return; @@ -442,6 +453,10 @@ export const amplitude = { ) { const { serviceName, clientId } = getServiceNameAndClientId(data); + if (typeof EVENT_PROPERTIES[eventGroup] !== 'function') { + throw new Error(`Unknown event group: ${eventGroup}`); + } + return Object.assign( pruneUnsetValues({ service: serviceName, diff --git a/packages/fxa-shared/test/metrics/amplitude.js b/packages/fxa-shared/test/metrics/amplitude.js index a1b8b4d032c..a48d74a1612 100644 --- a/packages/fxa-shared/test/metrics/amplitude.js +++ b/packages/fxa-shared/test/metrics/amplitude.js @@ -40,7 +40,7 @@ describe('metrics/amplitude:', () => { it('exports an initialize method', () => { assert.isFunction(amplitude.initialize); - assert.lengthOf(amplitude.initialize, 3); + assert.lengthOf(amplitude.initialize, 5); }); describe('initialize:', () => {