Skip to content

Commit

Permalink
bug(settings): Backend events for login_reg and login_complete have g…
Browse files Browse the repository at this point in the history
…one missing

Because:
- On august 9th, some backend events seemed to disappear
- This is due to the entry point not being defined on the event

This Commit:
- Fixes missing entrypoint glean data
- Ensures entrypoint is sent in graphql requests if it is present in URL
- Fixes typo in utm param utmContext should have been utmContent.
- Puts the context model in a shared place to reduce the change this happens again.
  • Loading branch information
dschom committed Nov 23, 2024
1 parent 39fc891 commit 6844078
Show file tree
Hide file tree
Showing 9 changed files with 114 additions and 36 deletions.
2 changes: 2 additions & 0 deletions libs/shared/metrics/glean/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* 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/. */

export * from './lib/metrics.context';
38 changes: 38 additions & 0 deletions libs/shared/metrics/glean/src/lib/metrics.context.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/* 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/. */
import { MetricsContext } from './metrics.context';

describe('metrics context', () => {
it('creates', () => {
// Note! The auth-server expects quite a few query params to be propagated during
// graphql calls. These are commonly referred to as the metrics context.
const queryParams = {
flowId: 'test1',
utmCampaign: 'test2',
utmContent: 'test3',
utmMedium: 'test4',
utmSource: 'test5',
utmTerm: 'test6',
flowBeginTime: '100',
foo: 'test7',
};
const result = new MetricsContext(queryParams);
expect(result.flowId).toEqual(queryParams.flowId);
expect(result.utmCampaign).toEqual(queryParams.utmCampaign);
expect(result.utmContent).toEqual(queryParams.utmContent);
expect(result.utmMedium).toEqual(queryParams.utmMedium);
expect(result.utmSource).toEqual(queryParams.utmSource);
expect(result.utmTerm).toEqual(queryParams.utmTerm);
expect(result.flowBeginTime).toEqual(Number(queryParams.flowBeginTime));

// Make sure type is legit
expect((result as any).foo).toBeUndefined();
});

it('prunes empty fields', () => {
const result = new MetricsContext({});
const pruned = result.prune();
expect(pruned).toEqual({});
});
});
46 changes: 46 additions & 0 deletions libs/shared/metrics/glean/src/lib/metrics.context.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/* 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/. */

/**
* Represents common metrics context for auth server glean events. This
* is context that typically exists in query params on web pages, and
* drives important metric dashboards
*/
export class MetricsContext {
deviceId?: string;
entrypoint?: string;
flowId?: string;
flowBeginTime?: Number;
utmCampaign?: string;
utmContent?: string;
utmMedium?: string;
utmSource?: string;
utmTerm?: string;

constructor(queryParams?: Record<string, string>) {
queryParams = queryParams || {};
this.deviceId = queryParams['deviceId'];
this.entrypoint = queryParams['entrypoint'];
this.flowId = queryParams['flowId'];
this.flowBeginTime = queryParams['flowBeginTime']
? Number(queryParams['flowBeginTime'])
: undefined;
this.utmCampaign = queryParams['utmCampaign'];
this.utmContent = queryParams['utmContent'];
this.utmMedium = queryParams['utmMedium'];
this.utmSource = queryParams['utmSource'];
this.utmTerm = queryParams['utmTerm'];
}

/** Returns a partial object with empty fields removed. */
prune(): Partial<MetricsContext> {
const pruned: any = Object.assign({}, this);
Object.keys(pruned).forEach((x) => {
if (pruned[x] === undefined) {
delete pruned[x];
}
});
return pruned;
}
}
18 changes: 1 addition & 17 deletions packages/fxa-auth-client/lib/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { Credentials } from './crypto';
import * as hawk from './hawk';
import { SaltVersion, createSaltV2 } from './salt';
import * as Sentry from '@sentry/browser';
import { MetricsContext } from '@fxa/shared/metrics/glean';

enum ERRORS {
INVALID_TIMESTAMP = 111,
Expand Down Expand Up @@ -118,23 +119,6 @@ export type AuthServerError = Error & {
retryAfterLocalized?: string;
};

export const MetricsContextKeys = [
'deviceId',
'flowId',
'flowBeginTime',
'utmCampaign',
'utmContext',
'utmMedium',
'utmSource',
'utmTerm',
] as const;
export type MetricsContext = Omit<
{
[k in (typeof MetricsContextKeys)[number]]?: string;
},
'flowBeginTime'
> & { flowBeginTime?: number };

export type VerificationMethod =
| 'email'
| 'email-otp'
Expand Down
3 changes: 2 additions & 1 deletion packages/fxa-auth-server/lib/metrics/glean/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { createHash } from 'crypto';
import { AuthRequest } from '../../types';
import * as AppError from '../../error';
import { clientId as clientIdValidator } from '../../oauth/validators';
import { MetricsContext } from '@fxa/shared/metrics/glean';

// According to @types/hapi, request.auth.credentials.user is of type
// UserCredentials, which is just {}. That's not actually the case and it
Expand Down Expand Up @@ -137,7 +138,7 @@ const createEventFn =
return;
}

