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

Uncaught exception that is a POJO and no error is hard to understand #11778

Open
3 tasks done
Kobby-Bawuah opened this issue Apr 24, 2024 · 9 comments
Open
3 tasks done

Comments

@Kobby-Bawuah
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/browser

SDK Version

7.109.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

A user who recently upgraded Sentry's JavaScript SDK reports that there have been breaking changes introduced that were not documented, particularly around the captureMessage function's signature. Furthermore, errors originating from non-standard JavaScript objects are being misclassified by the SDK, potentially due to these changes.

Upgraded Sentry SDK from version 7.41.0 to 7.109.0

This line Sentry.captureMessage('non-human login, client-side detection', {level: 'warning', id: resp.user_id});
began to throw the error:

Object captured as exception with keys: isDefaultPrevented, isTrigger
Object captured as exception with keys: isTrigger, jQuery36009365839822830004

Expected Result

Changes in the SDK that require different method signatures should be documented clearly, especially when they are breaking changes.

Actual Result

Breaking change not documented

@mydea
Copy link
Member

mydea commented Apr 25, 2024

It does not appear to me as if that error would come from this captureMessage call. The error message captured indicates this is probably some Event or similar being captured, not a string 🤔

I'd need a link to a captured error event in SaaS, to look at this.

E.g. this error:

Object captured as exception with keys: isDefaultPrevented, isTrigger

comes from something like this happening:

const notReallyAnError = { isDefaultPrevented: false, isTrigger: true };
Sentry.captureException(notReallyAnError);

Or more likely something like this:

someHandler(notReallyAnError) {
  Sentry.captureException(notReallyAnError);
}

@ammurphy
Copy link

From __serialized__, and a fair amount of google-fu, we were able to ascertain that Object captured as exception with keys: ... was being generated by datatables.net code which was throwing a plain object. Fortunately, via the setting below, we could make it throw a true exception and see what was actually going on.
$.fn.dataTable.ext.errMode = 'throw';

Now we can see the actual error message created by datatables.net:
DataTables warning: table id=table_1 - Cannot reinitialise DataTable.

All that being said, the sentry.io server or client should be able to pass along such valuable info even if captureError is called on a plain JavaScript object (rather than the more appropriate captureMessage). The contents from __serialized__ as shown below are far from informative. Furthermore, Sentry will show the keys passed to the server, but delete all the values. None of this would be a real problem except for the fact that often times clients cannot readily control or configure library code as we have done above.

{
isTrigger: 3,
jQuery36006392669587889106: true,
namespace: dt,
rnamespace: {},
target: [HTMLElement: HTMLTableElement],
timeStamp: 1713094652307,
type: error
}

The purpose of Sentry is to find and fix bugs, not create new bugs based on rigid and incomplete reporting mechanisms.

@mydea
Copy link
Member

mydea commented May 2, 2024

From __serialized__, and a fair amount of google-fu, we were able to ascertain that Object captured as exception with keys: ... was being generated by datatables.net code which was throwing a plain object. Fortunately, via the setting below, we could make it throw a true exception and see what was actually going on. $.fn.dataTable.ext.errMode = 'throw';

Now we can see the actual error message created by datatables.net: DataTables warning: table id=table_1 - Cannot reinitialise DataTable.

All that being said, the sentry.io server or client should be able to pass along such valuable info even if captureError is called on a plain JavaScript object (rather than the more appropriate captureMessage). The contents from __serialized__ as shown below are far from informative. Furthermore, Sentry will show the keys passed to the server, but delete all the values. None of this would be a real problem except for the fact that often times clients cannot readily control or configure library code as we have done above.

{
isTrigger: 3,
jQuery36006392669587889106: true,
namespace: dt,
rnamespace: {},
target: [HTMLElement: HTMLTableElement],
timeStamp: 1713094652307,
type: error
}

The purpose of Sentry is to find and fix bugs, not create new bugs based on rigid and incomplete reporting mechanisms.

