feat(nestjs): Handle non-HTTP contexts in SentryGlobalFilter - #24326
tiagovilasboas wants to merge 2 commits into
Conversation
Fixes getsentry#18472 Co-authored-by: Tiago Vilas Boas <tcarvalhovb@gmail.com>
0a5e99f to
00688ae
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 00688ae. Configure here.
| } | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Feat missing integration or E2E test
Medium Severity
This feat only adds unit tests for the new necord path in SentryGlobalFilter. The review rules require feat PRs to include at least one integration or E2E test, so the new context handling is not covered at that level. An integration test can drive a NestJS host with getType() returning necord and assert the captured event without adding necord or discord.js.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 00688ae. Configure here.
|
👋 @nicohrubec — Please review this PR when you get a chance! |
|
👋 @nicohrubec — Please review this PR when you get a chance! |
| if (contextType === 'necord') { | ||
| if (!isExpectedError(exception)) { | ||
| captureException(exception, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.necord.nestjs.global_filter', | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Adding necord (a library) here would require adding all different library exceptions here and therefore adding a big catch-all.
It makes probably more sense to just check if the context is not http.
// Custom context types (necord, ...) run through ExternalContextCreator and have no HTTP adapter.
if (contextType !== 'http') {
if (!isExpectedError(exception)) {
captureException(exception, {
mechanism: { handled: false, type: `auto.${contextType}.nestjs.global_filter` },
});
}
//...
}There was a problem hiding this comment.
Done in d069192 — replaced the Necord-only branch with a generic contextType !== 'http' path (dynamic mechanism type per context) and covered it with unit tests for necord and custom context types.
Replace the Necord-only branch with a generic contextType !== http path, as requested in review. Mechanism type stays dynamic per context.
tiagovilasboas
changed the title
|
👋 @mydea — Please review this PR when you get a chance! |
|
👋 @nicohrubec — Please review this PR when you get a chance! |
| this._logger.error(exception.message, exception.stack); | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
did you test the behavior for this? usually we rethrow exceptions to not interfere with user application behavior. however, there are exceptions for instance if you look at the rpc branch in this file. so not entirely sure this is correct, depends a bit on how the framework behaves here.
There was a problem hiding this comment.
Yes, tested it against real Nest (10.4.15): registered SentryGlobalFilter as APP_FILTER and ran a throwing handler through ExternalContextCreator with a necord context type, the same way necord wires its handlers.
The return doesn't swallow the error. ExternalExceptionsHandler.next() only uses the filter result if it's truthy, otherwise it falls back to ExternalExceptionFilter, which rethrows. So the caller still gets the original error, same as without the Sentry filter, and we capture it once. The rpc branch works the same way: returning there falls through to BaseRpcExceptionFilter, which still sends the error back to the client (checked with @nestjs/microservices 10.4.15). Nest master has the same fallback.
One side effect: the error gets logged twice (our _logger.error plus Nest's ExternalExceptionFilter). I can drop our log line in this branch if you'd like, and I can also add the framework-level test to the PR.
This branch has not been deployed
3 participants
NestJS custom execution contexts (
getType() !== 'http', e.g. Necord Discord handlers) were falling through to the HTTPBaseExceptionFilterpath, which expects an HTTP adapter and cannot reply on those hosts.SentryGlobalFilternow treats any remaining non-HTTP context the same way after the existing GraphQL / RPC / WebSocket branches: capture unexpected errors, skip expected Nest control-flow exceptions, logErrorinstances, and return without delegating to HTTP handling.The mechanism type follows the existing convention (
auto.${contextType}.nestjs.global_filter). No library-specific dependency is added — context detection uses the string Nest already sets on the execution host.Addresses review feedback from @s1gr1d (prefer
contextType !== 'http'over a Necord-only branch).Fixes #18472