const metricsContext = await request.app.metricsContext;
const metricsContext: MetricsContext = await request.app.metricsContext;

// metrics sent with every event
const commonMetrics = {
Expand Down
1 change: 1 addition & 0 deletions packages/fxa-settings/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
"^react-native$": "react-native-web",
"^.+\\.module\\.(css|sass|scss)$": "identity-obj-proxy",
"@fxa/shared/l10n": "<rootDir>/../../libs/shared/l10n/src/index.ts",
"@fxa/shared/metrics/glean": "<rootDir>/../../libs/shared/metrics/glean/src/index.ts",
"^@fxa/shared/assets(.*)$": "<rootDir>/../../libs/shared/assets/src$1"
},
"moduleFileExtensions": [
Expand Down
17 changes: 17 additions & 0 deletions packages/fxa-settings/src/lib/metrics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
usePageViewEvent,
logErrorEvent,
initUserPreferences,
queryParamsToMetricsContext,
} from './metrics';

import { window } from './window';
Expand Down Expand Up @@ -460,3 +461,19 @@ describe('logError', () => {
expectPayloadEvents(['error.unknown context.unknown namespace.-1']);
});
});

describe('queryParamsToMetricsContext', () => {
it('creates metrics context from query params', () => {
// Note! The auth-server expects quite a few query params to be propagated during
// graphql calls. These are commonly referred to as the metrics context.
const queryParams = {
flowId: 'test1',
foo: 'test2',
};
const result = queryParamsToMetricsContext(queryParams);
expect(result.flowId).toEqual(queryParams.flowId);

// Make sure type is legit
expect((result as any).foo).toBeUndefined();
});
});
20 changes: 5 additions & 15 deletions packages/fxa-settings/src/lib/metrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { v4 as uuid } from 'uuid';
import { QueryParams } from '..';
import { once } from './utilities';
import { useEffect } from 'react';
import { MetricsContext, MetricsContextKeys } from 'fxa-auth-client/browser';
import { MetricsContext } from '@fxa/shared/metrics/glean';

export const settingsViewName = 'settings';

Expand Down Expand Up @@ -512,21 +512,11 @@ export function addExperiment(choice: string, group: string) {
/**
* Take a record and pick out key-values for a MetricsContext. Note that the
* value passed in could've been asserted to be of QueryParam type with
* specific keys but the value is actually a record of all the URL query params
* specific keys but the value is actually a record of all the URL query params.
*/
export function queryParamsToMetricsContext(
queryParams: Record<string, string>
): MetricsContext {
const metricsContext: MetricsContext = {};
return MetricsContextKeys.reduce((acc, k) => {
if (queryParams[k]) {
// Special case for flowBeginTime, which is a number
if (k === 'flowBeginTime') {
acc[k] = Number(queryParams[k]);
} else {
acc[k] = queryParams[k] as any;
}
}
return acc;
}, metricsContext);
): Partial<MetricsContext> {
const context = new MetricsContext(queryParams);
return context.prune();
}
5 changes: 2 additions & 3 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@
"@fxa/accounts/recovery-phone": [
"libs/accounts/recovery-phone/src/index.ts"
],
"@fxa/accounts/two-factor": [
"libs/accounts/two-factor/src/index.ts"
],
"@fxa/accounts/two-factor": ["libs/accounts/two-factor/src/index.ts"],
"@fxa/payments/capability": ["libs/payments/capability/src/index.ts"],
"@fxa/payments/cart": ["libs/payments/cart/src/index.ts"],
"@fxa/payments/currency": ["libs/payments/currency/src/index.ts"],
Expand Down Expand Up @@ -71,6 +69,7 @@
"@fxa/shared/l10n/server": ["libs/shared/l10n/src/server.ts"],
"@fxa/shared/log": ["libs/shared/log/src/index.ts"],
"@fxa/shared/metrics/statsd": ["libs/shared/metrics/statsd/src/index.ts"],
"@fxa/shared/metrics/glean": ["libs/shared/metrics/glean/src/index.ts"],
"@fxa/shared/mozlog": ["libs/shared/mozlog/src/index.ts"],
"@fxa/shared/notifier": ["libs/shared/notifier/src/index.ts"],
"@fxa/shared/otel": ["libs/shared/otel/src/index.ts"],
Expand Down

0 comments on commit 6844078

Please sign in to comment.