metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Fix sentry sessions

Open pedronfigueiredo opened this issue 1 year ago • 3 comments

Description

The session count in our Sentry dashboard has decreased drastically since v11.1.0.

It seems that the only time the session count is incremented is when using captureException and captureMessage methods. This explains why the session count is much lower than previously but not zero. It also explains a crash rate close to but different than 100% — because captureMessage is much less used than captureException on our repository.

Investigation Notes & Fix

The toggleSession() function call inside setParticipateInMetaMetrics() in the ui/store/actions.ts file is redundant, as both onboarding/metametrics.js (for onboarding) and ui/pages/settings/security-tab/security-tab.component.js (for the toggling flow) call setParticipateInMetaMetrics() on the MetaMetrics controller that starts or stops the session appropriately and updates the state directly. Moreover, by the time toggleSession() is called inside setParticipateInMetaMetrics() in the ui/store/actions.ts file, the participateInMetaMetrics property in the MetaMetrics controller state has not yet been updated. After removing that toggleSession() function call, we can see that the state is changed on both flows by using copy(await window.stateHooks.getCleanAppState()).

We also don’t need to use getMetaMetricsEnabled() for startSession() and endSession() anymore because setParticipateInMetaMetrics() is explicitly called with an argument that dictates whether to call the start session or end session methods. As such, I removed getMetaMetricsEnabled from the conditions on startSession and endSession.

Using Hub.captureSession and Hub.captureSession(true) on start and end session respectively yields the right behaviour for the https://sentry.io/api/<project_id>/envelope/?sentry_key=<project secret> requests. When starting a session the request payload includes "init": true and when ending a session, the request payload includes "init": false and a "duration" property.

Finally, the sessions are still not captured on the sentry dashboard, unless the @sentry/browser package is updated. This may be because we used the newer version for testing with sentry-example, and two different package versions on the same project may confuse the dashboard internals. Upgrading the package to the latest version fixes this.

Summary of changes:

  • Remove redundant toggleSession() function call from setParticipateInMetaMetrics in ui/store/actions.ts.
  • Remove toggleSession() function definition from app/scripts/lib/setupSentry.js because it’s not used anymore.
  • Remove usage of getMetaMetricsEnabled() inside startSession() and endSession() in app/scripts/lib/setupSentry.js because repeated calls for both methods are not possible.
  • Update sentry browser package version from ^7.53.0 to ^7.81.1 so the sessions are logged on the sentry dashboard.

Manual testing steps

  1. Create a new project on sentry as per the documentation

  2. Remove the extension completely

  3. Build the extension using yarn dist

  4. Install the extension from the dist folder

  5. Open the dev tools on the network tab for the UI and background processes.

  6. Onboard opting into MetaMetrics

  7. See the Sentry request with type “session” and “init” true.

  8. You should now see the sessions on Releases > Adoption.

  9. Reload the Extension on the Extension tab

  10. Repeat Steps 5-8

  11. Reload the Extension UI in the browser

  12. Repeat Steps 5-8

  13. When opting in and out of metametrics, Sentry session requests should be visible as well.

Pre-merge author checklist

  • [ ] I’ve followed MetaMask Coding Standards.
  • [ ] I've clearly explained what problem this PR is solving and how it is solved.
  • [ ] I've linked related issues
  • [ ] I've included manual testing steps
  • [ ] I've included screenshots/recordings if applicable
  • [ ] 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.
  • [ ] I’ve properly set the pull request status:
    • [ ] In case it's not yet "ready for review", I've set it to "draft".
    • [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

pedronfigueiredo avatar Nov 24 '23 12:11 pedronfigueiredo

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.

github-actions[bot] avatar Nov 24 '23 12:11 github-actions[bot]

@metamaskbot update-policies

pedronfigueiredo avatar Nov 30 '23 11:11 pedronfigueiredo

Policies updated

metamaskbot avatar Nov 30 '23 11:11 metamaskbot