-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
AggregateError error message is ignored during processing #62299
Comments
Thanks @kamilogorek for the hint! <3 As for solving this issue, I think this needs to be fixed in Relay or the Product. There is some mechanism that is deciding what to render as the error message - I currently don't know what that mechanism is. The SDK, as of now, is sending an array of errors ( The above is all defined in this RFC: https://github.com/getsentry/rfcs/blob/760467b85dbf86bd8b2b88d2a81f1a258dc07a1d/text/0079-exception-groups.md To make solving this a bit easier for the teams that pick this up, here is an example event: https://sentry-sdks.sentry.io/issues/4765181401/?project=4505391490007040&query=is%3Aunresolved&referrer=issue-stream&statsPeriod=30d&stream_index=1 Here is that event's payload:
And here is the envelope item the SDK sent for that event:
SuggestionIf we detect that there is an Exception Group in an event, we prioritize the message of the error that has |
Assigning to @getsentry/support for routing ⏲️ |
Routing to @getsentry/product-owners-issues for triage ⏲️ |
Routing to @getsentry/product-owners-settings-relay for triage ⏲️ |
Routing to @getsentry/product-owners-settings-relay for triage ⏲️ |
This seems to happen because the sentry/src/sentry/grouping/strategies/newstyle.py Lines 813 to 819 in 24f4a83
There might be a bug in the logic that identifies the main exception. |
Routing to @getsentry/product-owners-issues for triage ⏲️ |
@jjbayer Dayum that could be it! Thanks for the investigation! |
One thing though... the SDK doesn't set that. Does Sentry default to setting it to 1? |
It's a server-side helper flag that only ever gets set in the code i linked above, IIUC. If the flag is not set, we default to the last exception: sentry/src/sentry/eventtypes/error.py Lines 36 to 40 in e018852
|
Thanks everyone for the help debugging! I'll add this to our backlog and notify the people working on grouping improvements so they can prioritize as needed. |
This is related to, but not the same as, #59679 and, more generally, #64088. In those cases, we have chained errors rather than aggregate errors, and the problem is that we've been labeling linked errors as aggregate errors, and the main/top error has therefore been getting ignored. getsentry/sentry-javascript#10850 fixes that, but only just got merged and hasn't yet been released. And @jjbayer, yeah, all of the above matches what I found here. It also matches what's in the RFC. So I think this is indeed a backend fix, rather than an SDK fix as the above was. From your comment above:
I'm not sure it's a bug - it seems to match the RFC - but that approach makes the assumption that the root doesn't contain meaningful information, and here that's backfiring. Specifically, @kamilogorek (BTW, hey, Kamil! 🙂), in your example above, if your three child errors were separate events, they'd all fall into the same group (matching stacktraces), so for our purposes they count as only a single distinct exception, which in turn lets us ignore the root error. The only way we'd not ignore the root is if we couldn't collapse all the child exceptions into one, at which point it's sort of like, ugh, I guess we have to use the root, 'cause we can't decide between the kids... - in other words, we don't want to consider the root, but we will if we have no other choice. (The full logic, from which Joris quoted above, is here.) Now, in some cases, that's a perfectly reasonable assumption. Run the following and you will indeed get a root error worth ignoring: await Promise.any(
[1, 2, 3].map((num) => {
return Promise.reject(new Error(`Rejected promise #${num}`));
})
); which results in
The question is, how do we tell the difference between that case and the case where ignoring the root is the wrong choice? And how common is that second case? I'm not sure I know the answer to either question, especially a platform-independent answer. Kamil, or anyone else, thoughts? |
In this specific example yes, but imagine something like this: async function deleteIntegration(integration) {
await runTransaction(integration, [
cleanupConnections,
removeAuthorization,
notifyUser,
])
}
const deleteIntegrationResults = await Promise.allSettled(
integrations.map(async (integration) => deleteIntegration(integration))
)
const deleteIntegrationRejections = deleteIntegrationResults
.filter(isRejected)
.map((r) => r.reason)
if (deleteIntegrationRejections.length) {
Sentry.captureException(new AggregateError(deleteIntegrationRejections, `Failed to delete integrations ${integration.id} for user ${user.id}`))
} It can break on any of 3 operations inside the transaction, and those should not be grouped together. Hey 👋 |
Hmmm, yeah. That's a perfectly valid use case, and you're right that we don't handle it. As a workaround for specific errors, I think it should work to use the project-level fingerprint rules to both fix the grouping and give it the right title. So, for your example above, it'd be
(Or you could do That said, doing it that way could get to be a pain if you've got lots of different errors like this, plus most users have no idea it's even an option. A(slightly) better way would be for our backend to recognize a new Sentry.captureException(
new AggregateError(
deleteIntegrationRejections,
`Failed to delete integrations ${integration.id} for user ${user.id}`
),
{
mechanism: {
this_is_a_meaningful_aggregate_error_so_please_actually_use_it_when_grouping_and_titling_the_issue: true,
},
}
); Even better would be for the SDK to recognize a manual call to Anyway, for now I think the fingerprint rules are your best bet. |
I'm trying to decipher what's going on with this issue. I'm trying to figure out if displaying AggregateError's sub-errors requires some config (I would appreciate a link to the relevant docs), or if it's something that still needs dev work. |
@moonray it doesn't require any config. This issue is about the message that is being rendered in Sentry alongside the issue. |
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.91.0
Framework Version
No response
Link to Sentry event
No response
SDK Setup
No response
Steps to Reproduce
Expected Result
When creating a new
AggregateError
instance, it accepts an optional 2nd argument, which should be used as the error message per https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-aggregate-errorCurrently
LinkedErrors
will ignore that property and use the message from the first child instead.In the case above,
wat
should be the message for the root error.Actual Result
First child error is used as the source of the message.
The text was updated successfully, but these errors were encountered: