-
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
feat: Add simulation metrics to "Transaction Submitted" and "Transaction Finalized" events #28240
feat: Add simulation metrics to "Transaction Submitted" and "Transaction Finalized" events #28240
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. |
case TransactionMetaMetricsEvent.finalized: | ||
transactionMetricsRequest.updateEventFragment(uniqueId, { | ||
properties: payload.properties, | ||
sensitiveProperties: payload.sensitiveProperties, | ||
}); | ||
break; | ||
|
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.
minor cleanup since they were calling the same logic
Builds ready [54d3cb3]
Page Load Metrics (1763 ± 53 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [b279b9c]
Page Load Metrics (1841 ± 103 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [0f4c0d9]
Page Load Metrics (1982 ± 106 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [671cadd]
Page Load Metrics (1936 ± 90 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
Great fix, though eager for us to come back to this soon to reduce coupling.
Thanks for the reviews and can do @matthewwalsh0! dedupe related comment: #28240 (comment) |
Builds ready [9bebfe6]
Page Load Metrics (2012 ± 70 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
This PR adds simulation metrics to the
Transaction Submitted
andTransaction Finalized
events.Please refer to Solution B) below for details.
The challenge here is that there are two separate transaction fragment buckets:
transaction-added-<id>
andtransaction-submitted-<id>
.transaction-added-<id>
fragments are associated with the events:transaction-submitted-<id>
fragments are associated with the events:Simulation metrics were added to "Transaction Approved" and "Transaction Rejected" events using updateEventFragment. When they were added here through the useTransactionEventFragment hook, the
transaction-submitted-<id>
fragment had not yet been created. Thetransaction-submitted-<id>
fragment is created after a user clicks the Confirm button on a transaction which invokes metrics#handleTransactionSubmitted and calls createTransactionEventFragmentTo solve this, I proposed two solutions (TLDR; went with Solution B):
Solution A)
Solution B)
transaction-submitted-<id>
fragment. Prior to this PR fragments only exist once they are created.transaction-submitted-<id>
fragmenttransaction-submitted-${transactionId}
, params)" to store simulation metric propsSolution C)
I went with solution B due to time constraints.
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3507
Manual testing steps
Transaction Submitted
andTransaction Finalized
eventsScreenshots/Recordings
no UI/UX changes
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist