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 some authentication state to sentry logs #29432

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Jan 6, 2025

Description

This adds some authentication controller state to sentry logs. We do not log sensitive and important info such as the accessToken.

This should help with logging some of the service failures that rely on authentication.

Open in GitHub Codespaces

Related issues

Fixes: N/A

Manual testing steps

  1. Follow the steps in the development readme to invoke a sentry error
    • DEBUG=metamask:sentry:* and METAMASK_DEBUG=1 can help to show the sentry state we are sending.

Screenshots/Recordings

Before

Screenshot 2025-01-06 at 09 04 48

After

Screenshot 2025-01-06 at 11 28 23

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.

this will help for logging some service failures that rely on authentication.
We do not log sensitive and important info such as the access token
Copy link
Contributor

github-actions bot commented Jan 6, 2025

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.

@metamaskbot metamaskbot added the team-notifications Notifications team label Jan 6, 2025
Comment on lines +45 to +49
sessionData: {
profile: true,
accessToken: false,
expiresIn: true,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will expose some useful information when inspecting sentry errors related to authenticated services. Exposes:

profile.profileId - the uuid for a given profile
profile.identifierId - a unique hash from what a user used to sign in with
expiresIn - a string that you can parse with `Date.parse()`. Can be useful if to determine if an access token expired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Image to show what we are exposing.

Screenshot 2025-01-06 at 11 28 23

@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review January 6, 2025 11:21
@metamaskbot
Copy link
Collaborator

Builds ready [7a7aed2]
Page Load Metrics (1906 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint34921891831372179
domContentLoaded15672160187514971
load15792186190614771
domInteractive27143553015
backgroundConnect1073332110
firstReactRender1896452713
getState568232010
initialActions01000
loadScripts10951616138513364
setupStore782262612
uiStartup186926542219210101

@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Jan 6, 2025
Merged via the queue into main with commit 4ee7e3f Jan 6, 2025
94 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the feat/sentry-logs-authentication-state branch January 6, 2025 13:58
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2025
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Jan 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-notifications Notifications team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants