Skip to content

CHECKOUT-6524: Fix Sentry client initialization to avoid conflict #1610

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

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

davidchin
Copy link
Contributor

@davidchin davidchin commented Dec 7, 2023

What?

  • Create our own BrowserClient and Hub to avoid conflicting with the global BrowserClient and Hub provided by Sentry Browser SDK.
  • Relax the error filtering rule to report errors as long as some of the frames in the stack trace are thrown from the app code.

Why?

  • Some of our merchants also use Sentry and want to log front-end errors against their account. We want to ensure that our Sentry client doesn't interfere with theirs. Otherwise, errors will only get logged against one of the accounts, depending on their initialisation order. The way to solve this problem is by creating a separate client instance and using it directly. See https://docs.sentry.io/platforms/javascript/troubleshooting/#using-a-client-directly).
  • Not all error frames come from the application code. For example, some are native code so they don't have a filename. Currently, if one of the error frames in the stack trace doesn't have a filename or a filename that doesn't match with the asset host, the error doesn't get reported. To fix this issue, we need to relax the filtering rule.

Testing / Proof

Screenshot 2023-12-08 at 12 32 35 pm

@bigcommerce/team-checkout

@davidchin davidchin requested a review from a team as a code owner December 7, 2023 04:16
});
};

this.hub = createHub(clientOptions as BrowserClientOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a typeguard here and avoid the as keyword please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good spot, I made some tweaks to the type so I no longer need to use as. 🙂

@davidchin davidchin force-pushed the fix_sentry_client branch 4 times, most recently from 5d50dcd to 19fb467 Compare December 8, 2023 05:13
Copy link
Contributor

@snaderiBC snaderiBC left a comment

Choose a reason for hiding this comment

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

Thanks for the reference to the documentation 👍

@davidchin davidchin merged commit 8abbec2 into master Dec 11, 2023
@davidchin davidchin deleted the fix_sentry_client branch December 11, 2023 23:29
stackParser: defaultStackParser,
};

this.hub = createHub(clientOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @bigcommerce/team-checkout , I'm going to revert this PR because this line is not working in production and stopping Sentry from logging any issue.

An example of a production error message:

Refused to create a worker from 'blob:https://checkout.solostove.com/5837bd07-3018-47d1-8af1-50e16ed86d04' because it violates the following Content Security Policy directive: "script-src 'none'". Note that 'worker-src' was not explicitly set, so 'script-src' is used as a fallback.
Screenshot 2023-12-27 at 11 29 13 am

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After more thorough testing and research, I found out that the proposed solution won’t work. Although Sentry SDK allows multiple client instances for different accounts, many of its features (a.k.a. integrations) only works with the primary client. For example, only the primary client includes breadcrumbs in its error logs, and reports uncaught exceptions. It also won’t work if other client instances are constructed using a different SDK version. This is because all of the client instances actually share the same underlying __SENTRY__ global variable. Several issues have been reported regarding the limitation of running multiple client instances (e.g.: getsentry/sentry-javascript#3271, getsentry/sentry-javascript#3268, getsentry/sentry-javascript#2732 etc…). Until Sentry SDK has a better support for multiple client instances, I think the only way around this problem is to recommend merchants to not to initialise their own client instance on the checkout page if they are using the default OOPC.

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.

4 participants