-
-
Notifications
You must be signed in to change notification settings - Fork 17
refactor: migrate from @sentry/minimal to @sentry/node for improved error handling #1875
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ import { ConnectionTypesEnum } from '@rocketadmin/shared-code/dist/src/shared/en | |
| import { IDataAccessObject } from '@rocketadmin/shared-code/dist/src/shared/interfaces/data-access-object.interface.js'; | ||
| import { IDataAccessObjectAgent } from '@rocketadmin/shared-code/dist/src/shared/interfaces/data-access-object-agent.interface.js'; | ||
| import { FoundRowsDS } from '@rocketadmin/shared-code/src/data-access-layer/shared/data-structures/found-rows.ds.js'; | ||
| import Sentry from '@sentry/minimal'; | ||
| import * as Sentry from '@sentry/node'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/rocket-admin-rocketadmin-52f99a3d -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- changed file ---'
cat -n backend/src/entities/table/use-cases/get-table-rows.use.case.ts
printf '%s\n' '--- Sentry capture and exception filter bindings ---'
rg -n -C 8 --glob '*.{ts,js}' \
"captureException|class AllExceptionsFilter|UnknownSQLException|APP_FILTER|AllExceptionsFilter" \
backend/srcRepository: rocket-admin/rocketadmin Length of output: 50381 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- exact exception definition and uses ---'
rg -n -C 12 --glob '*.ts' "UnknownSQLException" backend
printf '%s\n' '--- exact filter implementation and registration ---'
rg -l --glob '*.ts' "AllExceptionsFilter|APP_FILTER" backend | while IFS= read -r file; do
echo "### $file"
cat -n "$file"
doneRepository: rocket-admin/rocketadmin Length of output: 50381 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- applicable backend convention ---'
cat /tmp/coderabbit-repo-knowledge/rocket-admin-rocketadmin-52f99a3d/conventions/backend.md
printf '%s\n' '--- exception hierarchy ---'
cat -n backend/src/exceptions/custom-exceptions/base-rocketadmin.exception.ts
cat -n backend/src/exceptions/custom-exceptions/unknown-sql-exception.ts
printf '%s\n' '--- global filter ---'
cat -n backend/src/exceptions/all-exceptions.filter.ts
printf '%s\n' '--- global filter registration ---'
sed -n '38,52p' backend/src/main.tsRepository: rocket-admin/rocketadmin Length of output: 9030 Remove the local Sentry capture for DAO errors. This path can report one DAO failure three times: the original error at line 207, 🤖 Prompt for AI Agents |
||
| import AbstractUseCase from '../../../common/abstract-use.case.js'; | ||
| import { IGlobalDatabaseContext } from '../../../common/application/global-database-context.interface.js'; | ||
| import { BaseType } from '../../../common/data-injection.tokens.js'; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,36 +1,16 @@ | ||
| import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nestjs/common'; | ||
| import Sentry from '@sentry/minimal'; | ||
| import { Observable } from 'rxjs'; | ||
| import { tap } from 'rxjs/operators'; | ||
|
|
||
| // Passthrough since plan 30. This interceptor used to capture every exception on its controllers | ||
| // itself — through the deprecated @sentry/minimal v6 API, whose hub the @sentry/node v10 client | ||
| // never wires, so those captures were silently dropped for as long as both packages coexisted — | ||
| // and each error was ALSO captured by the global AllExceptionsFilter, which sees every exception | ||
| // on every route. The filter is now the single (working) capture point, with the request context | ||
| // attached inside a per-event scope. The class stays so the existing | ||
| // @UseInterceptors(SentryInterceptor) decorators keep compiling; remove them at leisure. | ||
| @Injectable() | ||
| export class SentryInterceptor implements NestInterceptor { | ||
| async intercept(context: ExecutionContext, next: CallHandler): Promise<Observable<any>> { | ||
| try { | ||
| const contextArgs = context.getArgs(); | ||
| const userEmail = contextArgs[0]?.decoded?.email; | ||
| const receivedConnectionHost = contextArgs[0]?.body?.host; | ||
| return next.handle().pipe( | ||
| tap(null, async (exception) => { | ||
| Sentry.setContext('user_email', { | ||
| email: userEmail ? userEmail : 'unknown', | ||
| }); | ||
| if (receivedConnectionHost) { | ||
| Sentry.setContext('received_connection_hostname', { | ||
| hostname: receivedConnectionHost, | ||
| }); | ||
| } | ||
| if (exception.originalMessage) { | ||
| Sentry.setContext('original_exception_message', { | ||
| originalMessage: exception.originalMessage, | ||
| }); | ||
| } | ||
| Sentry.captureException(exception); | ||
| }), | ||
| ); | ||
| } catch (e) { | ||
| console.error(e); | ||
| return next.handle(); | ||
| } | ||
| intercept(_context: ExecutionContext, next: CallHandler): Observable<unknown> { | ||
| return next.handle(); | ||
| } | ||
| } |
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep unexpected authorization errors on one Sentry capture path.
These authorization catches capture non-
HttpExceptionerrors locally and then map them toInternalServerErrorException, which the global filter captures again. One failure can therefore produce duplicate events while the second loses the original error context. Remove the local captures, or preserve the original error and make the global filter the sole capture owner.📍 Affects 2 files
backend/src/authorization/auth-with-api.middleware.ts#L39-L39(this comment)backend/src/authorization/non-scoped-auth.middleware.ts#L74-L74🤖 Prompt for AI Agents