-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
refactor: Send signature metrics as event fragments in middleware & feat: Add signature alert metrics to events #26597
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. |
8d7328d
to
cd0208f
Compare
…4636) ## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> Adding the request id to reference metric event fragments created from the createRPCMethodTrackingMiddleware in the client. See use-case here: MetaMask/metamask-extension#26597 ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? For example: Blocks: MetaMask/metamask-extension#26597 * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/message-manager` - **feat**: Add request id to messageParams ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Jongsun Suh <jongsun.suh@icloud.com>
This reverts commit a06925a.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #26597 +/- ##
===========================================
- Coverage 70.17% 70.14% -0.03%
===========================================
Files 1425 1425
Lines 49659 49684 +25
Branches 13891 13898 +7
===========================================
+ Hits 34846 34850 +4
- Misses 14813 14834 +21 ☔ View full report in Codecov by Sentry. |
Builds ready [ad65b1c]
Page Load Metrics (1896 ± 153 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [123fd08]
Page Load Metrics (1587 ± 67 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
const requestId = (currentConfirmation as SignatureRequestType)?.msgParams | ||
?.requestId; | ||
const fragmentUniqueId = `signature-${requestId}`; | ||
updateEventFragment(fragmentUniqueId, { |
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.
Is it worth a parallel useSignatureEventFragment
hook to encapsulate the generation of the fragment ID and allow other parts of the UI to add properties to signatures?
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.
hey @matthewwalsh0! I think this is a good idea. We don't have additional use-cases for this yet, but I imagine we will. I have some code written now, but I think it should go in a separate PR. Will create one following the merge of this one
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.
created here: #27043
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.
or something similar to updateTransactionEventFragment
to encapsulate this whole thing.
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.
updateTransactionEventFragment
is included in useSignatureEventFragment
in #27043
I added a brief description of my thoughts using a hook vs. util function in the PR description. Curious if you two have additional opinions on this
heads up, PR is in draft as I'm working on supporting tests. I don't see unit tests for updateTransactionEventFragment
so I think it relies on other tests to support the hook. Will take more time to investigate later
properties, | ||
}); | ||
if (signatureUniqueId) { | ||
metaMetricsController.updateEventFragment(signatureUniqueId, { |
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.
❤️
Quality Gate passedIssues Measures |
Builds ready [8cb7817]
Page Load Metrics (1733 ± 52 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
finalizeSignatureFragment(metaMetricsController, req, { | ||
abandoned: event === eventType.REJECTED, | ||
properties, | ||
}); |
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.
Hey @digiwand is event of type REJECTED received in this middleware ?
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.
hi @jpuri, yep! Within this next() node.js callback, the event is set based on the response above. This may include eventType.REJECTED
Description
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2718
Blocked by: https://github.com/MetaMask/core/pull/4636/files and following PRs to update packages
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist