ci: followup to CircleCI Sentry reporting (#27548)

## **Description**

Implementing some suggestions from @matthewwalsh0 from #27412

~Also incorporates changes from @legobeat's #27268~

- Renamed `doNotForceSentryForThisTest` to `doNotForceForThisTest`
because the `Sentry` is now implied by the parent property
- Abstracted to `addFlagsFromPrBody()` and `addFlagsFromGitMessage()`
functions
- Only supports one flag right now (`tracesSampleRate`) but it's built
to be easily extendable for anything
- It's now an incredibly powerful general way to pass runtime flags into
the Extension in CircleCI, either through the PR body or through the Git
commit message
  - In either the PR body or the Git commit message, add a line like
  `flags = {"sentry": {"tracesSampleRate": x.xx}}`
  If you do both, Git commit message takes precedence
  - This changes the format from
  `[flags.sentry.tracesSampleRate: x.xx]` to
  `flags = {"sentry": {"tracesSampleRate": x.xx}}`

Note: This PR, as is, will hit the following error because it's trying
to actually parse the sample code above with `x.xx`. The good news is it
fails gracefully.
```
Error parsing flags from PR body, ignoring flags
 SyntaxError: Unexpected token 'x', ..."pleRate": x.xx}}" is not valid JSON
```

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27548?quickstart=1)

## **Related issues**

Followup to: #27412

## **Manual testing steps**

## **Screenshots/Recordings**
### **Before**
### **After**
## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

.circleci/scripts/git-diff-develop.ts

@@ -104,12 +104,18 @@ async function gitDiff(): Promise<string> {
   return diffResult;
 }
 
+function writePrBodyToFile(prBody: string) {
+  const prBodyPath = path.resolve(CHANGED_FILES_DIR, 'pr-body.txt');
+  fs.writeFileSync(prBodyPath, prBody.trim());
+  console.log(`PR body saved to ${prBodyPath}`);
+}
+
 /**
- * Stores the output of git diff to a file.
+ * Main run function, stores the output of git diff and the body of the matching PR to a file.
  *
- * @returns Returns a promise that resolves when the git diff output is successfully stored.
+ * @returns Returns a promise that resolves when the git diff output and PR body is successfully stored.
  */
