feat(metrics): Add missing metrics for recovery phone

Author: vbudhram

libs/accounts/recovery-phone/src/lib/sms.manager.spec.ts

@@ -17,9 +17,15 @@ describe('SmsManager', () => {
     messages: {
       create: jest.fn(),
     },
+    lookups: {
+      v2: {
+        phoneNumbers: jest.fn(),
+      },
+    },
   };
   const mockMetrics = {
     increment: jest.fn(),
+    histogram: jest.fn(),
   };
   const mockLog = {
     log: jest.fn(),
@@ -29,6 +35,7 @@ describe('SmsManager', () => {
     validNumberPrefixes: ['+1'],
     maxMessageLength: 160,
     maxRetries: 2,
+    extraLookupFields: ['sms_pumping_risk'],
   };
 
   let manager: SmsManager;
@@ -187,4 +194,61 @@ describe('SmsManager', () => {
     expect(number2).toBeDefined();
     expect(number1).not.toEqual(number2);
   });
+
+  describe('phoneNumberLookup', () => {
+    it('should increment metrics and return result on success', async () => {
+      const phoneNumber = '+15005551111';
+      const mockResult = {
+        simSwap: { lastSimSwap: { swappedPeriod: '1' } },
+        smsPumpingRisk: { smsPumpingRiskScore: 0.5 },
+        phoneNumberQualityScore: 0.8,
+        toJSON: jest.fn().mockReturnValue({ phoneNumber }),
+      };
+
+      mockTwilioSmsClient.lookups.v2.phoneNumbers.mockReturnValue({
+        fetch: jest.fn().mockResolvedValue(mockResult),
+      });
+
+      const result = await manager.phoneNumberLookup(phoneNumber);
+
+      expect(mockMetrics.increment).toHaveBeenCalledWith(
+        'sms.phoneNumberLookup.start'
+      );
+      expect(mockMetrics.increment).toHaveBeenCalledWith(
+        'sms.phoneNumberLookup.success'
+      );
+      expect(mockMetrics.increment).toHaveBeenCalledWith(
+        'sms.phoneNumberLookup.success.simSwap',
+        { period: '1' }
+      );
+      expect(mockMetrics.histogram).toHaveBeenCalledWith(
+        'sms.phoneNumberLookup.success.smsPumpingRisk',
+        0.5
+      );
+      expect(mockMetrics.histogram).toHaveBeenCalledWith(
+        'sms.phoneNumberLookup.success.phoneNumberQualityScore',
+        0.8
+      );
+      expect(result).toEqual({ phoneNumber });
+    });
+
+    it('should increment failed metric and throw error on failure', async () => {
+      const phoneNumber = '+15005551111';
+      const mockError = new Error('Lookup failed');
+
+      mockTwilioSmsClient.lookups.v2.phoneNumbers.mockReturnValue({
+        fetch: jest.fn().mockRejectedValue(mockError),
+      });
+
+      await expect(manager.phoneNumberLookup(phoneNumber)).rejects.toThrow(
+        mockError
+      );
+      expect(mockMetrics.increment).toHaveBeenCalledWith(
+        'sms.phoneNumberLookup.start'
+      );
+      expect(mockMetrics.increment).toHaveBeenCalledWith(
+        'sms.phoneNumberLookup.failure'
+      );
+    });
+  });
 });

libs/accounts/recovery-phone/src/lib/sms.manager.ts

@@ -29,12 +29,47 @@ export class SmsManager {
   ) {}
 
   public async phoneNumberLookup(phoneNumber: string) {
-    const result = await this.client.lookups.v2
-      .phoneNumbers(phoneNumber)
-      .fetch({ fields: this.config.extraLookupFields.join(',') });
-    // Calling toJSON converts PhoneNumberInstance into a
-    // object that just holds state and can be serialized.
-    return result.toJSON();
+    try {
+      this.metrics.increment('sms.phoneNumberLookup.start');
+      const result = await this.client.lookups.v2
+        .phoneNumbers(phoneNumber)
+        .fetch({ fields: this.config.extraLookupFields.join(',') });
+
+      this.metrics.increment('sms.phoneNumberLookup.success');
+
+      /**
+       * See https://www.twilio.com/docs/lookup/v2-api/sim-swap for more details
+       * on the sim swap object returned by Twilio. We currently track only
+       * the last sim swap period.
+       */
+      const simSwapPeriod = result.simSwap?.lastSimSwap?.swappedPeriod;
+      if (simSwapPeriod) {
+        this.metrics.increment('sms.phoneNumberLookup.success.simSwap', {
+          period: simSwapPeriod,
+        });
+      }
+      const smsPumpingRisk = result?.smsPumpingRisk?.smsPumpingRiskScore;
+      if (smsPumpingRisk) {
+        this.metrics.histogram(
+          'sms.phoneNumberLookup.success.smsPumpingRisk',
+          smsPumpingRisk
+        );
+      }
+      const phoneNumberQualityScore = result.phoneNumberQualityScore;
+      if (phoneNumberQualityScore) {
+        this.metrics.histogram(
+          'sms.phoneNumberLookup.success.phoneNumberQualityScore',
+          phoneNumberQualityScore
+        );
+      }
+
+      // Calling toJSON converts PhoneNumberInstance into a
+      // object that just holds state and can be serialized.
+      return result.toJSON();
+    } catch (err) {
+      this.metrics.increment('sms.phoneNumberLookup.failure');
+      throw err;
+    }
   }
 
   public async sendSMS({

packages/fxa-auth-server/lib/routes/index.js

@@ -141,7 +141,8 @@ module.exports = function (
     db,
     glean,
     log,
-    mailer
+    mailer,
+    statsd
   );
   const securityEvents = require('./security-events')(log, db, config);
   const session = require('./session')(

packages/fxa-auth-server/lib/routes/recovery-phone.ts

@@ -52,7 +52,8 @@ class RecoveryPhoneHandler {
     private readonly db: any,
     private readonly glean: GleanMetricsType,
     private readonly log: any,
-    private readonly mailer: any
+    private readonly mailer: any,
+    private readonly statsd: any
   ) {
     this.recoveryPhoneService = Container.get(RecoveryPhoneService);
     this.accountManager = Container.get(AccountManager);
@@ -138,7 +139,7 @@ class RecoveryPhoneHandler {
     }
 
     if (success) {
-      this.log.info('account.recoveryPhone.signinSendCode.success', { uid });
+      this.statsd.increment('account.recoveryPhone.signinSendCode.success');
       await this.glean.twoStepAuthPhoneCode.sent(request);
 
       this.accountEventsManager.recordSecurityEvent(this.db, {
@@ -185,9 +186,7 @@ class RecoveryPhoneHandler {
         getFormattedMessage
       );
       if (success) {
-        this.log.info('account.recoveryPhone.setupPhoneNumber.success', {
-          uid,
-        });
+        this.statsd.increment('account.recoveryPhone.setupPhoneNumber.success');
         await this.glean.twoStepAuthPhoneCode.sent(request);
 
         let nationalFormat: string | null = null;
@@ -299,7 +298,7 @@ class RecoveryPhoneHandler {
       const { acceptLanguage, geo, ua } = request.app;
 
       if (isSetup) {
-        this.log.info('account.recoveryPhone.phoneAdded.success', { uid });
+        this.statsd.increment('account.recoveryPhone.phoneAdded.success');
 
         try {
           const { phoneNumber, nationalFormat } =
@@ -344,7 +343,7 @@ class RecoveryPhoneHandler {
           });
         }
       } else {
-        this.log.info('account.recoveryPhone.phoneSignin.success', { uid });
+        this.statsd.increment('account.recoveryPhone.phoneSignin.success');
 
         this.accountEventsManager.recordSecurityEvent(this.db, {
           name: 'account.recovery_phone_signin_complete',
@@ -422,7 +421,7 @@ class RecoveryPhoneHandler {
     }
 
     if (success) {
-      this.log.info('account.recoveryPhone.phoneRemoved.success', { uid });
+      this.statsd.increment('account.recoveryPhone.phoneRemoved.success');
       await this.glean.twoStepAuthPhoneRemove.success(request);
 
       const account = await this.db.account(uid);
@@ -540,14 +539,16 @@ export const recoveryPhoneRoutes = (
   db: any,
   glean: GleanMetricsType,
   log: any,
-  mailer: any
+  mailer: any,
+  statsd: any
 ) => {
   const recoveryPhoneHandler = new RecoveryPhoneHandler(
     customs,
     db,
     glean,
     log,
-    mailer
+    mailer,
+    statsd
   );
   const routes = [
     {

packages/fxa-auth-server/test/local/routes/recovery-phone.js

@@ -38,6 +38,10 @@ describe('/recovery_phone', () => {
     check: sandbox.fake(),
     checkAuthenticated: sandbox.fake(),
   };
+  const mockStatsd = {
+    increment: sandbox.fake(),
+    histogram: sandbox.fake(),
+  };
   const mockGlean = {
     twoStepAuthPhoneCode: {
       sent: sandbox.fake(),
@@ -75,7 +79,8 @@ describe('/recovery_phone', () => {
       mockDb,
       mockGlean,
       mockLog,
-      mockMailer
+      mockMailer,
+      mockStatsd
     );
   });
 
@@ -124,6 +129,10 @@ describe('/recovery_phone', () => {
           tokenId: undefined,
         }
       );
+      assert.calledOnceWithExactly(
+        mockStatsd.increment,
+        'account.recoveryPhone.signinSendCode.success'
+      );
     });
 
     it('handles failure to send recovery phone code', async () => {
@@ -211,6 +220,10 @@ describe('/recovery_phone', () => {
         mockCustoms.checkAuthenticated.getCall(0).args[2],
         'recoveryPhoneCreate'
       );
+      assert.calledOnceWithExactly(
+        mockStatsd.increment,
+        'account.recoveryPhone.setupPhoneNumber.success'
+      );
     });
 
     it('indicates failure sending sms', async () => {
@@ -363,6 +376,11 @@ describe('/recovery_phone', () => {
           tokenId: undefined,
         }
       );
+
+      assert.calledOnceWithExactly(
+        mockStatsd.increment,
+        'account.recoveryPhone.phoneAdded.success'
+      );
     });
 
     it('indicates a failure confirming code', async () => {
@@ -439,6 +457,10 @@ describe('/recovery_phone', () => {
           tokenId: undefined,
         }
       );
+      assert.calledOnceWithExactly(
+        mockStatsd.increment,
+        'account.recoveryPhone.phoneSignin.success'
+      );
     });
 
     it('fails confirms a code during signin', async () => {
@@ -496,6 +518,10 @@ describe('/recovery_phone', () => {
           tokenId: undefined,
         }
       );
+      assert.calledOnceWithExactly(
+        mockStatsd.increment,
+        'account.recoveryPhone.phoneRemoved.success'
+      );
     });
 
     it('indicates service failure while removing phone', async () => {