From 88bc0a7f2888ebfe6fe2430786dcec74f31a3495 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 21 Jun 2023 17:05:33 +0200 Subject: [PATCH] feat: Better event name handling for non-Error objects --- .../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 | 21 ++++++++++++ .../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 | 32 +++++++++++++++++-- .../test/integration/suites/onerror.js | 4 +-- .../suites/onunhandledrejection.js | 6 ++-- .../browser/test/unit/eventbuilder.test.ts | 27 ++++++++++++++++ .../browser/tsconfig.test-integration.json | 12 +++++++ packages/browser/tsconfig.test.json | 5 +-- packages/utils/src/is.ts | 2 +- 22 files changed, 186 insertions(+), 12 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..6aae7660d394 --- /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: '`MyTestClass` 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..9c09ba374e78 --- /dev/null +++ b/packages/browser-integration-tests/suites/public-api/captureException/errorEvent/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 ErrorEvent', 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: '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..7981f0608cb0 --- /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` (custom) captured as exception with keys: currentTarget, isTrusted, target, type', + 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..10da3b7cd854 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,29 @@ export function eventFromString( return event; } + +function getNonErrorObjectExceptionValue( + exception: Record, + { 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}`; +} 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..cb1aec9dae41 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` (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); @@ -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..1a8478036534 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,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][])( + '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..2e6d4eee46e8 --- /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..ca0255dbfb07 100644 --- a/packages/browser/tsconfig.test.json +++ b/packages/browser/tsconfig.test.json @@ -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 } diff --git a/packages/utils/src/is.ts b/packages/utils/src/is.ts index 350826cb567c..b787ce7f607a 100644 --- a/packages/utils/src/is.ts +++ b/packages/utils/src/is.ts @@ -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'); }