Skip to content

Commit

Permalink
feat: Better event name handling for non-Error objects
Browse files Browse the repository at this point in the history
  • Loading branch information
mydea committed Jun 21, 2023
1 parent 4fb043e commit 88bc0a7
Show file tree
Hide file tree
Showing 22 changed files with 186 additions and 12 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: '`MyTestClass` 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://[email protected]/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,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 ErrorEvent', 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: '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` (custom) captured as exception with keys: currentTarget, isTrusted, target, type',
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,
},
});
});
32 changes: 29 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,29 @@ 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';
const prototype: Prototype | null = Object.getPrototypeOf(exception);
const className = prototype ? prototype.constructor.name : undefined;

// 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}\``;
}

const name = isEvent(exception)
? `Event \`${className}\` (${exception.type})`
: className && className !== 'Object'
? `\`${className}\``
: 'Object';

const label = `${name} captured as ${captureType}`;

return `${label} with keys: ${keys}`;
}
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` (unhandledrejection) captured as promise rejection with keys: currentTarget, isTrusted, target, type'
);
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
27 changes: 27 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,26 @@ 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(), '`MyTestClass` captured as exception with keys: prop1, prop2'],
[
'Event',
new Event('custom'),
'Event `Event` (custom) captured as exception with keys: currentTarget, isTrusted, target, type',
],
[
'MouseEvent',
new MouseEvent('click'),
'Event `MouseEvent` (click) captured as exception with keys: currentTarget, isTrusted, target, type',
],
] 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
}
}
5 changes: 3 additions & 2 deletions packages/browser/tsconfig.test.json
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
{
"extends": "./tsconfig.json",

"include": ["test/**/*"],
"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
2 changes: 1 addition & 1 deletion packages/utils/src/is.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ function isBuiltin(wat: unknown, className: string): boolean {
* @param wat A value to be checked.
* @returns A boolean representing the result.
*/
export function isErrorEvent(wat: unknown): boolean {
export function isErrorEvent(wat: unknown): wat is ErrorEvent {
return isBuiltin(wat, 'ErrorEvent');
}

Expand Down

0 comments on commit 88bc0a7

Please sign in to comment.