-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Clarify NestJS error filters #12054
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
Clarify NestJS error filters #12054
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
Bundle ReportChanges will decrease total bundle size by 15 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying this! Reads way better now than before.
| } | ||
| ``` | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Can we also mention the last point, about HttpExceptions not being caught, below the code block just above this line? I think it's valuable information since users might expect otherwise in a catch-all filter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you move it in general or duplicate it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as you have it now
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
| In SDK versions `8.39.0` and above, the `SentryGlobalGenericFilter` is deprecated because the `SentryGlobalFilter` will handle GraphQL contexts automatically. | ||
|
|
||
| By default, exceptions with status code 4xx are not sent to Sentry. If you still want to capture these exceptions, you can do so manually with `Sentry.captureException()`: | ||
| If you have error filters for specific types of exceptions (for example `@Catch(HttpException)`, or any other `@Catch(...)` with arguments) and you want to capture errors caught by these filters, capture the errors in the `catch()` handler with `Sentry.captureException()`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case they would also need to remove our SentryExceptionCaptured decorator, otherwise unexpected errors get captured twice right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky one to think through. In general, our dedupe integration will drop the second capture call, as long as the two capture calls happen consecutively (no capture call on another error in between).
I am not sure if we should tell people to remove their SentryExceptionCaptured decorators though. The two things serve different purposes. One is more fine grained the other more blanket.
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
DESCRIBE YOUR PR
Hopefully makes it a bit more clear how specific error filters need to be transformed so that they will capture errors.
closes getsentry/sentry-javascript#14580
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.