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

[mydea]

This PR adjusts the exception name generation for non-error objects in the browser SDK.

Previously

Previously, all non-error objects would get a message like:

Non-Error exception captured with keys: currentTarget, isTrusted, target, type

New

Now, we take the class name into consideration, as well as special casing some types:

• POJO: Object captured as exception with keys: prop1, prop2
• Class instance: MyTestClass captured as exception with keys: prop1, prop2
• Event: Event MouseEvent (click) captured as exception with keys: currentTarget, isTrusted, target, type
• ErrorEvent: Event ErrorEvent captured as exception with message Script error.

ref #7941

[github-actions]

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	21.3 KB (+0.62% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	66.43 KB (+0.46% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	19.82 KB (+0.59% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	58.9 KB (+0.53% 🔺)
@sentry/browser - Webpack (gzipped + minified)	21.43 KB (+0.48% 🔺)
@sentry/browser - Webpack (minified)	69.87 KB (+0.51% 🔺)
@sentry/react - Webpack (gzipped + minified)	21.46 KB (+0.45% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	49.39 KB (+0.23% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	28.9 KB (+0.37% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	27.12 KB (+0.36% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	49.33 KB (-0.01% 🔽)
@sentry/replay - Webpack (gzipped + minified)	43.08 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	68.48 KB (+0.19% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	61.37 KB (+0.19% 🔺)

[lforst]

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 🤔

[mydea]

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.

[lforst]

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.

[AbhiPrasad]

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?

[mydea]

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)

[AbhiPrasad]

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

[lforst]

Good change!

[lforst]

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

[mydea]

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

[lforst]

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

[lforst]

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

[mydea]

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?

[lforst]

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

[lforst]

Good change!