feat: Better event name handling for non-Error objects

mydea · mydea · commit 0e6cf143c767 · 2023-06-22T10:38:40.000+02:00
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
@@ -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
@@ -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,
+    },
+  });
+});
diff --git a/packages/browser-integration-tests/suites/public-api/captureException/emptyObj/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/emptyObj/subject.js
diff --git a/packages/browser-integration-tests/suites/public-api/captureException/emptyObj/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
@@ -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
@@ -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
@@ -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,
+    },
+  });
+});
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
@@ -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
@@ -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,
+    },
+  });
+});
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
@@ -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
@@ -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,
+    },
+  });
+});
diff --git a/packages/browser-integration-tests/suites/public-api/captureException/simpleError/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/simpleError/subject.js
diff --git a/packages/browser-integration-tests/suites/public-api/captureException/simpleError/test.ts b/packages/browser-integration-tests/suites/public-api/captureException/simpleError/test.ts
diff --git a/packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/subject.js b/packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/subject.js
diff --git a/packages/browser-integration-tests/suites/public-api/captureException/undefinedArg/test.ts b/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
@@ -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<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}`;
+}
diff --git 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
@@ -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
@@ -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, unknown>, 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
@@ -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
@@ -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
@@ -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');
 }
 

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Sentry.captureException(new Event('custom'));`