From 82ddf943f032071af126144bf2caedd8e6e69011 Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Thu, 18 Jan 2024 15:00:37 +0100 Subject: [PATCH] feat(sentry-plugin): Use ErrorHandlerStrategy for better error coverage Using the new ErrorHandlerStrategy allows us to collect errors from both the server and the worker. It also prevents the default exception filter from being overridden. --- .../src/sentry-error-handler-strategy.ts | 42 +++++++++++++++++++ packages/sentry-plugin/src/sentry-plugin.ts | 16 ++----- packages/sentry-plugin/src/sentry.filter.ts | 38 ++++++++++------- 3 files changed, 68 insertions(+), 28 deletions(-) create mode 100644 packages/sentry-plugin/src/sentry-error-handler-strategy.ts diff --git a/packages/sentry-plugin/src/sentry-error-handler-strategy.ts b/packages/sentry-plugin/src/sentry-error-handler-strategy.ts new file mode 100644 index 0000000000..9df5ae2ae2 --- /dev/null +++ b/packages/sentry-plugin/src/sentry-error-handler-strategy.ts @@ -0,0 +1,42 @@ +import { ArgumentsHost, ExecutionContext } from '@nestjs/common'; +import { GqlContextType, GqlExecutionContext } from '@nestjs/graphql'; +import { setContext } from '@sentry/node'; +import { ErrorHandlerStrategy, I18nError, Injector, Job, LogLevel } from '@vendure/core'; + +import { SentryService } from './sentry.service'; + +export class SentryErrorHandlerStrategy implements ErrorHandlerStrategy { + private sentryService: SentryService; + + init(injector: Injector) { + this.sentryService = injector.get(SentryService); + } + + handleServerError(exception: Error, { host }: { host: ArgumentsHost }) { + // We only care about errors which have at least a Warn log level + const shouldLogError = exception instanceof I18nError ? exception.logLevel <= LogLevel.Warn : true; + if (shouldLogError) { + if (host?.getType() === 'graphql') { + const gqlContext = GqlExecutionContext.create(host as ExecutionContext); + const info = gqlContext.getInfo(); + setContext('GraphQL Error Context', { + fieldName: info.fieldName, + path: info.path, + }); + } + const variables = (exception as any).variables; + if (variables) { + setContext('GraphQL Error Variables', variables); + } + this.sentryService.captureException(exception); + } + } + + handleWorkerError(exception: Error, { job }: { job: Job }) { + setContext('Worker Context', { + queueName: job.queueName, + jobId: job.id, + }); + this.sentryService.captureException(exception); + } +} diff --git a/packages/sentry-plugin/src/sentry-plugin.ts b/packages/sentry-plugin/src/sentry-plugin.ts index 8b92646074..8ac18e6391 100644 --- a/packages/sentry-plugin/src/sentry-plugin.ts +++ b/packages/sentry-plugin/src/sentry-plugin.ts @@ -1,5 +1,4 @@ import { MiddlewareConsumer, NestModule } from '@nestjs/common'; -import { APP_FILTER } from '@nestjs/core'; import { PluginCommonModule, VendurePlugin } from '@vendure/core'; import { SentryAdminTestResolver } from './api/admin-test.resolver'; @@ -8,7 +7,7 @@ import { ErrorTestService } from './api/error-test.service'; import { SENTRY_PLUGIN_OPTIONS } from './constants'; import { SentryApolloPlugin } from './sentry-apollo-plugin'; import { SentryContextMiddleware } from './sentry-context.middleware'; -import { SentryExceptionsFilter } from './sentry.filter'; +import { SentryErrorHandlerStrategy } from './sentry-error-handler-strategy'; import { SentryService } from './sentry.service'; import { SentryPluginOptions } from './types'; @@ -108,21 +107,14 @@ const SentryOptionsProvider = { */ @VendurePlugin({ imports: [PluginCommonModule], - providers: [ - SentryOptionsProvider, - SentryService, - ErrorTestService, - { - provide: APP_FILTER, - useClass: SentryExceptionsFilter, - }, - ], + providers: [SentryOptionsProvider, SentryService, ErrorTestService], configuration: config => { config.apiOptions.apolloServerPlugins.push( new SentryApolloPlugin({ enableTracing: !!SentryPlugin.options.enableTracing, }), ); + config.systemOptions.errorHandlers.push(new SentryErrorHandlerStrategy()); return config; }, adminApiExtensions: { @@ -130,7 +122,7 @@ const SentryOptionsProvider = { resolvers: () => (SentryPlugin.options.includeErrorTestMutation ? [SentryAdminTestResolver] : []), }, exports: [SentryService], - compatibility: '^2.0.0', + compatibility: '^2.2.0', }) export class SentryPlugin implements NestModule { static options: SentryPluginOptions = {} as any; diff --git a/packages/sentry-plugin/src/sentry.filter.ts b/packages/sentry-plugin/src/sentry.filter.ts index cd248842ec..c69ef47c42 100644 --- a/packages/sentry-plugin/src/sentry.filter.ts +++ b/packages/sentry-plugin/src/sentry.filter.ts @@ -2,26 +2,32 @@ import type { ArgumentsHost, ExceptionFilter } from '@nestjs/common'; import { Catch, ExecutionContext } from '@nestjs/common'; import { GqlContextType, GqlExecutionContext } from '@nestjs/graphql'; import { setContext } from '@sentry/node'; +import { ExceptionLoggerFilter, ForbiddenError, I18nError, LogLevel } from '@vendure/core'; import { SentryService } from './sentry.service'; -@Catch() -export class SentryExceptionsFilter implements ExceptionFilter { - constructor(private readonly sentryService: SentryService) {} +export class SentryExceptionsFilter extends ExceptionLoggerFilter { + constructor(private readonly sentryService: SentryService) { + super(); + } - catch(exception: Error, host: ArgumentsHost): void { - if (host.getType() === 'graphql') { - const gqlContext = GqlExecutionContext.create(host as ExecutionContext); - const info = gqlContext.getInfo(); - setContext('GraphQL Error Context', { - fieldName: info.fieldName, - path: info.path, - }); - } - const variables = (exception as any).variables; - if (variables) { - setContext('GraphQL Error Variables', variables); + catch(exception: Error, host: ArgumentsHost) { + const shouldLogError = exception instanceof I18nError ? exception.logLevel <= LogLevel.Warn : true; + if (shouldLogError) { + if (host.getType() === 'graphql') { + const gqlContext = GqlExecutionContext.create(host as ExecutionContext); + const info = gqlContext.getInfo(); + setContext('GraphQL Error Context', { + fieldName: info.fieldName, + path: info.path, + }); + } + const variables = (exception as any).variables; + if (variables) { + setContext('GraphQL Error Variables', variables); + } + this.sentryService.captureException(exception); } - this.sentryService.captureException(exception); + return super.catch(exception, host); } }