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

byCedric · 2022-06-16T12:23:41Z

Checklist

I've added an entry to CHANGELOG.md if necessary. You can comment this pull request with /changelog-entry [breaking-change|new-feature|bug-fix|chore] [message] and CHANGELOG.md will be updated automatically.

Why

Fixes ENG-5291

See discussion in #1147 (comment)

How

Added EXPO_NO_TELEMETRY for eas metadata
Refactored subscribeTelemetry to withTelemetryAsync
Added more tests for withTelemetryAsync
Added Error container to safely wrap executionId with the thrown error
Updated shared error handler to use the Error container

Test Plan

$ yarn test telemetry

byCedric · 2022-06-16T12:24:31Z

/changelog-entry chore Refactor metadata telemetry with opt-out

byCedric · 2022-06-16T12:26:39Z

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

+ * 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) {


I'm not a big fan of this, since it kind of obfuscates the original error. But I don't see a "safe" way of exposing the executionId to the user outside withTelemetryAsync.

We can always do something like the snippet below, but that's not super safe. And adding an intermediate Error class also feels a bit weird (e.g. MetadataDownloadError < TelemetryError < Error). But I'm open to other suggestions.

try { ... } catch (error: any) { error.executionId = ... throw error; }

You could make withTelemetryAsync return whatever passed callback returns

const errors = await withTelemetryAsync(MetadataEvent.APPLE_METADATA_DOWNLOAD, { app, auth }, async () => { ... return errors; }) if (errors.length > 0) { throw new MetadataDownloadError(errors); }

or handle that error inside withTelemetryAsync (or inside callback that you are passing to withTelemetryAsync)

I'm against returning errors as if they were regular data. I like the other suggestion better.

github-actions · 2022-06-16T12:27:39Z

Size Change: -629 B (0%)

Total Size: 25.1 MB

Filename	Size	Change
`./packages/eas-cli/dist/eas-linux-x64.tar.gz`	25.1 MB	-629 B (0%)

_{compressed-size-action}

…ainer

codecov · 2022-06-16T12:38:26Z

Codecov Report

Merging #1169 (97c361f) into main (db338b2) will increase coverage by 0.09%.
The diff coverage is 26.67%.

@@            Coverage Diff             @@
##             main    #1169      +/-   ##
==========================================
+ Coverage   51.01%   51.10%   +0.09%     
==========================================
  Files         371      371              
  Lines       13219    13221       +2     
  Branches     2543     2694     +151     
==========================================
+ Hits         6743     6755      +12     
+ Misses       6464     5953     -511     
- Partials       12      513     +501

Impacted Files	Coverage Δ
packages/eas-cli/src/metadata/download.ts	`29.73% <6.25%> (+1.53%)`	⬆️
packages/eas-cli/src/metadata/upload.ts	`27.03% <6.25%> (+0.72%)`	⬆️
packages/eas-cli/src/metadata/errors.ts	`25.00% <57.15%> (+6.25%)`	⬆️
packages/eas-cli/src/metadata/utils/telemetry.ts	`77.09% <100.00%> (+18.39%)`	⬆️
packages/eas-cli/src/project/publish.ts	`93.66% <0.00%> (-0.04%)`	⬇️
packages/eas-cli/src/prompts.ts	`43.34% <0.00%> (ø)`
packages/eas-cli/src/vcs/git.ts	`27.28% <0.00%> (ø)`
packages/eas-json/src/reader.ts	`80.44% <0.00%> (ø)`
packages/eas-cli/src/platform.ts	`50.00% <0.00%> (ø)`
packages/eas-cli/src/vcs/local.ts	`79.49% <0.00%> (ø)`
... and 130 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e224f9...97c361f. Read the comment docs.

linear · 2022-06-16T12:58:05Z

ENG-5291 Implement `EXPO_NO_TELEMETRY`

Allow users to disable it

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

dsokal · 2022-06-20T11:20:28Z

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

-      errors.push(error);
+    for (const task of tasks) {
+      try {
+        await task.downloadAsync({ config, context: taskCtx as AppleData });


Can we get rid of the as AppleData cast?

It's the same as: #1169 (comment) We could relax the types, but we would need to add more checks in the tasks.

dsokal · 2022-06-20T11:22:36Z

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

+ * 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) {


I'm against returning errors as if they were regular data. I like the other suggestion better.

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

dsokal · 2022-06-20T11:24:31Z

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

+  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);
+    }
+  });


