Skip to content

fix: Fix MV2 build sourcemap upload#26467

Merged
Gudahtt merged 6 commits intodevelopfrom
fix-mv2-sourcemap-upload
Aug 21, 2024
Merged

fix: Fix MV2 build sourcemap upload#26467
Gudahtt merged 6 commits intodevelopfrom
fix-mv2-sourcemap-upload

Conversation

@Gudahtt
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt commented Aug 16, 2024

Description

The sourcemaps for MV2 builds (used for Firefox) were not being uploaded to Sentry at all. This resulted in invalid stack traces for Firefox error reports.

The Sentry initiatization has been updated to use a dist option, letting us differentiate between different types of build for the same version. This is now used to signify which builds are mv2 and which are mv3. Both distributions are uploaded separately as part of the release process (for Flask and main builds; we don't have MV2 builds for MMI or beta).

Open in GitHub Codespaces

Related issues

Fixes #26466

Manual testing steps

Unfortunately I don't know of an easy way to test the CircleCI changes. Those will just have to be read carefully.

We can test the application changes and the Sentry script changes though. The two outcomes we want to test are:

  • The source code and sourcemaps are uploaded correctly for both MV2 and MV3 builds
  • Application bundles for MV2 and MV3 builds are reporting errors properly tagged as being from an MV2 or MV3 build, and are mapped correctly in Sentry.

Here are the steps I used to test this:

  • Setup a personal Sentry account with a metamask project
  • Create a Custom Integration in Sentry (Settings -> Custom Integrations)
    • Steps:
      • Navigate to "Settings -> Custom Integrations" on the Sentry dashboard for your personal Sentry account
      • Click the purple "Create New Integration" button on the top-right of the page
      • Select "Internal integration"
      • Provide any name, and grant it "Admin" access to "Releases"
    • Alternatively an Auth Token might work as well, but some of the commands used by our script to check for pre-existing releases seem to not work with the org:ci scope, which is the only scope available for Sentry auth tokens.
  • Checkout this branch
  • Bump the patch version in package.json (just in case you need to re-test, this is an easy way to separate old errors/builds from new ones)
  • Before creating builds, make sure that SENTRY_DSN_DEV is set to the DSN of your personal Sentry account's metamask project
  • Run yarn dist:mv2 to create an MV2 build
  • Move it to the dist-mv2 directory (mv dist dist-mv2)
  • Run yarn dist to create an MV3 build
  • Before uploading sourcemaps, make sure that SENTRY_AUTH_TOKEN is set to the Auth Token generated from the custom integration earlier.
  • Run yarn sentry:publish --org [your organization] to upload the MV3 build
  • Run yarn sentry:publish --org [your organization] --dist mv2 to upload the MV2 build

At this point, you should be able to see the releases on the Sentry dashboard along with the artifacts. Look in "Settings -> Projects -> Source Maps" for these. They are labeled by release number and dist.

Now, load each build in your browser (one at a time, never both enabled at once) and follow these steps:

  • Proceed through onboarding, opting in to MetaMetrics
  • Navigate to the test-dapp and connect to it
  • Click the "INVALID TRANSACTION TYPE (NOT SUPPORTED)" button in the "Malformed Transactions" section of the test dapp, then reject the confirmation after it shows up. This should trigger an error in Sentry.
  • Look for the error in Sentry and ensure that the frame that shows transactionController.updateSecurityAlertResponse( is mapped to source code correctly (it should resolve to app/scripts/lib/ppom/ppom-util.ts). Also check that the dist is correct in the "Tags" section of the Sentry issue page.
Here are screenshots of what that looked like for me:

Screenshot 2024-08-20 at 19 19 15

Screenshot 2024-08-20 at 19 06 08

Screenshots/Recordings

N/A

Pre-merge author checklist

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

From local testing, it seems that this was not working. It was returning empty even for releases with artifacts. I think this is a legacy command that no longer works due to a change in how Sentry tracks artifacts that was made >1 year ago (they're no longer considered as "release bundles", they're "artifact bundles" now, which aren't returned here.)

Since it wasn't working anyway, I removed it rather than trying to update it to use --dist correctly (it didn't seem to accept a --dist flag, as far as I could tell, so there was no easy way to get this to work).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This reflects the directory names we use in the CircleCI config

@Gudahtt Gudahtt added the team-extension-platform Extension Platform team label Aug 16, 2024
@Gudahtt Gudahtt force-pushed the fix-mv2-sourcemap-upload branch 3 times, most recently from 0001ba3 to c3b092e Compare August 16, 2024 13:45
@Gudahtt Gudahtt marked this pull request as ready for review August 16, 2024 14:20
@Gudahtt Gudahtt requested review from a team and kumavis as code owners August 16, 2024 14:20
DDDDDanica
DDDDDanica previously approved these changes Aug 16, 2024
@DDDDDanica
Copy link
Copy Markdown
Contributor

LGTM !

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [c3b092e]
Page Load Metrics (335 ± 298 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint692981265325
domContentLoaded106027126
load432005335621298
domInteractive106027126
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 163 Bytes (0.00%)

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.02%. Comparing base (590f0a9) to head (4c27315).

Files Patch % Lines
shared/modules/mv3.utils.js 0.00% 2 Missing ⚠️
app/scripts/lib/setupSentry.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26467      +/-   ##
===========================================
- Coverage    70.02%   70.02%   -0.00%     
===========================================
  Files         1406     1406              
  Lines        49076    49078       +2     
  Branches     13725    13727       +2     
===========================================
  Hits         34364    34364              
- Misses       14712    14714       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Aug 16, 2024

I manually tested this and found one bug, two parameters were in the wrong order. Fixed in be3203f

It appears to work correctly now.

@Gudahtt Gudahtt requested a review from DDDDDanica August 16, 2024 17:36
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [be3203f]
Page Load Metrics (76 ± 15 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint762841094522
domContentLoaded97927157
load43198763215
domInteractive97927157
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 163 Bytes (0.00%)

@Gudahtt Gudahtt added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 19, 2024
DDDDDanica
DDDDDanica previously approved these changes Aug 19, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [351625e]
Page Load Metrics (257 ± 263 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint661851112612
domContentLoaded96327157
load441999257548263
domInteractive96327157
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 162 Bytes (0.00%)

seaona
seaona previously approved these changes Aug 20, 2024
Copy link
Copy Markdown
Member

@seaona seaona left a comment

Choose a reason for hiding this comment

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

code LGTM and the functionality is working. Tested:

  • flask mv2
  • mv2
  • mv3
    All source maps are uploaded correctly to Sentry

Screenshot from 2024-08-20 18-12-26

@Gudahtt Gudahtt marked this pull request as draft August 20, 2024 19:05
@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Aug 20, 2024

Found a problem with the MV3 builds. They're still showing as having a dist of mv2 because isManifestV3 is coming out as false when loaded as part of setupSentry.js even on MV3 builds.

@Gudahtt Gudahtt marked this pull request as ready for review August 20, 2024 21:29
@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Aug 20, 2024

The issue referenced here was resolved in this commit. This is ready for review again, I have re-tested it. Updating the testing instructions now.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [011ff47]
Page Load Metrics (349 ± 312 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint661591072010
domContentLoaded106933157
load452225349650312
domInteractive106933157
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 149 Bytes (0.00%)

@github-actions
Copy link
Copy Markdown
Contributor

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.

The sourcemaps for MV2 builds (used for Firefox) were not being
uploaded to Sentry at all. This resulted in invalid stack traces for
Firefox error reports.

The Sentry initiatization has been updated to use a `dist` option,
letting us differentiate between different types of build for the same
version. This is now used to signify which builds are mv2 and which are
mv3. Both distributions are uploaded separately as part of the release
process (for Flask and main builds; we don't have MV2 builds for MMI or
beta).

Fixes #26466
This reverts commit 4bd50d4.
I was seeing a bug where MV3 builds were still set as `dist: 'mv2'`.
This was because `isManifestV3` was resolving to `false` in the
`sentry-install.js` bundle for some reason.

It turns out that the runtime manifest is unavailable because the
Chrome extension API doesn't exist in the global scope early on in a
service worker process. It magically appears some time later. So in
this case, the fallback `process.env.ENABLE_MV3` was used. This
fallback case was broken because it expected `ENABLE_MV3` to be set
to the string `"true"`, but it was set to the boolean `true`. The
fallback check has been updated to account for both cases.
@Gudahtt Gudahtt force-pushed the fix-mv2-sourcemap-upload branch from 85b1aa2 to 4c27315 Compare August 21, 2024 12:57
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@desi desi left a comment

Choose a reason for hiding this comment

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

I don't see anything here that is alarming from a circle ci point of view and trust that the other reviewers would have caught issues in the other areas I am less familiar with. LGTM.

@Gudahtt Gudahtt merged commit 5781edb into develop Aug 21, 2024
@Gudahtt Gudahtt deleted the fix-mv2-sourcemap-upload branch August 21, 2024 13:20
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 21, 2024
@metamaskbot metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 21, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [4c27315]
Page Load Metrics (76 ± 8 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint741891032412
domContentLoaded4911570178
load4911976178
domInteractive10472584
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 149 Bytes (0.00%)

@gauthierpetetin gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release-12.4.0 Issue or pull request that will be included in release 12.4.0 team-extension-platform Extension Platform team

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[Bug]: Sentry sourcemaps misconfigured for Firefox

8 participants