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: enable Sentry in renderer process #397

Merged
merged 21 commits into from
Jan 15, 2025

Conversation

cristianoventura
Copy link
Collaborator

Description

This PR enables the Sentry integration in the renderer process, accounting for the user's preference to opt out of it. This PR is dependant on #393.

How to Test

  • Enable Sentry to run the development environment (both in main.ts and renderer.ts)
  • Manually throw an error in the React code
  • Run the app with the SENTRY_DSN env variable set (grab it from the project in Sentry) SENTRY_DSN=xx npm start
  • Reproduce the error in runtime and check Sentry

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (npm run lint) and all checks pass.
  • I have run tests locally (npm test) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Screenshots (if appropriate):

Related PR(s)/Issue(s)

Resolves #396

@cristianoventura cristianoventura self-assigned this Jan 7, 2025
src/renderer.ts Outdated
Comment on lines 39 to 45
integrations: [
Sentry.browserTracingIntegration(),
Sentry.replayIntegration(),
],
tracesSampleRate: 1.0,
replaysSessionSampleRate: 0.1,
replaysOnErrorSampleRate: 1.0,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Enabled some basic integrations such as browser tracing and session replay, which may help us debug issues when they arise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider filtering events to scrub sensitive data?

@cristianoventura cristianoventura marked this pull request as ready for review January 7, 2025 19:45
@cristianoventura cristianoventura requested a review from a team as a code owner January 7, 2025 19:45
@cristianoventura cristianoventura requested review from going-confetti and e-fisher and removed request for a team January 7, 2025 19:45
Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

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

Unhandled promise rejections are not reported to sentry, to reproduce it: add a throw to an async method, I've used validateAndSaveHarFile in views/Recorder/Recorder.tsx.

I've skimmed through docs and looks like sentry/react integration should handle this automatically if we add it additionally to sentry/electron, here's an example how to setup additional SDK: https://docs.sentry.io/platforms/javascript/guides/electron/#framework-specific-sdks

screencapture 2025-01-08 at 09 40 00

@going-confetti going-confetti requested review from Scutua and removed request for Scutua January 9, 2025 15:35
Base automatically changed from feat/crash-reporter-settings to main January 9, 2025 16:31
Copy link
Collaborator Author

@cristianoventura cristianoventura left a comment

Choose a reason for hiding this comment

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

@e-fisher Unfortunately @sentry/react didn't work and after spending some time trying to figure out why I learned the following.

The unhandled rejection gets raised inside the callback of the onBrowserClosed subscription, which is where validateAndSaveHarFile is being called.

useEffect(() => {
return window.studio.browser.onBrowserClosed(async () => {
const fileName = await validateAndSaveHarFile()

However, onBrowserClosed is declared in the preload context (not in the main renderer).

Electron creates two separate environments for the renderer process: the main renderer and the preload script. Sentry was only initialized in the renderer.ts file, which prevented it from capturing errors originating from functions or callbacks within the preload context.

The problem was solved by:

  • initializing Sentry in the preload and renderer contexts
  • adding an event listener for unhandledrejection to catch unhandled promise rejections


// conditionally send the event based on the user's settings
beforeSend: async (event) => {
const getSettings = getSettingsFn || window.studio.settings.getSettings
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

window isn't available in the preload context so I'm adding flexibility here to pass the getSettings function as an argument optionally.

e-fisher
e-fisher previously approved these changes Jan 13, 2025
Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

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

Looks like that did the trick! 🙌
I've noticed this error in the console when sentry is enabled, otherwise LGTM No longer reproducible

Refused to create a worker from 'blob:http://localhost:5173/45616690-949f-4daf-a53c-4fc7e0a2906f' because it violates the following Content Security Policy directive: "script-src 'self'". Note that 'worker-src' was not explicitly set, so 'script-src' is used as a fallback.

@cristianoventura
Copy link
Collaborator Author

I've removed the integrations as we've discussed today!

@cristianoventura cristianoventura merged commit 2850e61 into main Jan 15, 2025
3 checks passed
@cristianoventura cristianoventura deleted the feat/sentry-renderer-process branch January 15, 2025 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Sentry in the renderer process
3 participants