-async function storeGitDiffOutput() {
+async function storeGitDiffOutputAndPrBody() {
   try {
     // Create the directory
     // This is done first because our CirleCI config requires that this directory is present,
@@ -132,6 +138,7 @@ async function storeGitDiffOutput() {
       return;
     } else if (baseRef !== MAIN_BRANCH) {
       console.log(`This is for a PR targeting '${baseRef}', skipping git diff`);
+      writePrBodyToFile(prInfo.body);
       return;
     }
 
@@ -142,13 +149,15 @@ async function storeGitDiffOutput() {
     // Store the output of git diff
     const outputPath = path.resolve(CHANGED_FILES_DIR, 'changed-files.txt');
     fs.writeFileSync(outputPath, diffOutput.trim());
-
     console.log(`Git diff results saved to ${outputPath}`);
+
+    writePrBodyToFile(prInfo.body);
+
     process.exit(0);
   } catch (error: any) {
     console.error('An error occurred:', error.message);
     process.exit(1);
   }
 }
 
-storeGitDiffOutput();
+storeGitDiffOutputAndPrBody();

app/scripts/lib/manifestFlags.ts

@@ -11,7 +11,7 @@ export type ManifestFlags = {
   };
   sentry?: {
     tracesSampleRate?: number;
-    doNotForceSentryForThisTest?: boolean;
+    forceEnable?: boolean;
   };
 };
 

app/scripts/lib/setupSentry.js

@@ -123,7 +123,7 @@ function getTracesSampleRate(sentryTarget) {
 
   if (flags.circleci) {
     // Report very frequently on develop branch, and never on other branches
-    // (Unless you do a [flags.sentry.tracesSampleRate: x.xx] override)
+    // (Unless you use a `flags = {"sentry": {"tracesSampleRate": x.xx}}` override)
     if (flags.circleci.branch === 'develop') {
       return 0.03;
     }
@@ -238,7 +238,7 @@ function getSentryEnvironment() {
 
 function getSentryTarget() {
   if (
-    getManifestFlags().sentry?.doNotForceSentryForThisTest ||
+    !getManifestFlags().sentry?.forceEnable ||
     (process.env.IN_TEST && !SENTRY_DSN_DEV)
   ) {
     return SENTRY_DSN_FAKE;
@@ -272,7 +272,7 @@ async function getMetaMetricsEnabled() {
 
   if (
     METAMASK_BUILD_TYPE === 'mmi' ||
-    (flags.circleci && !flags.sentry?.doNotForceSentryForThisTest)
+    (flags.circleci && flags.sentry.forceEnable)
   ) {
     return true;
   }

test/e2e/set-manifest-flags.ts

@@ -1,5 +1,6 @@
 import { execSync } from 'child_process';
 import fs from 'fs';
+import { merge } from 'lodash';
 import { ManifestFlags } from '../../app/scripts/lib/manifestFlags';
 
 export const folder = `dist/${process.env.SELENIUM_BROWSER}`;
@@ -8,23 +9,82 @@ function parseIntOrUndefined(value: string | undefined): number | undefined {
   return value ? parseInt(value, 10) : undefined;
 }
 
-// Grab the tracesSampleRate from the git message if it's set
-function getTracesSampleRateFromGitMessage(): number | undefined {
+/**
+ * Search a string for `flags = {...}` and return ManifestFlags if it exists
+ *
+ * @param str - The string to search
+ * @param errorType - The type of error to log if parsing fails
+ * @returns The ManifestFlags object if valid, otherwise undefined
+ */
+function regexSearchForFlags(
+  str: string,
+  errorType: string,
+): ManifestFlags | undefined {
+  // Search str for `flags = {...}`
+  const flagsMatch = str.match(/flags\s*=\s*(\{.*\})/u);
+
+  if (flagsMatch) {
+    try {
+      // Get 1st capturing group from regex
+      return JSON.parse(flagsMatch[1]);
+    } catch (error) {
+      console.error(
+        `Error parsing flags from ${errorType}, ignoring flags\n`,
+        error,
+      );
+    }
+  }
+
+  return undefined;
+}
+
+/**
+ * Add flags from the GitHub PR body if they are set
+ *
+ * To use this feature, add a line to your PR body like:
+ * `flags = {"sentry": {"tracesSampleRate": 0.1}}`
+ * (must be valid JSON)
+ *
+ * @param flags - The flags object to add to
+ */
+function addFlagsFromPrBody(flags: ManifestFlags) {
+  let body;
+
+  try {
+    body = fs.readFileSync('changed-files/pr-body.txt', 'utf8');
+  } catch (error) {
+    console.debug('No pr-body.txt, ignoring flags');
+    return;
+  }
+
+  const newFlags = regexSearchForFlags(body, 'PR body');
+
+  if (newFlags) {
+    // Use lodash merge to do a deep merge (spread operator is shallow)
+    merge(flags, newFlags);
+  }
+}
+
+/**
+ * Add flags from the Git message if they are set
+ *
+ * To use this feature, add a line to your commit message like:
+ * `flags = {"sentry": {"tracesSampleRate": 0.1}}`
+ * (must be valid JSON)
+ *
+ * @param flags - The flags object to add to
+ */
+function addFlagsFromGitMessage(flags: ManifestFlags) {
   const gitMessage = execSync(
     `git show --format='%B' --no-patch "HEAD"`,
   ).toString();
 
-  // Search gitMessage for `[flags.sentry.tracesSampleRate: 0.000 to 1.000]`
-  const tracesSampleRateMatch = gitMessage.match(
-    /\[flags\.sentry\.tracesSampleRate: (0*(\.\d+)?|1(\.0*)?)\]/u,
-  );
+  const newFlags = regexSearchForFlags(gitMessage, 'git message');
 
-  if (tracesSampleRateMatch) {
-    // Return 1st capturing group from regex
-    return parseFloat(tracesSampleRateMatch[1]);
+  if (newFlags) {
+    // Use lodash merge to do a deep merge (spread operator is shallow)
+    merge(flags, newFlags);
   }
-
-  return undefined;
 }
 
 // Alter the manifest with CircleCI environment variables and custom flags
@@ -41,12 +101,15 @@ export function setManifestFlags(flags: ManifestFlags = {}) {
       ),
     };
 
-    const tracesSampleRate = getTracesSampleRateFromGitMessage();
+    addFlagsFromPrBody(flags);
+    addFlagsFromGitMessage(flags);
 
-    // 0 is a valid value, so must explicitly check for undefined
-    if (tracesSampleRate !== undefined) {
-      // Add tracesSampleRate to flags.sentry (which may or may not already exist)
-      flags.sentry = { ...flags.sentry, tracesSampleRate };
+    // Set `flags.sentry.forceEnable` to true by default
+    if (flags.sentry === undefined) {
+      flags.sentry = {};
+    }
+    if (flags.sentry.forceEnable === undefined) {
+      flags.sentry.forceEnable = true;
     }
   }
 

test/e2e/tests/metrics/errors.spec.js

@@ -247,7 +247,7 @@ describe('Sentry errors', function () {
           title: this.test.fullTitle(),
           testSpecificMock: mockSentryMigratorError,
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -278,7 +278,7 @@ describe('Sentry errors', function () {
           title: this.test.fullTitle(),
           testSpecificMock: mockSentryTestError,
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -319,7 +319,7 @@ describe('Sentry errors', function () {
           title: this.test.fullTitle(),
           testSpecificMock: mockSentryMigratorError,
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -365,7 +365,7 @@ describe('Sentry errors', function () {
           title: this.test.fullTitle(),
           testSpecificMock: mockSentryMigratorError,
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -426,7 +426,7 @@ describe('Sentry errors', function () {
           title: this.test.fullTitle(),
           testSpecificMock: mockSentryInvariantMigrationError,
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -475,7 +475,7 @@ describe('Sentry errors', function () {
           testSpecificMock: mockSentryTestError,
           ignoredConsoleErrors: ['TestError'],
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -521,7 +521,7 @@ describe('Sentry errors', function () {
           testSpecificMock: mockSentryTestError,
           ignoredConsoleErrors: ['TestError'],
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -585,7 +585,7 @@ describe('Sentry errors', function () {
           title: this.test.fullTitle(),
           testSpecificMock: mockSentryTestError,
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -621,7 +621,7 @@ describe('Sentry errors', function () {
           testSpecificMock: mockSentryTestError,
           ignoredConsoleErrors: ['TestError'],
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -656,7 +656,7 @@ describe('Sentry errors', function () {
           title: this.test.fullTitle(),
           testSpecificMock: mockSentryTestError,
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -702,7 +702,7 @@ describe('Sentry errors', function () {
           title: this.test.fullTitle(),
           testSpecificMock: mockSentryTestError,
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, ganacheServer, mockedEndpoint }) => {
@@ -766,7 +766,7 @@ describe('Sentry errors', function () {
           testSpecificMock: mockSentryTestError,
           ignoredConsoleErrors: ['TestError'],
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, mockedEndpoint }) => {
@@ -810,7 +810,7 @@ describe('Sentry errors', function () {
           testSpecificMock: mockSentryTestError,
           ignoredConsoleErrors: ['TestError'],
           manifestFlags: {
-            sentry: { doNotForceSentryForThisTest: true },
+            sentry: { forceEnable: false },
           },
         },
         async ({ driver, ganacheServer, mockedEndpoint }) => {
@@ -898,7 +898,7 @@ describe('Sentry errors', function () {
         ganacheOptions,
         title: this.test.fullTitle(),
         manifestFlags: {
-          sentry: { doNotForceSentryForThisTest: true },
+          sentry: { forceEnable: false },
         },
       },
       async ({ driver }) => {

test/e2e/tests/metrics/sessions.spec.ts

@@ -38,7 +38,7 @@ describe('Sessions', function () {
         title: this.test?.fullTitle(),
         testSpecificMock: mockSentrySession,
         manifestFlags: {
-          sentry: { doNotForceSentryForThisTest: true },
+          sentry: { forceEnable: false },
         },
       },
       async ({ driver, mockedEndpoint }) => {
@@ -60,7 +60,7 @@ describe('Sessions', function () {
         title: this.test?.fullTitle(),
         testSpecificMock: mockSentrySession,
         manifestFlags: {
-          sentry: { doNotForceSentryForThisTest: true },
+          sentry: { forceEnable: false },
         },
       },
       async ({ driver, mockedEndpoint }) => {

test/e2e/tests/metrics/traces.spec.ts

@@ -51,7 +51,7 @@ describe('Traces', function () {
         title: this.test?.fullTitle(),
         testSpecificMock: mockSentryCustomTrace,
         manifestFlags: {
-          sentry: { doNotForceSentryForThisTest: true },
+          sentry: { forceEnable: false },
         },
       },
       async ({ driver, mockedEndpoint }) => {
@@ -73,7 +73,7 @@ describe('Traces', function () {
         title: this.test?.fullTitle(),
         testSpecificMock: mockSentryCustomTrace,
         manifestFlags: {
-          sentry: { doNotForceSentryForThisTest: true },
+          sentry: { forceEnable: false },
         },
       },
       async ({ driver, mockedEndpoint }) => {
@@ -95,7 +95,7 @@ describe('Traces', function () {
         title: this.test?.fullTitle(),
         testSpecificMock: mockSentryAutomatedTrace,
         manifestFlags: {
-          sentry: { doNotForceSentryForThisTest: true },
+          sentry: { forceEnable: false },
         },
       },
       async ({ driver, mockedEndpoint }) => {
@@ -117,7 +117,7 @@ describe('Traces', function () {
         title: this.test?.fullTitle(),
         testSpecificMock: mockSentryAutomatedTrace,
         manifestFlags: {
-          sentry: { doNotForceSentryForThisTest: true },
+          sentry: { forceEnable: false },
         },
       },
       async ({ driver, mockedEndpoint }) => {