Skip to content

Commit

Permalink
feat(browser): Better event name handling for non-Error objects (#8374)
Browse files Browse the repository at this point in the history
This PR adjusts the exception name generation for non-error objects in
the browser SDK.

ref #7941
  • Loading branch information
mydea authored Jun 23, 2023
1 parent 00641e2 commit 5217485
Show file tree
Hide file tree
Showing 21 changed files with 184 additions and 10 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class MyTestClass {
prop1 = 'value1';
prop2 = 2;
}

Sentry.captureException(new MyTestClass());
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';

sentryTest('should capture an POJO', async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });

const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);

expect(eventData.exception?.values).toHaveLength(1);
expect(eventData.exception?.values?.[0]).toMatchObject({
type: 'Error',
value: 'Object captured as exception with keys: prop1, prop2',
mechanism: {
type: 'generic',
handled: true,
},
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ sentryTest('should capture an empty object', async ({ getLocalTestPath, page })
expect(eventData.exception?.values).toHaveLength(1);
expect(eventData.exception?.values?.[0]).toMatchObject({
type: 'Error',
value: 'Non-Error exception captured with keys: [object has no keys]',
value: 'Object captured as exception with keys: [object has no keys]',
mechanism: {
type: 'generic',
handled: true,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
defaultIntegrations: false,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
window.addEventListener('error', function (event) {
Sentry.captureException(event);
});

window.thisDoesNotExist();
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';

sentryTest('should capture an ErrorEvent', async ({ getLocalTestPath, page, browserName }) => {
// On Firefox, the ErrorEvent has the `error` property and thus is handled separately
if (browserName === 'firefox') {
sentryTest.skip();
}
const url = await getLocalTestPath({ testDir: __dirname });

const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);

expect(eventData.exception?.values).toHaveLength(1);
expect(eventData.exception?.values?.[0]).toMatchObject({
type: 'ErrorEvent',
value: 'Event `ErrorEvent` captured as exception with message `Script error.`',
mechanism: {
type: 'generic',
handled: true,
},
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Sentry.captureException(new Event('custom'));
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';

sentryTest('should capture an Event', async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });

const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);

expect(eventData.exception?.values).toHaveLength(1);
expect(eventData.exception?.values?.[0]).toMatchObject({
type: 'Event',
value: 'Event `Event` (type=custom) captured as exception',
mechanism: {
type: 'generic',
handled: true,
},
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Sentry.captureException({
prop1: 'value1',
prop2: 2,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';

sentryTest('should capture an class instance', async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });

const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);

expect(eventData.exception?.values).toHaveLength(1);
expect(eventData.exception?.values?.[0]).toMatchObject({
type: 'Error',
value: 'Object captured as exception with keys: prop1, prop2',
mechanism: {
type: 'generic',
handled: true,
},
});
});
36 changes: 33 additions & 3 deletions packages/browser/src/eventbuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import {
resolvedSyncPromise,
} from '@sentry/utils';

type Prototype = { constructor: (...args: unknown[]) => unknown };

/**
* This function creates an exception from a JavaScript Error
*/
Expand Down Expand Up @@ -55,9 +57,7 @@ export function eventFromPlainObject(
values: [
{
type: isEvent(exception) ? exception.constructor.name : isUnhandledRejection ? 'UnhandledRejection' : 'Error',
value: `Non-Error ${
isUnhandledRejection ? 'promise rejection' : 'exception'
} captured with keys: ${extractExceptionKeysForMessage(exception)}`,
value: getNonErrorObjectExceptionValue(exception, { isUnhandledRejection }),
},
],
},
Expand Down Expand Up @@ -283,3 +283,33 @@ export function eventFromString(

return event;
}

function getNonErrorObjectExceptionValue(
exception: Record<string, unknown>,
{ isUnhandledRejection }: { isUnhandledRejection?: boolean },
): string {
const keys = extractExceptionKeysForMessage(exception);
const captureType = isUnhandledRejection ? 'promise rejection' : 'exception';

// Some ErrorEvent instances do not have an `error` property, which is why they are not handled before
// We still want to try to get a decent message for these cases
if (isErrorEvent(exception)) {
return `Event \`ErrorEvent\` captured as ${captureType} with message \`${exception.message}\``;
}

if (isEvent(exception)) {
const className = getObjectClassName(exception);
return `Event \`${className}\` (type=${exception.type}) captured as ${captureType}`;
}

return `Object captured as ${captureType} with keys: ${keys}`;
}

function getObjectClassName(obj: unknown): string | undefined | void {
try {
const prototype: Prototype | null = Object.getPrototypeOf(obj);
return prototype ? prototype.constructor.name : undefined;
} catch (e) {
// ignore errors here
}
}
4 changes: 2 additions & 2 deletions packages/browser/test/integration/suites/onerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ describe('window.onerror', function () {
} else {
assert.equal(
summary.events[0].exception.values[0].value,
'Non-Error exception captured with keys: error, somekey'
'Object captured as exception with keys: error, somekey'
);
}
assert.equal(summary.events[0].exception.values[0].stacktrace.frames.length, 1); // always 1 because thrown objects can't provide > 1 frame
Expand Down Expand Up @@ -119,7 +119,7 @@ describe('window.onerror', function () {
assert.equal(summary.events[0].exception.values[0].type, 'Error');
assert.equal(
summary.events[0].exception.values[0].value,
'Non-Error exception captured with keys: otherKey, type'
'Object captured as exception with keys: otherKey, type'
);
assert.deepEqual(summary.events[0].extra.__serialized__, {
type: 'error',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ describe('window.onunhandledrejection', function () {
// non-error rejections don't provide stacktraces so we can skip that assertion
assert.equal(
summary.events[0].exception.values[0].value,
'Non-Error promise rejection captured with keys: currentTarget, isTrusted, target, type'
'Event `Event` (type=unhandledrejection) captured as promise rejection'
);
assert.equal(summary.events[0].exception.values[0].type, 'Event');
assert.equal(summary.events[0].exception.values[0].mechanism.handled, false);
Expand Down Expand Up @@ -144,7 +144,7 @@ describe('window.onunhandledrejection', function () {
// non-error rejections don't provide stacktraces so we can skip that assertion
assert.equal(
summary.events[0].exception.values[0].value,
'Non-Error promise rejection captured with keys: a, b, c'
'Object captured as promise rejection with keys: a, b, c'
);
assert.equal(summary.events[0].exception.values[0].type, 'UnhandledRejection');
assert.equal(summary.events[0].exception.values[0].mechanism.handled, false);
Expand Down Expand Up @@ -172,7 +172,7 @@ describe('window.onunhandledrejection', function () {
// non-error rejections don't provide stacktraces so we can skip that assertion
assert.equal(
summary.events[0].exception.values[0].value,
'Non-Error promise rejection captured with keys: a, b, c, d, e'
'Object captured as promise rejection with keys: a, b, c, d, e'
);
assert.equal(summary.events[0].exception.values[0].type, 'UnhandledRejection');
assert.equal(summary.events[0].exception.values[0].mechanism.handled, false);
Expand Down
19 changes: 19 additions & 0 deletions packages/browser/test/unit/eventbuilder.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ jest.mock('@sentry/core', () => {
};
});

class MyTestClass {
prop1 = 'hello';
prop2 = 2;
}

afterEach(() => {
jest.resetAllMocks();
});
Expand Down Expand Up @@ -61,4 +66,18 @@ describe('eventFromPlainObject', () => {
},
});
});

it.each([
['empty object', {}, 'Object captured as exception with keys: [object has no keys]'],
['pojo', { prop1: 'hello', prop2: 2 }, 'Object captured as exception with keys: prop1, prop2'],
['Custom Class', new MyTestClass(), 'Object captured as exception with keys: prop1, prop2'],
['Event', new Event('custom'), 'Event `Event` (type=custom) captured as exception'],
['MouseEvent', new MouseEvent('click'), 'Event `MouseEvent` (type=click) captured as exception'],
] as [string, Record<string, unknown>, string][])(
'has correct exception value for %s',
(_name, exception, expected) => {
const actual = eventFromPlainObject(defaultStackParser, exception);
expect(actual.exception?.values?.[0]?.value).toEqual(expected);
},
);
});
12 changes: 12 additions & 0 deletions packages/browser/tsconfig.test-integration.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"extends": "./tsconfig.json",

"include": ["test/integration/**/*"],

"compilerOptions": {
// should include all types from `./tsconfig.json` plus types for all test frameworks used
"types": ["node", "mocha", "chai", "sinon"]

// other package-specific, test-specific options
}
}
3 changes: 2 additions & 1 deletion packages/browser/tsconfig.test.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@
"extends": "./tsconfig.json",

"include": ["test/**/*"],
"exclude": ["test/integration/**/*"],

"compilerOptions": {
// should include all types from `./tsconfig.json` plus types for all test frameworks used
"types": ["node", "mocha", "chai", "sinon", "jest"]
"types": ["node", "jest"]

// other package-specific, test-specific options
}
Expand Down

0 comments on commit 5217485

Please sign in to comment.