From 687cf3a2d23a6dba6fb672029095a537d7902582 Mon Sep 17 00:00:00 2001 From: Howard Braham Date: Thu, 10 Oct 2024 00:39:15 -0700 Subject: [PATCH] ci: followup to CircleCI Sentry reporting (#27548) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **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 | 19 +++-- app/scripts/lib/manifestFlags.ts | 2 +- app/scripts/lib/setupSentry.js | 6 +- test/e2e/set-manifest-flags.ts | 95 ++++++++++++++++++++----- test/e2e/tests/metrics/errors.spec.js | 28 ++++---- test/e2e/tests/metrics/sessions.spec.ts | 4 +- test/e2e/tests/metrics/traces.spec.ts | 8 +-- 7 files changed, 117 insertions(+), 45 deletions(-) diff --git a/.circleci/scripts/git-diff-develop.ts b/.circleci/scripts/git-diff-develop.ts index 9f6c8f0ae4df..43435db17418 100644 --- a/.circleci/scripts/git-diff-develop.ts +++ b/.circleci/scripts/git-diff-develop.ts @@ -104,12 +104,18 @@ async function gitDiff(): Promise { 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,8 +149,10 @@ 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); @@ -151,4 +160,4 @@ async function storeGitDiffOutput() { } } -storeGitDiffOutput(); +storeGitDiffOutputAndPrBody(); diff --git a/app/scripts/lib/manifestFlags.ts b/app/scripts/lib/manifestFlags.ts index a013373ac9f2..93925bf63a0c 100644 --- a/app/scripts/lib/manifestFlags.ts +++ b/app/scripts/lib/manifestFlags.ts @@ -11,7 +11,7 @@ export type ManifestFlags = { }; sentry?: { tracesSampleRate?: number; - doNotForceSentryForThisTest?: boolean; + forceEnable?: boolean; }; }; diff --git a/app/scripts/lib/setupSentry.js b/app/scripts/lib/setupSentry.js index e6f4a0d4524e..d440578144cc 100644 --- a/app/scripts/lib/setupSentry.js +++ b/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; } diff --git a/test/e2e/set-manifest-flags.ts b/test/e2e/set-manifest-flags.ts index e8d02a12e2cd..290e8b863a9e 100644 --- a/test/e2e/set-manifest-flags.ts +++ b/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; } } diff --git a/test/e2e/tests/metrics/errors.spec.js b/test/e2e/tests/metrics/errors.spec.js index fdeb4437d428..dfe77f758fcb 100644 --- a/test/e2e/tests/metrics/errors.spec.js +++ b/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 }) => { diff --git a/test/e2e/tests/metrics/sessions.spec.ts b/test/e2e/tests/metrics/sessions.spec.ts index f1bdee4538fb..7c79e5510116 100644 --- a/test/e2e/tests/metrics/sessions.spec.ts +++ b/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 }) => { diff --git a/test/e2e/tests/metrics/traces.spec.ts b/test/e2e/tests/metrics/traces.spec.ts index 194f36ff73b0..9166281f90e5 100644 --- a/test/e2e/tests/metrics/traces.spec.ts +++ b/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 }) => {