Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(browser): Better event name handling for non-Error objects #8374

Merged
merged 5 commits into from
Jun 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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: 'Object 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]',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this should just read 'Object captured as exception'. Probably fine to save bundle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally wanted to do that as well, but the [object has no keys] part comes from the existing extractExceptionKeysForMessage which is used in multiple places, so I figured we can just keep it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea let's keep it. I trust people to understand what's going on here.

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://public@dsn.ingest.sentry.io/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,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();
}
Comment on lines +9 to +11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we just want to add the FF behaviour to this test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FF has a completely different behavior, the error does not go into the eventFromPlainObject path. So the whole test assertion would be different 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I leave it up to you. If you think it's valuable we can add a test - otherwise it's fine too.

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` (type=custom) captured as exception',
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,
},
});
});
36 changes: 33 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,33 @@ 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';

// 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried about this for similar reasons outlined in: #8161 (comment)

If people throw an object of a custom class and their code is minified, prototype.constructor.name will contain the minified name. Minified symbols may change across deploys and grouping may therefore break across deploys.
The current implementation doesn't have this issue as object keys are generally not minified.

I don't know yet how I would proceeed here though 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is true (tried it in a test app). What about the following - slightly hacky but maybe OK - solution:

if (className.length === 1) {
  // this is most likely minified/mangled, so just use "Object"
  return "Object"
}

? This will not capture everything (you may have mangled class names with more than one char), but usually mangling will reduce to one character as it is the smallest, and you rarely have more than 26 entities in the same scope 🤔 Or possibly we could even do className.length < 3 or so, as it is not necessarily a problem if we don't capture one or the other "real" class name, I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't do that. That seems a bit silly.

The more I think about this, the more I wouldn't change anything. We're just introducing new complexity that doesn't solve any particular issue.

The one thing I would consider is either updating the Error message to be a bit more clear what is happening or removing the "with keys ..." part and using scope.setFingerprint() to keep the grouping intact.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would shy away from using the fingerprint because it make it harder for users to understand how to filter the events out and understand how they are being grouped.

How about let's just not differentiate between class instances and plain js object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, for simplicity I will just go with Object for everything except events then? That should already be better than what we have right now and we can still tweak/further improve this later. (FWIW, I still think it would be helpful to get the class name, but we can think this over some more)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup sounds good to me - I think we can keep iterating

} catch (e) {
// ignore errors here
}
}
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` (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);
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
19 changes: 19 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,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, 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
}
}
3 changes: 2 additions & 1 deletion packages/browser/tsconfig.test.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down