Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ci: followup to CircleCI Sentry reporting #27548

Merged
merged 5 commits into from
Oct 10, 2024
Merged

ci: followup to CircleCI Sentry reporting #27548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
057ce5f
remove the `Sentry` from this property as it's now implied by the par…
HowardBraham Sep 30, 2024
32ff8e2
general getFlagsFromGitMessage() function
HowardBraham Oct 1, 2024
dc6ac27
addFlagsFromPrBody
HowardBraham Oct 2, 2024
4a6adba
doNotForceForThisTest -> forceEnable
HowardBraham Oct 8, 2024
2d953bf
minor JSDoc change
HowardBraham Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions .circleci/scripts/git-diff-develop.ts
View file
Open in desktop
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,18 @@ async function gitDiff(): Promise<string> {
return diffResult;
}

function writePrBodyToFile(prBody: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Why is the PR body written to file? Is there something that prevents it from being passed differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. This needs to get to all of the test-e2e-* VMs. I'm trying to avoid calling api.github.com hundreds of times, and instead store it in artifacts.

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,
Expand All @@ -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;
}

Expand All @@ -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();
2 changes: 1 addition & 1 deletion app/scripts/lib/manifestFlags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export type ManifestFlags = {
};
sentry?: {
tracesSampleRate?: number;
doNotForceSentryForThisTest?: boolean;
forceEnable?: boolean;
};
};

Expand Down
6 changes: 3 additions & 3 deletions app/scripts/lib/setupSentry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -238,7 +238,7 @@ function getSentryEnvironment() {

function getSentryTarget() {
if (
getManifestFlags().sentry?.doNotForceSentryForThisTest ||
!getManifestFlags().sentry?.forceEnable ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to change the || here to && because we flipped the meaning of the flag

(process.env.IN_TEST && !SENTRY_DSN_DEV)
) {
return SENTRY_DSN_FAKE;
Expand Down Expand Up @@ -272,7 +272,7 @@ async function getMetaMetricsEnabled() {

if (
METAMASK_BUILD_TYPE === 'mmi' ||
(flags.circleci && !flags.sentry?.doNotForceSentryForThisTest)
(flags.circleci && flags.sentry.forceEnable)
) {
return true;
}
Expand Down
95 changes: 79 additions & 16 deletions test/e2e/set-manifest-flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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}`;
Expand All @@ -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);
legobeat marked this conversation as resolved.
Show resolved Hide resolved

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
Expand All @@ -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;
}
}

Expand Down
28 changes: 14 additions & 14 deletions test/e2e/tests/metrics/errors.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ describe('Sentry errors', function () {
title: this.test.fullTitle(),
testSpecificMock: mockSentryMigratorError,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -278,7 +278,7 @@ describe('Sentry errors', function () {
title: this.test.fullTitle(),
testSpecificMock: mockSentryTestError,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -319,7 +319,7 @@ describe('Sentry errors', function () {
title: this.test.fullTitle(),
testSpecificMock: mockSentryMigratorError,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -365,7 +365,7 @@ describe('Sentry errors', function () {
title: this.test.fullTitle(),
testSpecificMock: mockSentryMigratorError,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -426,7 +426,7 @@ describe('Sentry errors', function () {
title: this.test.fullTitle(),
testSpecificMock: mockSentryInvariantMigrationError,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -475,7 +475,7 @@ describe('Sentry errors', function () {
testSpecificMock: mockSentryTestError,
ignoredConsoleErrors: ['TestError'],
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -521,7 +521,7 @@ describe('Sentry errors', function () {
testSpecificMock: mockSentryTestError,
ignoredConsoleErrors: ['TestError'],
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -585,7 +585,7 @@ describe('Sentry errors', function () {
title: this.test.fullTitle(),
testSpecificMock: mockSentryTestError,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -621,7 +621,7 @@ describe('Sentry errors', function () {
testSpecificMock: mockSentryTestError,
ignoredConsoleErrors: ['TestError'],
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -656,7 +656,7 @@ describe('Sentry errors', function () {
title: this.test.fullTitle(),
testSpecificMock: mockSentryTestError,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -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 }) => {
Expand Down Expand Up @@ -766,7 +766,7 @@ describe('Sentry errors', function () {
testSpecificMock: mockSentryTestError,
ignoredConsoleErrors: ['TestError'],
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down Expand Up @@ -810,7 +810,7 @@ describe('Sentry errors', function () {
testSpecificMock: mockSentryTestError,
ignoredConsoleErrors: ['TestError'],
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, ganacheServer, mockedEndpoint }) => {
Expand Down Expand Up @@ -898,7 +898,7 @@ describe('Sentry errors', function () {
ganacheOptions,
title: this.test.fullTitle(),
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver }) => {
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/tests/metrics/sessions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('Sessions', function () {
title: this.test?.fullTitle(),
testSpecificMock: mockSentrySession,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand All @@ -60,7 +60,7 @@ describe('Sessions', function () {
title: this.test?.fullTitle(),
testSpecificMock: mockSentrySession,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down
8 changes: 4 additions & 4 deletions test/e2e/tests/metrics/traces.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('Traces', function () {
title: this.test?.fullTitle(),
testSpecificMock: mockSentryCustomTrace,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand All @@ -73,7 +73,7 @@ describe('Traces', function () {
title: this.test?.fullTitle(),
testSpecificMock: mockSentryCustomTrace,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand All @@ -95,7 +95,7 @@ describe('Traces', function () {
title: this.test?.fullTitle(),
testSpecificMock: mockSentryAutomatedTrace,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand All @@ -117,7 +117,7 @@ describe('Traces', function () {
title: this.test?.fullTitle(),
testSpecificMock: mockSentryAutomatedTrace,
manifestFlags: {
sentry: { doNotForceSentryForThisTest: true },
sentry: { forceEnable: false },
},
},
async ({ driver, mockedEndpoint }) => {
Expand Down