-
Notifications
You must be signed in to change notification settings - Fork 5k
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.
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
feat(sentry): Adding extensionId and installType to Sentry #26482
Conversation
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. |
ec8df4f
to
e9dfb1f
Compare
Builds ready [925845a]
Page Load Metrics (102 ± 26 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #26482 +/- ##
===========================================
- Coverage 70.02% 70.01% -0.01%
===========================================
Files 1408 1409 +1
Lines 49105 49115 +10
Branches 13735 13737 +2
===========================================
+ Hits 34383 34386 +3
- Misses 14722 14729 +7 ☔ View full report in Codecov by Sentry. |
e08698f
to
1c74bad
Compare
Builds ready [1c74bad]
Page Load Metrics (106 ± 31 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good! No tests though. We could add this to the existing errors e2e tests, maybe as an additional assertion for the "should capture ___ state" tests?
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
1c74bad
to
649e677
Compare
Added tests in 649e677 |
Builds ready [844d090]
Page Load Metrics (70 ± 7 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* origin/develop: feat: Redesign Approve confirmation (#26464) fix: Fix MV2 build sourcemap upload (#26467) feat: Enable hardware wallets for smart transactions, sign a transaction only once (#26251) fix: Allowlist Snap UI card component (#26565) fix(deps): Bump `@metamask/eth-json-rpc-middleware` to `^14.0.0`, `@metamask/transaction-controller` to `^35.1.1` (#26143) fix: adding warning for origin on redesigned pages (#26306) fix: track `swapAndSend` transaction type (#26535)
@@ -1,12 +1,13 @@ | |||
import * as Sentry from '@sentry/browser'; | |||
import { createModuleLogger, createProjectLogger } from '@metamask/utils'; | |||
import { logger } from '@sentry/utils'; | |||
import { AllProperties } from '../../../shared/modules/object.utils'; | |||
import browser from 'webextension-polyfill'; | |||
import { isManifestV3 } from '../../../shared/modules/mv3.utils'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only line with a conflict in the most recent merge with develop. I just had to preserve this import
Just merged with |
Quality Gate passedIssues Measures |
Builds ready [3184f9f]
Page Load Metrics (79 ± 8 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Description
We want to differentiate between errors from our published extension, and errors from side-loaded extensions or forks that we don't support. Adding
extensionID
to our Sentry logs helps us do that. Following @Gudahtt's suggestion to use https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/management/getSelf to addinstallType
as well.Related issues
Fixes: #26444
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist