Repository navigation
Provide fingerprint on error object #11133
Description
Problem Statement
It is not possible for Sentry to always group errors correctly, as "correct" is subjective to the developer. If Sentry provides an option to enrich a thrown error object with a fingerprint, the developer is in full control over the fingerprint.
- Either Sentry groups to aggressively and groups errors that don't belong together -> Different errors (conceptually) that share the same stack trace.
- Conceptually the same errors that originate from different call stacks.
The current solutions has the following issues:
scope.setFingerprintonly works if the error is captured in the scope in which it occurs. If, instead, the error is thrown and caught in one central place, all scope information (and thus the fingerprint) is lost.beforeSendadds needless complexity by requiring both copy-pasta setup code for every project, and at least one custom class that must also be copied alongside.
Solution Brainstorm
Imagine the following simplified code snippet:
export async function getThing() {
const res = await fetch('https://api.example.com/thing');
if (!res.ok) {
throw new Error(
`Failed to fetch the thing: ${res.status} ${res.statusText}.`,
{
cause: {
status: res.status,
statusText: res.statusText,
body: await res.text(),
},
},
);
}
try {
return await res.json();
} catch (err) {
throw new Error('Failed to fetch the thing: invalid JSON.', {
cause: {
body: await res.text(),
},
});
}
}Any "Failed to fetch the thing: {status} {text}" error would be grouped as one issue by the same stack trace. There are several solutions to this:
Sentry.captureExceptioninstead of throwing the error. This, however, is not always an option. If there is no logger instance available, there is no way to otherwise log the error, and depending on the project, the option return types this solution implies, don't always work.- Attach fingerprint to the error object:
Either with an error derivate that Sentry provides and attachesfingerprintfrom a constructor parameter, or alternatively:
throw Object.assign(
new Error(`Failed to fetch the thing: ${res.status} ${res.statusText}.`, {
cause: {
status: res.status,
statusText: res.statusText,
body: await res.text(),
},
}),
{
fingerprint: ['fetch-thing-failed', res.status],
},
);Then, the Sentry SDK would check for .fingerprint properties on errors out of the box.
- Attach the scope to any thrown errors. The initial problem is that any errors thrown within a scope lose any context. This could be fixed with a
try-catchwithinSentry.withScopethat simply attaches itself to any errors it catches, so that any higher-level error handler still has access to the scope in which the error occurred. This would be the best solution in my opinion, as it also retains any other metadata set on the scope, such as tags and data, and makes those available to Sentry, as well as any other error logger, such as >stderr, which could then also benefit from scope data.
Reacted by Jesse Youngblood
Activity
AbhiPrasad commentedon Mar 15, 2024
Thanks for the really detailed writeup! I guess the decision here is if we want to do the following:
Right now you can already attach arbitrary properties to an error, and then in
beforeSendsetevent.fingerprintaccordingly based on theoriginalExceptionin the hint.My worry with adopting an out of the box default is that server-side generated fingerprints are right now dependent on stacktraces. Isolating this to just a error object potentially introduces a footgun that degrades the issue experience
Also we have some grouping improvements lined up! See: getsentry/sentry#66319
jeengbe commentedon Mar 15, 2024
What do you mean with this?
Unless you willingly set
.fingerprinton an error, nothing changes at all. Only if you do so, a fingerprint is already set, otherwise, the stack trace is still present.jeengbe commentedon Mar 15, 2024
Great discussion you've linked there, however, I feel that's only partly applicable here. This issue is fully about enabling developers to provide custom fingerprints, as in the end, we decide what is considered the same issue.
lforst commentedon Mar 18, 2024
I personally generally like the idea, but I am not fully convinced that we should add it to the SDK by default.
@jeengbe I second what Abhi was saying that you can pretty easily implement this yourself right now with a simple combination between setting a
fingerprintproperty on the error and then reading it off and populatingevent.fingerprintinbeforeSend. I see your point about it being potentially a lot of duplicated code though.I wanna let this simmer a bit. This is something we can add at any time. Maybe it is not high prio right now. But thanks again for the idea and feedback. Much appreciated!
jeengbe commentedon Mar 18, 2024
Perhaps focus the discussion on the last point;
The benefit with this would be that no new classes/properties/API would be required, and all other and future features a scope provides would come out of the box too. Might be worth to port this to a separate issue/discussion, even.
Lms24 commentedon Mar 19, 2024
I think this is a great point and I actually had to write some tests to believe that this is in fact the current behavior (see #11204) 😅 I'll take your suggestion about attaching the scope to the error back to the team. I think it sounds good but there are probably edge cases where this isn't a great idea.
Lms24 commentedon Apr 2, 2024
Following up on this issue: As discussed in getsentry/sentry-python#2857, general consensus across SDKs is that for the moment we're not going to attach things to the error object but instead keep scope validity as we currently do.
I know this is not the outcome you were looking for but for now my best advice would be to take care of this yourself in the code. as suggested in the comments above. Thanks though for making us/me aware of this still somewhat unintuitive behaviour. Closing this issue for the time being. Please feel free to ping me if you want to see this reopened. Thanks!