Linked exceptions and which is used for grouping?

[Parakoos]

Environment

SaaS (https://sentry.io/)

What are you trying to accomplish?

When playing a sound, I wrap the code in a try-catch and if I get a NotAllowedError then I catch it and wrap it in my own kind of error, with a more meaningful error message: throw new SgtError('Permission to play sounds not given.', caughtError) (where the constructor will set the cause property to the given error)

In Sentry, I can see the two errors:

Related Exceptions
    SgtError: Permission to play sounds not given.
        NotAllowedError: Permission was denied

BUT! And here is my problem, all the grouping, fingerprinting and the title and error message of the Sentry issue are all based on the underlying NotAllowedError.

This becomes a problem when in a different part of the code, I try to set the Wake Lock and that operation also throws NotAllowedError and so when I wrap that in a different custom error with a different message, the sound-related issues are all mixed up with the wake-lock issues, since the underlying error is the same, and the 'parent error' is ignored.

How are you getting stuck?

I would like to be able to signal to Sentry to focus on the main error instead of the linked error. Is there a way to do this? I am already using beforeSend to set custom finger prints for these errors, but... it seems clumsy and I can't seem to find a way to fix other things like title and description in the UI.

Where in the product are you?

Unknown

Link

No response

DSN

No response

Version

No response

┆Issue is synchronized with this Jira Improvement by Unito

[getsantry]

Assigning to @getsentry/support for routing ⏲️

[getsantry]

Routing to @getsentry/product-owners-unknown for triage ⏲️

[getsantry]

Routing to @getsentry/product-owners-issues for triage ⏲️

[lobsterkatie]

@Parakoos - You're right, that does seem like incorrect behavior! I'm going to mark this as a bug and add it to our backlog.

For posterity, here's what I found when I was digging into this:

• The overall problem is that main_exception_id is getting set to the wrong exception. Here's the JSON from an event in our FE project showing this behavior. main_exception_id is at the top level.

• main_exception_id was introduced in feat(grouping): Exception Groups #48653, and is set here. As you can see, it's related to the top-level exceptions computed here, and it's in that latter spot where the problem lies. In this case, the error we want is labeled as an exception group, and that function therefore ignores it and picks the next one down the chain.

• That logic, and the grouping and titling behavior, gibe with what's described in the RFC here and here. What they describe makes sense in situations where the first error in the chain isn't a meaningful error in and of itself, but a way to collect errors. Though it's (relatively) new, JS has this, too, in the form of AggregateError, but that's not what we're dealing with here.

So, it seems to me that the solution is either for the SDK to tag the outermost error as something other than an exception group, or for the logic I linked above to detect cases where the root error is meaningful rather than just a collector, and handle them differently.

@AbhiPrasad - As someone who both approved the RFC and knows the SDK, which of the above do you think is a better solution? [UPDATE: I've talked myself out of the SDK option, I think - see my comment below. Still curious if you have thoughts, but I'm going to throw this on our grouping project board for now.]

UPDATE:

Here's a screenshot of the JSON I linked above, showing the problem. The error we want to group on is marked as an exception group, and therefore it's not chosen as the "main" error.

Screenshot

[Parakoos]

Oh wow, well, I'm glad I reported it then! 😃

While you are working on this, is there some kind of workaround to manually set the main_exception_id, either by doing something in the beforeSend callback or in the way we capture and rethrow an error? Perhaps if I threw an AggregateError?

[lobsterkatie]

You could try wrapping your real error in an AggregateError, though it would probably lead to a certain amount of noise/unhelpful data on your issue detail page. I would say that you could use beforeSend to make the change I was suggesting the SDK make by default, but I'm guessing that might have other consequences in terms of how the error is displayed. You could certainly try it. It would mean changing the is_exception_group value.

If we did it for real in the SDK, we might have to make changes in our backend, too, to compensate. (The more I talk about this, the more I think that the SDK route probably isn't the way for us to fix this, for exactly that reason. Probably better/easier to just throw a check into our logic for setting main_exception_id.)

All of that said, really the thing is for us to fix this, rather than for you to resort to workarounds. No guarantee given the holidays, competing priorities, etc., but if this is an easy fix, I'll try to advocate for it getting pulled from the backlog.

[AbhiPrasad]

I'd avoid making changes in the SDK if possible, and changing how we set main_exception_id seems reasonable enough as a solution for this. It's been a while since I looked at this at the SDK side though, so only basing this off a cursory glance.

[lobsterkatie]

Yup, I agree with you.

UPDATE: I've talked myself out of the SDK option, I think

If nothing else, I kind of don't feel like tracking down all of the possible side effects of setting a different value for is_exception_group, let alone writing code to compensate for them. We're going to give the changing-the-main_exception_id-logic approach a try.

Thanks for taking a look!

[mattleonowicz]

Any updates on this?
I think this is pretty serious bug. Linked Errors are really powerful with the common practice of repackaging Errors to give them more context.
The temporary solution we have is to erase cause before passing the Error to Sentry and manually set up the serialized "cause error" in extra field instead. This has a huge downside of having a stacktrace which is not easy to explore (since it's without the usual sourcemap applied on top of it).

If you have any idea for a workaround that would allow us to use cause again, but force issue grouping on by the main error please share 🙏
Thank you

[the21st]

+1, we are paying Sentry a heft sum each month and this bug is completely unacceptable – it makes search unusable for us for any issues with linked errors.

[brianthi]

Appreciate y'all sharing your use cases with us. We'll revisit this issue today and share an update when we have a plan for a fix or a fix itself.

[lobsterkatie]

After more investigation, @AbhiPrasad and I agreed that the change actually should come in the SDK.

It's currently marking linked errors as exception groups, when that designation should be reserved for instances of AggregateError (as confirmed in our event schema here). When grouping events, we ignore the top error of an exception group because it's just a container, and since the top error in a chain of errors is getting designated incorrectly, we're therefore incorrectly ignoring it.

We'll post an update when we have news on that fix.

[andrewmclagan]

@lobsterkatie Will second how crippling this issue is, considering this is a costly paid service and this issue renders the service almost pointless. I would expect this issue to receive a very high priority, its been open for months.

[AbhiPrasad]

Hey folks - we've been busy working on our new major version for the JS SDK, which has been our primary focus.

Apologies for the delay, we'll bump this up in priority

[AbhiPrasad]

We've released a fix for this as part of JS SDK release https://github.com/getsentry/sentry-javascript/releases/tag/7.104.0.