I totally get the sentiment, however it is sadly not that easy to detect what certain libraries throw as errors as something that is actionable. If we get a POJO passed to captureException, we need to use some heuristic to extract the message, etc. from it. We try to do that, e.g. we recently made a change that if the POJO has an Error as a top level property, we use that (for example: captureException({ txt: 'something', error: new Error('actual error here') }).

I don't really know how we should get from this:

{
  isTrigger: 3,
  jQuery36006392669587889106: true,
  namespace: dt,
  rnamespace: {},
  target: [HTMLElement: HTMLTableElement],
  timeStamp: 1713094652307,
  type: error
}

to an actionable error message, in a systemic way 🤔 if you have a proposal for some specific, specified error format that is wildly used out there, we're happy to consider supporting this. But please understand that we can't possibly parse & understand every thing that any library could possibly throw as an error - the JS ecosystem is simply too large for this to be feasible.

(Side note: We only use the keys for the error in order to not break grouping, as the POJO values are likely to change and thus every error captured would not be grouped together, which may flood your issues with "the same" thing many times over)

@ammurphy
Copy link

ammurphy commented May 3, 2024

we can't possibly parse & understand every thing that any library could possibly throw as an error - the JS ecosystem is simply too large for this to be feasible.

I totally agree, and datatables.net may not be popular enough to expect Sentry to understand all its thrown POJOs, but React and jQuery certainly are. And during our research of this issue, we saw plenty of users of those libraries complaining of similarly opaque events on Sentry.

@mydea
Copy link
Member

mydea commented May 3, 2024

we can't possibly parse & understand every thing that any library could possibly throw as an error - the JS ecosystem is simply too large for this to be feasible.

I totally agree, and datatables.net may not be popular enough to expect Sentry to understand all its thrown POJOs, but React and jQuery certainly are. And during our research of this issue, we saw plenty of users of those libraries complaining of similarly opaque events on Sentry.

But what is a jquery error? I couldn't find a reference for this. Possibly this is some kind of event being thrown?

Looking at this object that is apparently being thrown:

{
  isTrigger: 3,
  jQuery36006392669587889106: true,
  namespace: dt,
  rnamespace: {},
  target: [HTMLElement: HTMLTableElement],
  timeStamp: 1713094652307,
  type: error
}

What would you expect us to capture there, from this POJO object?

@ammurphy
Copy link

ammurphy commented May 3, 2024

I would expect some sort of hint from Sentry that this thrown POJO is being caused by datatables.net. In this particular case, the namespace key and value and the jQuery36006392669587889106 key allowed us to hone in on the root cause. If we could figure this out via Google and Stack Overflow, Sentry could build up a knowledge base of __serialized__ objects and do this on an automated basis (for many of the most popular libraries).

@mydea mydea changed the title Issue with captureMessage signature change and errors from non-standard exceptions in Sentry JavaScript SDK Uncaught exception that is a POJO and no error is hard to understand May 6, 2024
@mydea
Copy link
Member

mydea commented May 6, 2024

All we do here is we capture uncaught exceptions. If some code does this, we have a hard time - again, without access to the underlying code etc. it is quite hard for us to extract a reasonable error from this:

{
  isTrigger: 3,
  jQuery36006392669587889106: true,
  namespace: dt,
  rnamespace: {},
  target: [HTMLElement: HTMLTableElement],
  timeStamp: 1713094652307,
  type: error
}

As long as we don't have any concrete ideas on how we could/should programmatically extract an error from this, I don't see how we can really improve this on our end - the problem in this case is really libraries throwing non-standard POJOs instead of regular errors 😢

We are unlikely to add special case handling for datatables.net errors, at this point. I'd suggest to instead petition that library to make errMode=throw the default, which makes much more sense to me, but 🤷 there may be other reasons for this of course. We do capture the full serialized object in the issue, which should always be there as an escape hatch to narrow down edge cases like these.

To reiterate, we're happy to add further handling in cases where this is possible to extrapolate, but we need a clear path to extract an error message from a given POJO structure which we do not have for this example as of now - happy to hear any concrete suggestions!

@ammurphy
Copy link

ammurphy commented May 6, 2024

As long as we don't have any concrete ideas on how we could/should programmatically extract an error from this,

Let's not get hung up on creating an error event, which given the current JavaScript SDK, Sentry cannot recreate.

The point, however, is not to create an error event (synthetic or otherwise). The point is to assist the customer, and I've detailed above how to do so. I will do so again (below), but I implore you to have a fresh set of eyes to examine this feature request.

For any Non-Error exception captured with keys event report, alongside each __serialized__ object, Sentry should attempt to infer what library produced the object in question.

How to build this feature

  • extract 100K random __serialized__ objects from all Non-Error exception captured with keys event reports.
  • for each of the most common objects, locate the library that may be creating such an object. As detailed above, this is what we did for our datatables.net case.
  • take knowledge gained and create a mapping function (e.g. __serialized__ object shape / keys -> library)
  • apply this mapping to all future Non-Error exception captured with keys event reports

Given the Pareto distribution of JavaScript library usage, I posit this is a tractable problem, and Sentry can pass long usable hints in a significant per cent of cases.

the problem in this case is really libraries throwing non-standard POJOs instead of regular errors

Above, I have detailed a feature request to handle such scenarios and assist frustrated customers.

@Lms24
Copy link
Member

Lms24 commented May 8, 2024

Hey @ammurphy just jumping in since Francesco is out of office at the moment: We fully understand that these issues are hard to decipher and that it'd be great to infer the originating libraries.

We could of course do this but as already pointed out, there's a lot of complexity behind a feature like this. We could do this in the SDK but the consequence is that bundle size will inevitably increase. Not great, considering that a lot of our users won't even use the libraries for which we'd need build some kind of rule set. Which leaves us with implementing this on the server/event ingestion side. Again, something we could do but there are of course also performance and complexity tradeoffs.
Frankly, no matter where these rules would live, they'd become quite a large conglomerate of different cases, checking for certain properties, values, object structures, etc. In the ever-changing realms of JS and its libraries this will become a considerable maintenance effort.

I don't think it's a bad idea and it might become reality at some point. For now though, I'm gonna backlog this issue. Will leave it open though because I think it's a valid idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Status: No status
Development

No branches or pull requests

4 participants