From 52174851d7a8fe612268302c6bb8bdf44a31de44 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 23 Jun 2023 14:13:22 +0200 Subject: [PATCH] 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 https://github.com/getsentry/sentry-javascript/issues/7941 --- .../captureException/classInstance/subject.js | 6 ++++ .../captureException/classInstance/test.ts | 21 +++++++++++ .../{empty_obj => emptyObj}/subject.js | 0 .../{empty_obj => emptyObj}/test.ts | 2 +- .../captureException/errorEvent/init.js | 8 +++++ .../captureException/errorEvent/subject.js | 5 +++ .../captureException/errorEvent/test.ts | 25 +++++++++++++ .../captureException/event/subject.js | 1 + .../public-api/captureException/event/test.ts | 21 +++++++++++ .../captureException/plainObject/subject.js | 4 +++ .../captureException/plainObject/test.ts | 21 +++++++++++ .../{simple_error => simpleError}/subject.js | 0 .../{simple_error => simpleError}/test.ts | 0 .../subject.js | 0 .../{undefined_arg => undefinedArg}/test.ts | 0 packages/browser/src/eventbuilder.ts | 36 +++++++++++++++++-- .../test/integration/suites/onerror.js | 4 +-- .../suites/onunhandledrejection.js | 6 ++-- .../browser/test/unit/eventbuilder.test.ts | 19 ++++++++++ .../browser/tsconfig.test-integration.json | 12 +++++++ packages/browser/tsconfig.test.json | 3 +- 21 files changed, 184 insertions(+), 10 deletions(-) create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/classInstance/subject.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/classInstance/test.ts rename packages/browser-integration-tests/suites/public-api/captureException/{empty_obj => emptyObj}/subject.js (100%) rename packages/browser-integration-tests/suites/public-api/captureException/{empty_obj => emptyObj}/test.ts (91%) create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/errorEvent/init.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/errorEvent/subject.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/errorEvent/test.ts create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/event/subject.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/event/test.ts create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/plainObject/subject.js create mode 100644 packages/browser-integration-tests/suites/public-api/captureException/plainObject/test.ts rename packages/browser-integration-tests/suites/public-api/captureException/{simple_error => simpleError}/subject.js (100%) rename packages/browser-integration-tests/suites/public-api/captureException/{simple_error => simpleError}/test.ts (100%) rename packages/browser-integration-tests/suites/public-api/captureException/{undefined_arg => undefinedArg}/subject.js (100%) rename packages/browser-integration-tests/suites/public-api/captureException/{undefined_arg => undefinedArg}/test.ts (100%) create mode 100644 packages/browser/tsconfig.test-integration.json diff --git a/packages/browser-integration-tests/suites/public-api/captureException/classInstance/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/classInstance/subject.js new file mode 100644 index 000000000000..d2d2b96a87fe --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/classInstance/subject.js @@ -0,0 +1,6 @@ +class MyTestClass { + prop1 = 'value1'; + prop2 = 2; +} + +Sentry.captureException(new MyTestClass()); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/classInstance/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/classInstance/test.ts new file mode 100644 index 000000000000..3a8865ec3672 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/classInstance/test.ts @@ -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(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, + }, + }); +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/empty_obj/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/emptyObj/subject.js similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/empty_obj/subject.js rename to packages/browser-integration-tests/suites/public-api/captureException/emptyObj/subject.js diff --git a/packages/browser-integration-tests/suites/public-api/captureException/empty_obj/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/emptyObj/test.ts similarity index 91% rename from packages/browser-integration-tests/suites/public-api/captureException/empty_obj/test.ts rename to packages/browser-integration-tests/suites/public-api/captureException/emptyObj/test.ts index 6ce86bfe7aeb..fa6b1dcb1562 100644 --- a/packages/browser-integration-tests/suites/public-api/captureException/empty_obj/test.ts +++ b/packages/browser-integration-tests/suites/public-api/captureException/emptyObj/test.ts @@ -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, diff --git a/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/init.js b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/init.js new file mode 100644 index 000000000000..3796a084234a --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/init.js @@ -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, +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/subject.js new file mode 100644 index 000000000000..207f9d1d58f6 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/subject.js @@ -0,0 +1,5 @@ +window.addEventListener('error', function (event) { + Sentry.captureException(event); +}); + +window.thisDoesNotExist(); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/test.ts new file mode 100644 index 000000000000..dbcaaf24a1cf --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/test.ts @@ -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(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, + }, + }); +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/event/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/event/subject.js new file mode 100644 index 000000000000..b5855af22829 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/event/subject.js @@ -0,0 +1 @@ +Sentry.captureException(new Event('custom')); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/event/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/event/test.ts new file mode 100644 index 000000000000..65c46a776731 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/event/test.ts @@ -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(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, + }, + }); +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/plainObject/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/plainObject/subject.js new file mode 100644 index 000000000000..ea827971bed4 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/plainObject/subject.js @@ -0,0 +1,4 @@ +Sentry.captureException({ + prop1: 'value1', + prop2: 2, +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/plainObject/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/plainObject/test.ts new file mode 100644 index 000000000000..e81fe0125906 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/plainObject/test.ts @@ -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(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, + }, + }); +}); diff --git a/packages/browser-integration-tests/suites/public-api/captureException/simple_error/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/simpleError/subject.js similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/simple_error/subject.js rename to packages/browser-integration-tests/suites/public-api/captureException/simpleError/subject.js diff --git a/packages/browser-integration-tests/suites/public-api/captureException/simple_error/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/simpleError/test.ts similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/simple_error/test.ts rename to packages/browser-integration-tests/suites/public-api/captureException/simpleError/test.ts diff --git a/packages/browser-integration-tests/suites/public-api/captureException/undefined_arg/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/subject.js similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/undefined_arg/subject.js rename to packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/subject.js diff --git a/packages/browser-integration-tests/suites/public-api/captureException/undefined_arg/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/test.ts similarity index 100% rename from packages/browser-integration-tests/suites/public-api/captureException/undefined_arg/test.ts rename to packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/test.ts diff --git a/packages/browser/src/eventbuilder.ts b/packages/browser/src/eventbuilder.ts index 4db58b9b7b43..6acc1080c56f 100644 --- a/packages/browser/src/eventbuilder.ts +++ b/packages/browser/src/eventbuilder.ts @@ -14,6 +14,8 @@ import { resolvedSyncPromise, } from '@sentry/utils'; +type Prototype = { constructor: (...args: unknown[]) => unknown }; + /** * This function creates an exception from a JavaScript Error */ @@ -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 }), }, ], }, @@ -283,3 +283,33 @@ export function eventFromString( return event; } + +function getNonErrorObjectExceptionValue( + exception: Record, + { 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 + } +} diff --git a/packages/browser/test/integration/suites/onerror.js b/packages/browser/test/integration/suites/onerror.js index d5a7f155d723..3db1a755061a 100644 --- a/packages/browser/test/integration/suites/onerror.js +++ b/packages/browser/test/integration/suites/onerror.js @@ -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 @@ -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', diff --git a/packages/browser/test/integration/suites/onunhandledrejection.js b/packages/browser/test/integration/suites/onunhandledrejection.js index d32708eb88cd..f9095d7c7333 100644 --- a/packages/browser/test/integration/suites/onunhandledrejection.js +++ b/packages/browser/test/integration/suites/onunhandledrejection.js @@ -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); @@ -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); @@ -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); diff --git a/packages/browser/test/unit/eventbuilder.test.ts b/packages/browser/test/unit/eventbuilder.test.ts index ac9b564e99e0..d7a2ab712959 100644 --- a/packages/browser/test/unit/eventbuilder.test.ts +++ b/packages/browser/test/unit/eventbuilder.test.ts @@ -23,6 +23,11 @@ jest.mock('@sentry/core', () => { }; }); +class MyTestClass { + prop1 = 'hello'; + prop2 = 2; +} + afterEach(() => { jest.resetAllMocks(); }); @@ -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][])( + 'has correct exception value for %s', + (_name, exception, expected) => { + const actual = eventFromPlainObject(defaultStackParser, exception); + expect(actual.exception?.values?.[0]?.value).toEqual(expected); + }, + ); }); diff --git a/packages/browser/tsconfig.test-integration.json b/packages/browser/tsconfig.test-integration.json new file mode 100644 index 000000000000..06a946007c8b --- /dev/null +++ b/packages/browser/tsconfig.test-integration.json @@ -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 + } +} diff --git a/packages/browser/tsconfig.test.json b/packages/browser/tsconfig.test.json index 03bd27bbfdda..9bdd2aa76dab 100644 --- a/packages/browser/tsconfig.test.json +++ b/packages/browser/tsconfig.test.json @@ -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 }