This looks almost like the other function in this PR (downloadMetadataAsync). Maybe it's worth writing some util to extract the common code?

That's right, they are both the "task runners". The main difference is the task it's executing and the error it could be throwing. I can create a task runner, but I didn't want to abstract too much in the first version.

Let's create a follow-up task to refactor this.

dsokal · 2022-06-20T11:26:18Z

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

+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,
+};


You could use ts-mockito instead of using as any. We use it in www.

Oh nice! TIL, I used the any to only pass a partial version of the original type (only the types that we need in the tests). I'll look into ts-mockito

dsokal · 2022-06-20T11:27:42Z

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

+    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));


await expect(...).rejects.toThrow(...)

dsokal · 2022-06-20T11:28:00Z

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

+    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));


await expect(...).rejects.toThrow(...)

I had this at first, but in we also need to test the "executionId". I don't think we can do such a thing with just that line, right?

I see your point but you're just checking if executionId and originalError were set to anything. If we're using typescript for all files in this repo then it's not possible to create an instance of MetadataTelemetryError without specifying those two fields.

I'm not feeling too strong about this though.

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

dsokal · 2023-04-25T09:31:37Z

@byCedric is this PR something we still want to merge? If no can we close it?

wkozyra95 · 2023-06-22T11:02:45Z

Inactive for a long time, plus a lot of changes in the underlying code, so I'm closing it.

byCedric requested a review from wkozyra95 June 16, 2022 12:23

byCedric requested a review from dsokal as a code owner June 16, 2022 12:24

byCedric commented Jun 16, 2022

View reviewed changes

byCedric added 3 commits June 16, 2022 14:32

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

6aa3a78

[eas-cli] Implement new wrapper and telemetry execution id error cont…

50f38a0

…ainer

update CHANGELOG.md

530649f

byCedric force-pushed the @bycedric/metadata/refactor-telemetry branch from 7c8fc45 to 530649f Compare June 16, 2022 12:33

[eas-cli] Remove EXPO_NO_TELEMETRY in favor of eas analytics

9cb17fa

dsokal reviewed Jun 20, 2022

View reviewed changes

[eas-cli] Add partial apple data types to task context

97c361f

byCedric marked this pull request as draft July 18, 2022 11:51

wkozyra95 closed this Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

byCedric commented Jun 16, 2022 •

edited

Loading

byCedric commented Jun 16, 2022

byCedric Jun 16, 2022 •

edited

Loading

wkozyra95 Jun 20, 2022

dsokal Jun 20, 2022

github-actions bot commented Jun 16, 2022 •

edited

Loading

codecov bot commented Jun 16, 2022 •

edited

Loading

linear bot commented Jun 16, 2022

dsokal Jun 20, 2022

byCedric Jun 20, 2022

dsokal Jun 20, 2022

dsokal Jun 20, 2022

byCedric Jun 20, 2022

dsokal Jun 20, 2022

dsokal Jun 20, 2022

byCedric Jun 20, 2022

dsokal Jun 20, 2022

dsokal Jun 20, 2022

byCedric Jun 20, 2022

dsokal Jun 20, 2022

dsokal commented Apr 25, 2023

wkozyra95 commented Jun 22, 2023

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

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

Conversation

byCedric commented Jun 16, 2022 • edited Loading

Checklist

Why

How

Test Plan

byCedric commented Jun 16, 2022

byCedric Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 16, 2022 • edited Loading

codecov bot commented Jun 16, 2022 • edited Loading

Codecov Report

linear bot commented Jun 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsokal commented Apr 25, 2023

wkozyra95 commented Jun 22, 2023

byCedric commented Jun 16, 2022 •

edited

Loading

byCedric Jun 16, 2022 •

edited

Loading

github-actions bot commented Jun 16, 2022 •

edited

Loading

codecov bot commented Jun 16, 2022 •

edited

Loading