Skip to content

Commit 5217485

Browse files
authored
feat(browser): Better event name handling for non-Error objects (#8374)
This PR adjusts the exception name generation for non-error objects in the browser SDK. ref #7941
1 parent 00641e2 commit 5217485

File tree

21 files changed

+184
-10
lines changed

21 files changed

+184
-10
lines changed
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
class MyTestClass {
2+
prop1 = 'value1';
3+
prop2 = 2;
4+
}
5+
6+
Sentry.captureException(new MyTestClass());
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import { expect } from '@playwright/test';
2+
import type { Event } from '@sentry/types';
3+
4+
import { sentryTest } from '../../../../utils/fixtures';
5+
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';
6+
7+
sentryTest('should capture an POJO', async ({ getLocalTestPath, page }) => {
8+
const url = await getLocalTestPath({ testDir: __dirname });
9+
10+
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
11+
12+
expect(eventData.exception?.values).toHaveLength(1);
13+
expect(eventData.exception?.values?.[0]).toMatchObject({
14+
type: 'Error',
15+
value: 'Object captured as exception with keys: prop1, prop2',
16+
mechanism: {
17+
type: 'generic',
18+
handled: true,
19+
},
20+
});
21+
});

packages/browser-integration-tests/suites/public-api/captureException/empty_obj/test.ts renamed to packages/browser-integration-tests/suites/public-api/captureException/emptyObj/test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ sentryTest('should capture an empty object', async ({ getLocalTestPath, page })
1212
expect(eventData.exception?.values).toHaveLength(1);
1313
expect(eventData.exception?.values?.[0]).toMatchObject({
1414
type: 'Error',
15-
value: 'Non-Error exception captured with keys: [object has no keys]',
15+
value: 'Object captured as exception with keys: [object has no keys]',
1616
mechanism: {
1717
type: 'generic',
1818
handled: true,
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
Sentry.init({
6+
dsn: 'https://[email protected]/1337',
7+
defaultIntegrations: false,
8+
});
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
window.addEventListener('error', function (event) {
2+
Sentry.captureException(event);
3+
});
4+
5+
window.thisDoesNotExist();
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import { expect } from '@playwright/test';
2+
import type { Event } from '@sentry/types';
3+
4+
import { sentryTest } from '../../../../utils/fixtures';
5+
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';
6+
7+
sentryTest('should capture an ErrorEvent', async ({ getLocalTestPath, page, browserName }) => {
8+
// On Firefox, the ErrorEvent has the `error` property and thus is handled separately
9+
if (browserName === 'firefox') {
10+
sentryTest.skip();
11+
}
12+
const url = await getLocalTestPath({ testDir: __dirname });
13+
14+
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
15+
16+
expect(eventData.exception?.values).toHaveLength(1);
17+
expect(eventData.exception?.values?.[0]).toMatchObject({
18+
type: 'ErrorEvent',
19+
value: 'Event `ErrorEvent` captured as exception with message `Script error.`',
20+
mechanism: {
21+
type: 'generic',
22+
handled: true,
23+
},
24+
});
25+
});
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Sentry.captureException(new Event('custom'));
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import { expect } from '@playwright/test';
2+
import type { Event } from '@sentry/types';
3+
4+
import { sentryTest } from '../../../../utils/fixtures';
5+
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';
6+
7+
sentryTest('should capture an Event', async ({ getLocalTestPath, page }) => {
8+
const url = await getLocalTestPath({ testDir: __dirname });
9+
10+
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
11+
12+
expect(eventData.exception?.values).toHaveLength(1);
13+
expect(eventData.exception?.values?.[0]).toMatchObject({
14+
type: 'Event',
15+
value: 'Event `Event` (type=custom) captured as exception',
16+
mechanism: {
17+
type: 'generic',
18+
handled: true,
19+
},
20+
});
21+
});
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Sentry.captureException({
2+
prop1: 'value1',
3+
prop2: 2,
4+
});
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import { expect } from '@playwright/test';
2+
import type { Event } from '@sentry/types';
3+
4+
import { sentryTest } from '../../../../utils/fixtures';
5+
import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers';
6+
7+
sentryTest('should capture an class instance', async ({ getLocalTestPath, page }) => {
8+
const url = await getLocalTestPath({ testDir: __dirname });
9+
10+
const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
11+
12+
expect(eventData.exception?.values).toHaveLength(1);
13+
expect(eventData.exception?.values?.[0]).toMatchObject({
14+
type: 'Error',
15+
value: 'Object captured as exception with keys: prop1, prop2',
16+
mechanism: {
17+
type: 'generic',
18+
handled: true,
19+
},
20+
});
21+
});

packages/browser/src/eventbuilder.ts

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import {
1414
resolvedSyncPromise,
1515
} from '@sentry/utils';
1616

17+
type Prototype = { constructor: (...args: unknown[]) => unknown };
18+
1719
/**
1820
* This function creates an exception from a JavaScript Error
1921
*/
@@ -55,9 +57,7 @@ export function eventFromPlainObject(
5557
values: [
5658
{
5759
type: isEvent(exception) ? exception.constructor.name : isUnhandledRejection ? 'UnhandledRejection' : 'Error',
58-
value: `Non-Error ${
59-
isUnhandledRejection ? 'promise rejection' : 'exception'
60-
} captured with keys: ${extractExceptionKeysForMessage(exception)}`,
60+
value: getNonErrorObjectExceptionValue(exception, { isUnhandledRejection }),
6161
},
6262
],
6363
},
@@ -283,3 +283,33 @@ export function eventFromString(
283283

284284
return event;
285285
}
286+
287+
function getNonErrorObjectExceptionValue(
288+
exception: Record<string, unknown>,
289+
{ isUnhandledRejection }: { isUnhandledRejection?: boolean },
290+
): string {
291+
const keys = extractExceptionKeysForMessage(exception);
292+
const captureType = isUnhandledRejection ? 'promise rejection' : 'exception';
293+
294+
// Some ErrorEvent instances do not have an `error` property, which is why they are not handled before
295+
// We still want to try to get a decent message for these cases
296+
if (isErrorEvent(exception)) {
297+
return `Event \`ErrorEvent\` captured as ${captureType} with message \`${exception.message}\``;
298+
}
299+
300+
if (isEvent(exception)) {
301+
const className = getObjectClassName(exception);
302+
return `Event \`${className}\` (type=${exception.type}) captured as ${captureType}`;
303+
}
304+
305+
return `Object captured as ${captureType} with keys: ${keys}`;
306+
}
307+
308+
function getObjectClassName(obj: unknown): string | undefined | void {
309+
try {
310+
const prototype: Prototype | null = Object.getPrototypeOf(obj);
311+
return prototype ? prototype.constructor.name : undefined;
312+
} catch (e) {
313+
// ignore errors here
314+
}
315+
}

packages/browser/test/integration/suites/onerror.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ describe('window.onerror', function () {
6161
} else {
6262
assert.equal(
6363
summary.events[0].exception.values[0].value,
64-
'Non-Error exception captured with keys: error, somekey'
64+
'Object captured as exception with keys: error, somekey'
6565
);
6666
}
6767
assert.equal(summary.events[0].exception.values[0].stacktrace.frames.length, 1); // always 1 because thrown objects can't provide > 1 frame
@@ -119,7 +119,7 @@ describe('window.onerror', function () {
119119
assert.equal(summary.events[0].exception.values[0].type, 'Error');
120120
assert.equal(
121121
summary.events[0].exception.values[0].value,
122-
'Non-Error exception captured with keys: otherKey, type'
122+
'Object captured as exception with keys: otherKey, type'
123123
);
124124
assert.deepEqual(summary.events[0].extra.__serialized__, {
125125
type: 'error',

packages/browser/test/integration/suites/onunhandledrejection.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ describe('window.onunhandledrejection', function () {
7777
// non-error rejections don't provide stacktraces so we can skip that assertion
7878
assert.equal(
7979
summary.events[0].exception.values[0].value,
80-
'Non-Error promise rejection captured with keys: currentTarget, isTrusted, target, type'
80+
'Event `Event` (type=unhandledrejection) captured as promise rejection'
8181
);
8282
assert.equal(summary.events[0].exception.values[0].type, 'Event');
8383
assert.equal(summary.events[0].exception.values[0].mechanism.handled, false);
@@ -144,7 +144,7 @@ describe('window.onunhandledrejection', function () {
144144
// non-error rejections don't provide stacktraces so we can skip that assertion
145145
assert.equal(
146146
summary.events[0].exception.values[0].value,
147-
'Non-Error promise rejection captured with keys: a, b, c'
147+
'Object captured as promise rejection with keys: a, b, c'
148148
);
149149
assert.equal(summary.events[0].exception.values[0].type, 'UnhandledRejection');
150150
assert.equal(summary.events[0].exception.values[0].mechanism.handled, false);
@@ -172,7 +172,7 @@ describe('window.onunhandledrejection', function () {
172172
// non-error rejections don't provide stacktraces so we can skip that assertion
173173
assert.equal(
174174
summary.events[0].exception.values[0].value,
175-
'Non-Error promise rejection captured with keys: a, b, c, d, e'
175+
'Object captured as promise rejection with keys: a, b, c, d, e'
176176
);
177177
assert.equal(summary.events[0].exception.values[0].type, 'UnhandledRejection');
178178
assert.equal(summary.events[0].exception.values[0].mechanism.handled, false);

packages/browser/test/unit/eventbuilder.test.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@ jest.mock('@sentry/core', () => {
2323
};
2424
});
2525

26+
class MyTestClass {
27+
prop1 = 'hello';
28+
prop2 = 2;
29+
}
30+
2631
afterEach(() => {
2732
jest.resetAllMocks();
2833
});
@@ -61,4 +66,18 @@ describe('eventFromPlainObject', () => {
6166
},
6267
});
6368
});
69+
70+
it.each([
71+
['empty object', {}, 'Object captured as exception with keys: [object has no keys]'],
72+
['pojo', { prop1: 'hello', prop2: 2 }, 'Object captured as exception with keys: prop1, prop2'],
73+
['Custom Class', new MyTestClass(), 'Object captured as exception with keys: prop1, prop2'],
74+
['Event', new Event('custom'), 'Event `Event` (type=custom) captured as exception'],
75+
['MouseEvent', new MouseEvent('click'), 'Event `MouseEvent` (type=click) captured as exception'],
76+
] as [string, Record<string, unknown>, string][])(
77+
'has correct exception value for %s',
78+
(_name, exception, expected) => {
79+
const actual = eventFromPlainObject(defaultStackParser, exception);
80+
expect(actual.exception?.values?.[0]?.value).toEqual(expected);
81+
},
82+
);
6483
});
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{
2+
"extends": "./tsconfig.json",
3+
4+
"include": ["test/integration/**/*"],
5+
6+
"compilerOptions": {
7+
// should include all types from `./tsconfig.json` plus types for all test frameworks used
8+
"types": ["node", "mocha", "chai", "sinon"]
9+
10+
// other package-specific, test-specific options
11+
}
12+
}

packages/browser/tsconfig.test.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,11 @@
22
"extends": "./tsconfig.json",
33

44
"include": ["test/**/*"],
5+
"exclude": ["test/integration/**/*"],
56

67
"compilerOptions": {
78
// should include all types from `./tsconfig.json` plus types for all test frameworks used
8-
"types": ["node", "mocha", "chai", "sinon", "jest"]
9+
"types": ["node", "jest"]
910

1011
// other package-specific, test-specific options
1112
}

0 commit comments

Comments
 (0)