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

feat: Add performance tracing infrastructure #11132

Merged
merged 21 commits into from
Sep 18, 2024

Conversation

OGPoyraz
Copy link
Member

@OGPoyraz OGPoyraz commented Sep 10, 2024

Description

  • Add initial tracing and performance metrics infrastructure.
    Add the trace, endTrace methods, to be used anywhere in the mobile code to generate a Sentry traces. Implementation is almost same as we have in the extension. The only difference is in extension withIsolatedScope, in mobile withScope is used because @sentry/browser version mismatch in @sentry/react-native. This is also noted in the trace.ts file.
  • Add developer options to settings (similar settings added in extension )
    This adds an easily accessible development action menu where devs wants to test some functionality locally. Specifically this PR adds generating nested traces in Sentry.
  • New environment variables:
    MM_ENABLE_SETTINGS_PAGE_DEV_OPTIONS : Needs to be set to true in order to show Developer Options in the settings page
    MM_SENTRY_DSN_DEV : This is explicitly set for test-metamask project and the expected value is in the .js.env.example

Related issues

Fixes: https://github.com/MetaMask/mobile-planning/issues/1877

Manual testing steps

  1. Go to settings page tab Developer Options
  2. Tab Generate Trace Test
  3. Open https://metamask.sentry.io/traces/?project=273496&statsPeriod=15m, in this link you will be able to see traces that have been send to test-metamask in last 15 minutes
  4. See Developer Test trace is triggered

Screenshots/Recordings

Before

After

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.

@OGPoyraz OGPoyraz added No QA Needed Apply this label when your PR does not need any QA effort. team-confirmations Push issues to confirmations team labels Sep 10, 2024
@OGPoyraz OGPoyraz requested a review from a team as a code owner September 10, 2024 10:47
@OGPoyraz OGPoyraz requested review from a team as code owners September 10, 2024 11:00
Copy link

socket-security bot commented Sep 10, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@sentry/types@7.119.0 None 0 314 kB sentry-bot

🚮 Removed packages: npm/@sentry/types@6.3.1

View full report↗︎

matthewwalsh0
matthewwalsh0 previously approved these changes Sep 10, 2024
app/components/Nav/Main/MainNavigator.js Show resolved Hide resolved
app/util/sentry/trace.ts Outdated Show resolved Hide resolved
app/util/sentry/trace.ts Outdated Show resolved Hide resolved
app/util/sentry/trace.ts Outdated Show resolved Hide resolved
app/util/sentry/trace.ts Outdated Show resolved Hide resolved
app/util/sentry/utils.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Left some comments

.js.env.example Outdated Show resolved Hide resolved
app/components/Nav/Main/MainNavigator.js Outdated Show resolved Hide resolved
app/components/Views/Settings/DeveloperOptions/index.tsx Outdated Show resolved Hide resolved
app/util/sentry/utils.js Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 82.24299% with 19 lines in your changes missing coverage. Please review.

Project coverage is 55.23%. Comparing base (8dcdd3c) to head (9ed584d).
Report is 71 commits behind head on main.

Files with missing lines Patch % Lines
app/util/trace.ts 90.66% 7 Missing ⚠️
...nts/Views/Settings/DeveloperOptions/SentryTest.tsx 64.28% 5 Missing ⚠️
app/util/sentry/utils.js 0.00% 5 Missing ⚠️
app/components/Views/Settings/index.tsx 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11132      +/-   ##
==========================================
+ Coverage   52.75%   55.23%   +2.47%     
==========================================
  Files        1534     1558      +24     
  Lines       36777    37227     +450     
  Branches     4335     4414      +79     
==========================================
+ Hits        19403    20561    +1158     
- Misses      16058    16194     +136     
+ Partials     1316      472     -844     

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

@OGPoyraz OGPoyraz force-pushed the 1877-add-performance-metric-infrastructure-to-mobile branch from 3481ff3 to 5e8c448 Compare September 12, 2024 06:47
@OGPoyraz OGPoyraz requested a review from a team as a code owner September 13, 2024 05:40
@OGPoyraz OGPoyraz force-pushed the 1877-add-performance-metric-infrastructure-to-mobile branch from b1ba884 to 947a51f Compare September 13, 2024 05:41
@OGPoyraz OGPoyraz added the Run Smoke E2E Triggers smoke e2e on Bitrise label Sep 13, 2024
Copy link
Contributor

github-actions bot commented Sep 13, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 3cc85dc
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c4d4093c-8d44-4a41-ac3d-30731525307e

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Cal-L
Cal-L previously approved these changes Sep 13, 2024
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Nice, LGTM. Feel free to reach out for another approval once the conflicts are fixed.

app/util/trace.ts Outdated Show resolved Hide resolved
app/util/trace.ts Outdated Show resolved Hide resolved
@OGPoyraz OGPoyraz added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Sep 16, 2024
Copy link
Contributor

github-actions bot commented Sep 16, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 6b55a47
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/663f81d5-dcc4-46d4-b7b5-0e10142b8bb9

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

Copy link
Member

@leotm leotm left a comment

Choose a reason for hiding this comment

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

lgtm

Screenshot 2024-09-18 at 6 52 31 PM

perhaps like https://github.com/MetaMask/metamask-extension/blob/develop/development/README.md#sentry

we should add a Sentry section here https://github.com/MetaMask/metamask-mobile/blob/main/docs/readme/testing.md

with heading e.g. Generating a trace test that mentions our new env vars, with the odd screenshot or two if it helps

@sethkfman sethkfman added this pull request to the merge queue Sep 18, 2024
Merged via the queue into main with commit bba430a Sep 18, 2024
42 checks passed
@sethkfman sethkfman deleted the 1877-add-performance-metric-infrastructure-to-mobile branch September 18, 2024 18:44
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2024
@metamaskbot metamaskbot added the release-7.32.0 Issue or pull request that will be included in release 7.32.0 label Sep 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
No QA Needed Apply this label when your PR does not need any QA effort. release-7.32.0 Issue or pull request that will be included in release 7.32.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-confirmations Push issues to confirmations team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants