[eas-cli] Use wrapper instead of telemetry subscriber for metadata

CHANGELOG.md

@@ -12,6 +12,8 @@ This is the log of notable changes to EAS CLI and related packages.
 
 ### 🧹 Chores
 
+- Refactor metadata telemetry to handle errors better. ([#1169](https://github.com/expo/eas-cli/pull/1169) by [@byCedric](https://github.com/byCedric))
+
 ## [0.54.1](https://github.com/expo/eas-cli/releases/tag/v0.54.1) - 2022-06-15
 
 ### 🎉 New features

packages/eas-cli/src/metadata/download.ts

@@ -9,7 +9,7 @@ import { createAppleTasks } from './apple/tasks';
 import { createAppleWriter } from './config';
 import { MetadataContext, ensureMetadataAppStoreAuthenticatedAsync } from './context';
 import { MetadataDownloadError, MetadataValidationError } from './errors';
-import { subscribeTelemetry } from './utils/telemetry';
+import { withTelemetryAsync } from './utils/telemetry';
 
 /**
  * Generate a local store configuration from the stores.
@@ -29,44 +29,38 @@ export async function downloadMetadataAsync(metadataCtx: MetadataContext): Promi
   }
 
   const { app, auth } = await ensureMetadataAppStoreAuthenticatedAsync(metadataCtx);
-  const { unsubscribeTelemetry, executionId } = subscribeTelemetry(
-    MetadataEvent.APPLE_METADATA_DOWNLOAD,
-    { app, auth }
-  );
 
   Log.addNewLineIfNone();
   Log.log('Downloading App Store configuration...');
 
-  const errors: Error[] = [];
-  const config = createAppleWriter();
-  const tasks = createAppleTasks(metadataCtx);
-  const taskCtx = { app };
+  await withTelemetryAsync(MetadataEvent.APPLE_METADATA_DOWNLOAD, { app, auth }, async () => {
+    const errors: Error[] = [];
+    const config = createAppleWriter();
+    const tasks = createAppleTasks(metadataCtx);
+    const taskCtx = { app };
 
-  for (const task of tasks) {
-    try {
-      await task.prepareAsync({ context: taskCtx });
-    } catch (error: any) {
-      errors.push(error);
+    for (const task of tasks) {
+      try {
+        await task.prepareAsync({ context: taskCtx });
+      } catch (error: any) {
+        errors.push(error);
+      }
     }
-  }
 
-  for (const task of tasks) {
-    try {
-      await task.downloadAsync({ config, context: taskCtx as AppleData });
-    } catch (error: any) {
-      errors.push(error);
+    for (const task of tasks) {
+      try {
+        await task.downloadAsync({ config, context: taskCtx as AppleData });
+      } catch (error: any) {
+        errors.push(error);
+      }
     }
-  }
 
-  try {
     await fs.writeJson(filePath, config.toSchema(), { spaces: 2 });
-  } finally {
-    unsubscribeTelemetry();
-  }
 
-  if (errors.length > 0) {
-    throw new MetadataDownloadError(errors, executionId);
-  }
+    if (errors.length > 0) {
+      throw new MetadataDownloadError(errors);
+    }
+  });
 
   return filePath;
 }

packages/eas-cli/src/metadata/errors.ts

@@ -14,14 +14,25 @@ export class MetadataValidationError extends Error {
   }
 }
 
+/**
+ * During telemetry actions, a random exection ID is generated.
+ * This ID should be communicated to users and reported to us.
+ * With that ID, we can look up the failed requests and help solve the issue.
+ */
+export class MetadataTelemetryError extends Error {
+  public constructor(public readonly originalError: Error, public readonly executionId: string) {
+    super(originalError.message);
+  }
+}
+
 /**
  * If a single entity failed to update, we don't block the other entities from uploading.
  * We still attempt to update the data in the stores as much as possible.
  * Because of that, we keep track of any errors encountered and throw this generic error.
  * It contains that list of encountered errors to present to the user.
  */
 export class MetadataUploadError extends Error {
-  public constructor(public readonly errors: Error[], public readonly executionId: string) {
+  public constructor(public readonly errors: Error[]) {
     super(
       `Store configuration upload encountered ${
         errors.length === 1 ? 'an error' : `${errors.length} errors`
@@ -32,12 +43,12 @@ export class MetadataUploadError extends Error {
 
 /**
  * If a single entity failed to download, we don't block the other entities from downloading.
- * We sill attempt to pull in the data from the stores as much as possible.
- * Because of that, we keep track of any errors envountered and throw this generic error.
+ * We still attempt to pull in the data from the stores as much as possible.
+ * Because of that, we keep track of any errors encountered and throw this generic error.
  * It contains that list of encountered errors to present to the user.
  */
 export class MetadataDownloadError extends Error {
-  public constructor(public readonly errors: Error[], public readonly executionId: string) {
+  public constructor(public readonly errors: Error[]) {
     super(
       `Store configuration download encountered ${
         errors.length === 1 ? 'an error' : `${errors.length} errors`
@@ -50,7 +61,10 @@ export class MetadataDownloadError extends Error {
  * Handle a thrown metadata error by informing the user what went wrong.
  * If a normal error is thrown, this method will re-throw that error to avoid consuming it.
  */
-export function handleMetadataError(error: Error): void {
+export function handleMetadataError(thrownError: Error): void {
+  const error =
+    thrownError instanceof MetadataTelemetryError ? thrownError.originalError : thrownError;
+
   if (error instanceof MetadataValidationError) {
     Log.newLine();
     Log.error(chalk.bold(error.message));

packages/eas-cli/src/metadata/upload.ts

@@ -8,7 +8,7 @@ import { createAppleTasks } from './apple/tasks';
 import { createAppleReader, validateConfig } from './config';
 import { MetadataContext, ensureMetadataAppStoreAuthenticatedAsync } from './context';
 import { MetadataUploadError, MetadataValidationError } from './errors';
-import { subscribeTelemetry } from './utils/telemetry';
+import { withTelemetryAsync } from './utils/telemetry';
 
 /**
  * Sync a local store configuration with the stores.
@@ -23,11 +23,6 @@ export async function uploadMetadataAsync(
   }
 
   const { app, auth } = await ensureMetadataAppStoreAuthenticatedAsync(metadataCtx);
-  const { unsubscribeTelemetry, executionId } = subscribeTelemetry(
-    MetadataEvent.APPLE_METADATA_UPLOAD,
-    { app, auth }
-  );
-
   const fileData = await fs.readJson(filePath);
   const { valid, errors: validationErrors } = validateConfig(fileData);
   if (!valid) {
@@ -37,32 +32,32 @@ export async function uploadMetadataAsync(
   Log.addNewLineIfNone();
   Log.log('Uploading App Store configuration...');
 
-  const errors: Error[] = [];
-  const config = createAppleReader(fileData);
-  const tasks = createAppleTasks(metadataCtx);
-  const taskCtx = { app };
-
-  for (const task of tasks) {
-    try {
-      await task.prepareAsync({ context: taskCtx });
-    } catch (error: any) {
-      errors.push(error);
+  await withTelemetryAsync(MetadataEvent.APPLE_METADATA_UPLOAD, { app, auth }, async () => {
+    const errors: Error[] = [];
+    const config = createAppleReader(fileData);
+    const tasks = createAppleTasks(metadataCtx);
+    const taskCtx = { app };
+
+    for (const task of tasks) {
+      try {
+        await task.prepareAsync({ context: taskCtx });
+      } catch (error: any) {
+        errors.push(error);
+      }
     }
-  }
 
-  for (const task of tasks) {
-    try {
-      await task.uploadAsync({ config, context: taskCtx as AppleData });
-    } catch (error: any) {
-      errors.push(error);
+    for (const task of tasks) {
+      try {
+        await task.uploadAsync({ config, context: taskCtx as AppleData });
+      } catch (error: any) {
+        errors.push(error);
+      }
     }
-  }
 
-  unsubscribeTelemetry();
-
-  if (errors.length > 0) {
-    throw new MetadataUploadError(errors, executionId);
-  }
+    if (errors.length > 0) {
+      throw new MetadataUploadError(errors);
+    }
+  });
 
   return { appleLink: `https://appstoreconnect.apple.com/apps/${app.id}/appstore` };
 }

packages/eas-cli/src/metadata/utils/__tests__/telemetry.test.ts

@@ -1,23 +1,73 @@
-import { App } from '@expo/apple-utils';
+import { App, getRequestClient } from '@expo/apple-utils';
 
-import { TelemetryContext, makeDataScrubber } from '../telemetry';
+import { MetadataEvent } from '../../../analytics/events';
+import { MetadataTelemetryError } from '../../errors';
+import { TelemetryContext, makeDataScrubber, withTelemetryAsync } from '../telemetry';
 
-describe(makeDataScrubber, () => {
-  const stub: TelemetryContext = {
-    // Only the App ID is considered to be sensitive
-    app: new App({} as any, 'SECRET_APP_ID', {} as any),
-    // Only the token properties and user credentials are considered to be sensitive
-    auth: {
-      username: 'SECRET_USERNAME',
-      password: 'SECRET_PASSWORD',
-      context: {
-        token: 'SECRET_TOKEN',
-        teamId: 'SECRET_TEAM_ID',
-        providerId: 1337,
-      },
-    } as any,
-  };
+const stub: TelemetryContext = {
+  // Only the App ID is considered to be sensitive
+  app: new App({} as any, 'SECRET_APP_ID', {} as any),
+  // Only the token properties and user credentials are considered to be sensitive
+  auth: {
+    username: 'SECRET_USERNAME',
+    password: 'SECRET_PASSWORD',
+    context: {
+      token: 'SECRET_TOKEN',
+      teamId: 'SECRET_TEAM_ID',
+      providerId: 1337,
+    },
+  } as any,
+};
+
+describe(withTelemetryAsync, () => {
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('detaches interceptors after action resolved', async () => {
+    const { interceptors } = getRequestClient();
+    const ejectInterceptor = jest.spyOn(interceptors.response, 'eject');
+
+    await withTelemetryAsync(MetadataEvent.APPLE_METADATA_DOWNLOAD, stub, async () => {
+      return 'testing resolved telemetry action';
+    });
+
+    expect(ejectInterceptor).toBeCalledWith(expect.any(Number));
+  });
+
+  it('detaches interceptors after action rejected', async () => {
+    const { interceptors } = getRequestClient();
+    const ejectInterceptor = jest.spyOn(interceptors.response, 'eject');
 
+    try {
+      await withTelemetryAsync(MetadataEvent.APPLE_METADATA_DOWNLOAD, stub, async () => {
+        throw new Error('testing rejected telemetry action');
+      });
+    } catch {
+      // intentional failure
+    }
+
+    expect(ejectInterceptor).toBeCalledWith(expect.any(Number));
+  });
+
+  it('adds executionId to thrown errors', async () => {
+    let caughtError: null | Error = null;
+
+    try {
+      await withTelemetryAsync(MetadataEvent.APPLE_METADATA_DOWNLOAD, stub, async () => {
+        throw new Error('testing exectution ID');
+      });
+    } catch (error: any) {
+      caughtError = error;
+    }
+
+    expect(caughtError).toBeInstanceOf(MetadataTelemetryError);
+    expect(caughtError).toHaveProperty('executionId', expect.any(String));
+    expect(caughtError).toHaveProperty('originalError', expect.any(Error));
+  });
+});
+
+describe(makeDataScrubber, () => {
   it('scrubs the app.id', () => {
     expect(makeDataScrubber(stub)('some text SECRET_APP_ID')).toBe('some text {APPLE_APP_ID}');
   });

packages/eas-cli/src/metadata/utils/telemetry.ts

@@ -3,26 +3,18 @@ import type { AxiosError } from 'axios';
 import { v4 as uuidv4 } from 'uuid';
 
 import { Analytics, MetadataEvent } from '../../analytics/events';
+import { MetadataTelemetryError } from '../errors';
 
 export type TelemetryContext = {
   app: App;
   auth: Partial<Session.AuthState>;
 };
 
-/**
- * Subscribe the telemetry to the ongoing metadata requests and responses.
- * When providing the app and auth info, we can scrub that data from the telemetry.
- * Returns an execution ID to group all events of a single run together, and a unsubscribe function.
- */
-export function subscribeTelemetry(
+export async function withTelemetryAsync<T>(
   event: MetadataEvent,
-  options: TelemetryContext
-): {
-  /** Unsubscribe the telemetry from all apple-utils events */
-  unsubscribeTelemetry: () => void;
-  /** The unique id added to all telemetry events from a single execution */
-  executionId: string;
-} {
+  options: TelemetryContext,
+  action: () => Promise<T>
+): Promise<T> {
   const executionId = uuidv4();
   const scrubber = makeDataScrubber(options);
   const { interceptors } = getRequestClient();
@@ -59,11 +51,13 @@ export function subscribeTelemetry(
     }
   );
 
-  function unsubscribeTelemetry(): void {
+  try {
+    return await action();
+  } catch (error: any) {
+    throw new MetadataTelemetryError(error, executionId);
+  } finally {
     interceptors.response.eject(responseInterceptorId);
   }
-
-  return { unsubscribeTelemetry, executionId };
 }
 
 /** Exposed for testing */