Skip to content

feat(checkout): CHECKOUT-7036 Filter out convertcart-related issues on Sentry. #1092

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.

Sign up for GitHub

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
Nov 10, 2022
Merged

feat(checkout): CHECKOUT-7036 Filter out convertcart-related issues on Sentry. #1092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c1ae3d
feat(checkout): CHECKOUT-0000 Filter out convertcart-related issues o…
bc-peng Nov 8, 2022
ddba6d0
feat(checkout): CHECKOUT-7036 Update denyUrls
bc-peng Nov 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 63 additions & 1 deletion packages/core/src/app/common/error/SentryErrorLogger.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,68 @@ describe('SentryErrorLogger', () => {
expect(clientOptions.sampleRate).toBe(0.123);
});

it('does not log exception event if it relates to convertcart', () => {
new SentryErrorLogger(config);

const clientOptions: BrowserOptions = (init as jest.Mock).mock.calls[0][0];
/* eslint-disable @typescript-eslint/naming-convention */
const event = {
breadcrumbs: [
{
timestamp: 1667952544.208,
category: 'fetch',
data: {
method: 'POST',
url: 'https://dc4.convertcart.com/event/v3/94892041/123.123',
status_code: 200,
},
type: 'http',
},
{
timestamp: 1667952544.549,
category: 'fetch',
data: {
method: 'GET',
url: '/api/storefront/checkouts/abcdefg',
status_code: 200,
},
type: 'http',
},
],
exception: {
values: [
{
type: 'TypeError',
value: "Cannot read properties of null (reading 'setAttribute')",
stacktrace: {
frames: [
{
filename: 'app:///608-12345.js',
function: 'u',
in_app: true,
lineno: 1,
colno: 10602,
},
],
},
mechanism: {
type: 'instrument',
handled: true,
data: {
function: 'setInterval',
},
},
},
],
},
};
/* eslint-enable @typescript-eslint/naming-convention */
const hint = { originalException: new Error('Unexpected error') };

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
expect(clientOptions.beforeSend!(event, hint)).toBeNull();
});

it('does not log exception event if it does not contain stacktrace', () => {
new SentryErrorLogger(config);

Expand Down Expand Up @@ -169,7 +231,7 @@ describe('SentryErrorLogger', () => {

expect(init).toHaveBeenCalledWith(
expect.objectContaining({
denyUrls: ['polyfill~checkout', 'sentry~checkout'],
denyUrls: ['polyfill~checkout', 'sentry~checkout', 'convertcart'],
}),
);
});
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/app/common/error/SentryErrorLogger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export default class SentryErrorLogger implements ErrorLogger {
init({
sampleRate: SAMPLE_RATE,
beforeSend: this.handleBeforeSend,
denyUrls: [...(config.denyUrls || []), 'polyfill~checkout', 'sentry~checkout'],
denyUrls: [...(config.denyUrls || []), 'polyfill~checkout', 'sentry~checkout', 'convertcart'],
integrations: [
new Integrations.GlobalHandlers({
onerror: false,
Expand Down Expand Up @@ -135,6 +135,12 @@ export default class SentryErrorLogger implements ErrorLogger {
}

private handleBeforeSend: (event: Event, hint?: EventHint) => Event | null = (event, hint) => {
if (
event.breadcrumbs?.filter((breadcrumb) => breadcrumb.data?.url.includes('convertcart'))
Copy link
Contributor

@animesh1987 animesh1987 Nov 9, 2022

Choose a reason for hiding this comment

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

Do you for see us having a list of works like convertcart that we need to ignore in future?

Copy link
Contributor Author

@bc-peng bc-peng Nov 9, 2022

Choose a reason for hiding this comment

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

For other issues, we might have to use other signatures to identify them.
Right now, I think we just aim to remove convertcart.

Copy link
Contributor

@snaderiBC snaderiBC Nov 9, 2022

Choose a reason for hiding this comment

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

Is it possible to use and take advantage of https://docs.sentry.io/platforms/javascript/guides/capacitor/configuration/options/#deny-urls

Copy link
Contributor Author

@bc-peng bc-peng Nov 9, 2022

Choose a reason for hiding this comment

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

Yesterday, the same Sentry issue's URL was https://www.extremekartz.com/checkout, today it is https://mcneelamusic.com/checkout/order-confirmation.
Therefore, I think it is better to identify convertcart in breadcrumbs.

Copy link
Contributor

@animesh1987 animesh1987 Nov 9, 2022

Choose a reason for hiding this comment

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

Found this in github issues getsentry/sentry-javascript#3147, seems similar. It also links a PR for denyUrls.

Copy link
Contributor Author

@bc-peng bc-peng Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have added convertcart to denyUrls.
Perhaps we can keep the filter in beforeSend, as mentioned in getsentry/sentry-javascript#3147.

) {
return null;
}

if (event.exception) {
if (
!this.shouldReportExceptions(
Expand Down