ci: followup to CircleCI Sentry reporting #27548

HowardBraham · 2024-10-01T22:09:23Z

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

Related issues

Followup to: #27412

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

github-actions · 2024-10-01T22:09:34Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

metamaskbot · 2024-10-01T23:13:29Z

Builds ready [a6d14e0]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1780 ± 96 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	260	2532	1647	509	244
		domContentLoaded	1550	2275	1751	174	83
		load	1557	2404	1780	199	96
		domInteractive	15	70	42	14	7

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: -12 Bytes (-0.00%)

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

legobeat · 2024-10-02T01:08:12Z

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

  }
  return diffResult;
 }

+function writePrBodyToFile(prBody: string) {


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

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.

metamaskbot · 2024-10-02T02:02:09Z

Builds ready [18502f9]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2089 ± 186 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	256	2530	1831	589	283
		domContentLoaded	1683	3404	2064	384	185
		load	1692	3409	2089	388	186
		domInteractive	18	416	67	83	40

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: -12 Bytes (-0.00%)

test/e2e/set-manifest-flags.ts

metamaskbot · 2024-10-08T04:10:13Z

Builds ready [038a11c]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1930 ± 97 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	449	2644	1849	385	185
		domContentLoaded	1667	2540	1908	202	97
		load	1686	2570	1930	202	97
		domInteractive	24	112	53	25	12

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: -12 Bytes (-0.00%)

Gudahtt · 2024-10-08T12:17:13Z

What is the intended purpose of this feature? i.e. when would we want to update the Sentry trace rate from a PR description. I see that this capability was added already in #27412, but I don't see a reference to its purpose there either.

Edit: I see, tracing is entirely disabled on PRs by default, so this would be for getting performance metrics from PRs I suppose. Makes sense.

app/scripts/lib/setupSentry.js

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

Gudahtt

Generally this looks good! But it would be ideal to make the unrelated changes to git-diff-develop in a separate PR. This is recommended by our contributor guidelines not just to make review easier and more effective, but also to make the git history easier to audit.

#27548 (review)

…ent property

Requested changes have been made

Gudahtt

LGTM!

sonarcloud · 2024-10-08T21:14:10Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

metamaskbot · 2024-10-08T21:52:19Z

Builds ready [2d953bf]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1860 ± 66 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1636	2217	1862	145	69
		domContentLoaded	1625	2212	1822	143	69
		load	1639	2216	1860	138	66
		domInteractive	24	99	41	19	9

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: -66 Bytes (-0.00%)

Gudahtt · 2024-10-15T20:10:10Z

app/scripts/lib/setupSentry.js

@@ -238,7 +238,7 @@ function getSentryEnvironment() {

 function getSentryTarget() {
  if (
-    getManifestFlags().sentry?.doNotForceSentryForThisTest ||
+    !getManifestFlags().sentry?.forceEnable ||


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

github-actions bot added the team-contributor-experience MetaMask Contributor Experience Group label Oct 1, 2024

HowardBraham changed the title ~~Sentry reporting develop~~ ci: followup to CircleCI Sentry reporting Oct 1, 2024

HowardBraham added team-tiger Tiger team (for tech debt reduction + performance improvements) and removed team-contributor-experience MetaMask Contributor Experience Group labels Oct 1, 2024

HowardBraham self-assigned this Oct 1, 2024

HowardBraham force-pushed the sentry-reporting-develop branch from 0839055 to a6d14e0 Compare October 1, 2024 22:32

HowardBraham force-pushed the sentry-reporting-develop branch 5 times, most recently from f8bbc5b to 7f0edcf Compare October 2, 2024 01:03

legobeat reviewed Oct 2, 2024

View reviewed changes

.circleci/scripts/git-diff-develop.ts Outdated Show resolved Hide resolved

.circleci/scripts/git-diff-develop.ts Outdated Show resolved Hide resolved

.circleci/scripts/git-diff-develop.ts Outdated Show resolved Hide resolved

legobeat reviewed Oct 2, 2024

View reviewed changes

HowardBraham marked this pull request as ready for review October 2, 2024 02:37

HowardBraham requested review from kumavis and a team as code owners October 2, 2024 02:37

HowardBraham requested review from matthewwalsh0, Gudahtt and legobeat October 2, 2024 02:37

legobeat previously approved these changes Oct 8, 2024

View reviewed changes

legobeat mentioned this pull request Oct 8, 2024

fix(deps): update from eth-rpc-errors to @metamask/rpc-errors (cause edition) #24496

Merged

10 tasks

legobeat reviewed Oct 8, 2024

View reviewed changes

test/e2e/set-manifest-flags.ts Show resolved Hide resolved

matthewwalsh0 previously approved these changes Oct 8, 2024

View reviewed changes

Gudahtt reviewed Oct 8, 2024

View reviewed changes

app/scripts/lib/setupSentry.js Outdated Show resolved Hide resolved

Gudahtt reviewed Oct 8, 2024

View reviewed changes

.circleci/scripts/git-diff-develop.ts Show resolved Hide resolved

Gudahtt previously requested changes Oct 8, 2024

View reviewed changes

Gudahtt mentioned this pull request Oct 8, 2024

ci: make git-diff-develop work for PRs from foreign repos #27268

Merged

HowardBraham added a commit to legobeat/metamask-extension that referenced this pull request Oct 8, 2024

back-ported changes from MetaMask#27548

c554b95

HowardBraham added 5 commits October 8, 2024 13:55

remove the Sentry from this property as it's now implied by the par…

057ce5f

…ent property

general getFlagsFromGitMessage() function

32ff8e2

addFlagsFromPrBody

dc6ac27

doNotForceForThisTest -> forceEnable

4a6adba

minor JSDoc change

2d953bf

HowardBraham dismissed matthewwalsh0’s stale review via 2d953bf October 8, 2024 21:02

HowardBraham force-pushed the sentry-reporting-develop branch from 038a11c to 2d953bf Compare October 8, 2024 21:02

HowardBraham requested review from legobeat and matthewwalsh0 October 8, 2024 21:09

Gudahtt approved these changes Oct 8, 2024

View reviewed changes

matthewwalsh0 approved these changes Oct 10, 2024

View reviewed changes

HowardBraham added this pull request to the merge queue Oct 10, 2024

Merged via the queue into develop with commit 687cf3a Oct 10, 2024
83 of 84 checks passed

HowardBraham deleted the sentry-reporting-develop branch October 10, 2024 08:01

github-actions bot locked and limited conversation to collaborators Oct 10, 2024

metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 10, 2024

Gudahtt reviewed Oct 15, 2024

View reviewed changes

gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: followup to CircleCI Sentry reporting #27548

ci: followup to CircleCI Sentry reporting #27548

HowardBraham commented Oct 1, 2024 •

edited by legobeat

Loading

github-actions bot commented Oct 1, 2024

metamaskbot commented Oct 1, 2024

legobeat Oct 2, 2024

HowardBraham Oct 2, 2024

metamaskbot commented Oct 2, 2024

metamaskbot commented Oct 8, 2024

Gudahtt commented Oct 8, 2024 •

edited

Loading

Gudahtt left a comment

Gudahtt left a comment

sonarcloud bot commented Oct 8, 2024

metamaskbot commented Oct 8, 2024

Gudahtt Oct 15, 2024

ci: followup to CircleCI Sentry reporting #27548

ci: followup to CircleCI Sentry reporting #27548

Conversation

HowardBraham commented Oct 1, 2024 • edited by legobeat Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 1, 2024

metamaskbot commented Oct 1, 2024

legobeat Oct 2, 2024

Choose a reason for hiding this comment

HowardBraham Oct 2, 2024

Choose a reason for hiding this comment

metamaskbot commented Oct 2, 2024

metamaskbot commented Oct 8, 2024

Gudahtt commented Oct 8, 2024 • edited Loading

Gudahtt left a comment

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 8, 2024

Quality Gate failed

metamaskbot commented Oct 8, 2024

Gudahtt Oct 15, 2024

Choose a reason for hiding this comment

HowardBraham commented Oct 1, 2024 •

edited by legobeat

Loading

Gudahtt commented Oct 8, 2024 •

edited

Loading