Skip to content
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

fix: Fix transaction metric event asset type detection #24770

Merged
merged 2 commits into from
May 28, 2024

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented May 24, 2024

Description

We collect metric data for each phase of a transaction's lifecycle. During one phase, we try to detect the type of asset being transacted. This step was broken due to a missing await, so the asset type was never identified. Additionally, the missing await meant that errors were not being handled correctly, resulting in many junk Sentry error events.

This seems to have been broken as part of a TypeScript refactor, in #23445, perhaps due to a pre-existing invalid type for this function that was added in #21330

Open in GitHub Codespaces

Related issues

Fixes #18300

Manual testing steps

TODO

I haven't reproduced any actual bug/missing metrics data from this yet, but presumably we should see that and be able to test this. I discovered this by looking into the Sentry error volume side-effect.

Screenshots/Recordings

N/A

Pre-merge author checklist

  • I’ve followed MetaMask 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.

We collect metric data for each phase of a transaction's lifecycle.
During one phase, we try to detect the type of asset being transacted.
This step was broken due to a missing `await`, so the asset type was
never identified. Additionally, the missing `await` meant that errors
were not being handled correctly, resulting in many junk Sentry error
events.

Fixes #18300
@metamaskbot
Copy link
Collaborator

Builds ready [aaf416c]
Page Load Metrics (916 ± 562 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint68161902412
domContentLoaded96515136
load5631899161171562
domInteractive96515136
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 6 Bytes (0.00%)

@dbrans dbrans marked this pull request as ready for review May 28, 2024 19:54
@dbrans dbrans requested a review from a team as a code owner May 28, 2024 19:54
dbrans
dbrans previously approved these changes May 28, 2024
Copy link
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.

@dbrans dbrans merged commit c1bba92 into develop May 28, 2024
69 of 72 checks passed
@dbrans dbrans deleted the fix-broken-transaction-asset-type-detection branch May 28, 2024 20:23
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [4bb8e75]
Page Load Metrics (661 ± 490 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6815294189
domContentLoaded9391373
load5724936611021490
domInteractive9381373
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 6 Bytes (0.00%)

@metamaskbot metamaskbot added release-11.16.1 Issue or pull request that will be included in release 11.16.1 and removed release-11.18.0 labels May 29, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.16.1 on PR. Adding release label release-11.16.1 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.1.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.16.1 Issue or pull request that will be included in release 11.16.1 team-extension-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants