feat(nestjs): Add SentryGlobalGraphQLFilter (#13545)

nicohrubec · web-flow · commit 86aeba871bef · 2024-09-02T15:56:43.000+02:00
`BaseExceptionFilter` should not be used in GraphQL applications ([ref](nestjs/nest#5958 (comment))). Currently the `SentryGlobalFilter` extends the `BaseExceptionFilter`, which is why GraphQL applications break if an exception is thrown. By default, NestJS + GraphQL environments use the `ExternalExceptionFilter` ([ref](https://github.com/nestjs/nest/blob/master/packages/core/exceptions/external-exception-filter.ts)), which essentially just rethrows the error. So adding a new `SentryGlobalGraphQLFilter` that captures the exception in Sentry and then simply rethrows the error. Additionally, added a new e2e test app to test GraphQL applications. Added a test to verify that basic error reporting works correctly now.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -908,6 +908,7 @@ jobs:
             'nestjs-basic',
             'nestjs-distributed-tracing',
             'nestjs-with-submodules',
+            'nestjs-graphql',
             'node-exports-test-app',
             'node-koa',
             'node-connect',
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/.gitignore b/dev-packages/e2e-tests/test-applications/nestjs-graphql/.gitignore
@@ -0,0 +1,56 @@
+# compiled output
+/dist
+/node_modules
+/build
+
+# Logs
+logs
+*.log
+npm-debug.log*
+pnpm-debug.log*
+yarn-debug.log*
+yarn-error.log*
+lerna-debug.log*
+
+# OS
+.DS_Store
+
+# Tests
+/coverage
+/.nyc_output
+
+# IDEs and editors
+/.idea
+.project
+.classpath
+.c9/
+*.launch
+.settings/
+*.sublime-workspace
+
+# IDE - VSCode
+.vscode/*
+!.vscode/settings.json
+!.vscode/tasks.json
+!.vscode/launch.json
+!.vscode/extensions.json
+
+# dotenv environment variable files
+.env
+.env.development.local
+.env.test.local
+.env.production.local
+.env.local
+
+# temp directory
+.temp
+.tmp
+
+# Runtime data
+pids
+*.pid
+*.seed
+*.pid.lock
+
+# Diagnostic reports (https://nodejs.org/api/report.html)
+report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/.npmrc b/dev-packages/e2e-tests/test-applications/nestjs-graphql/.npmrc
@@ -0,0 +1,2 @@
+@sentry:registry=http://127.0.0.1:4873
+@sentry-internal:registry=http://127.0.0.1:4873
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/nest-cli.json b/dev-packages/e2e-tests/test-applications/nestjs-graphql/nest-cli.json
@@ -0,0 +1,8 @@
+{
+  "$schema": "https://json.schemastore.org/nest-cli",
+  "collection": "@nestjs/schematics",
+  "sourceRoot": "src",
+  "compilerOptions": {
+    "deleteOutDir": true
+  }
+}
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/package.json b/dev-packages/e2e-tests/test-applications/nestjs-graphql/package.json
@@ -0,0 +1,50 @@
+{
+  "name": "nestjs-graphql",
+  "version": "0.0.1",
+  "private": true,
+  "scripts": {
+    "build": "nest build",
+    "format": "prettier --write \"src/**/*.ts\" \"test/**/*.ts\"",
+    "start": "nest start",
+    "start:dev": "nest start --watch",
+    "start:debug": "nest start --debug --watch",
+    "start:prod": "node dist/main",
+    "clean": "npx rimraf node_modules pnpm-lock.yaml",
+    "test": "playwright test",
+    "test:build": "pnpm install",
+    "test:assert": "pnpm test"
+  },
+  "dependencies": {
+    "@apollo/server": "^4.10.4",
+    "@nestjs/apollo": "^12.2.0",
+    "@nestjs/common": "^10.3.10",
+    "@nestjs/core": "^10.3.10",
+    "@nestjs/graphql": "^12.2.0",
+    "@nestjs/platform-express": "^10.3.10",
+    "@sentry/nestjs": "^8.21.0",
+    "graphql": "^16.9.0",
+    "reflect-metadata": "^0.1.13",
+    "rxjs": "^7.8.1"
+  },
+  "devDependencies": {
+    "@playwright/test": "^1.44.1",
+    "@sentry-internal/test-utils": "link:../../../test-utils",
+    "@nestjs/cli": "^10.0.0",
+    "@nestjs/schematics": "^10.0.0",
+    "@nestjs/testing": "^10.0.0",
+    "@types/express": "^4.17.17",
+    "@types/node": "18.15.1",
+    "@types/supertest": "^6.0.0",
+    "@typescript-eslint/eslint-plugin": "^6.0.0",
+    "@typescript-eslint/parser": "^6.0.0",
+    "eslint": "^8.42.0",
+    "eslint-config-prettier": "^9.0.0",
+    "eslint-plugin-prettier": "^5.0.0",
+    "prettier": "^3.0.0",
+    "source-map-support": "^0.5.21",
+    "supertest": "^6.3.3",
+    "ts-loader": "^9.4.3",
+    "tsconfig-paths": "^4.2.0",
+    "typescript": "^4.9.5"
+  }
+}
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/playwright.config.mjs b/dev-packages/e2e-tests/test-applications/nestjs-graphql/playwright.config.mjs
@@ -0,0 +1,7 @@
+import { getPlaywrightConfig } from '@sentry-internal/test-utils';
+
+const config = getPlaywrightConfig({
+  startCommand: `pnpm start`,
+});
+
+export default config;
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/src/app.module.ts b/dev-packages/e2e-tests/test-applications/nestjs-graphql/src/app.module.ts
@@ -0,0 +1,30 @@
+import { ApolloDriver } from '@nestjs/apollo';
+import { Logger, Module } from '@nestjs/common';
+import { APP_FILTER } from '@nestjs/core';
+import { GraphQLModule } from '@nestjs/graphql';
+import { SentryGlobalGraphQLFilter, SentryModule } from '@sentry/nestjs/setup';
+import { AppResolver } from './app.resolver';
+
+@Module({
+  imports: [
+    SentryModule.forRoot(),
+    GraphQLModule.forRoot({
+      driver: ApolloDriver,
+      autoSchemaFile: true,
+      playground: true, // sets up a playground on https://localhost:3000/graphql
+    }),
+  ],
+  controllers: [],
+  providers: [
+    AppResolver,
+    {
+      provide: APP_FILTER,
+      useClass: SentryGlobalGraphQLFilter,
+    },
+    {
+      provide: Logger,
+      useClass: Logger,
+    },
+  ],
+})
+export class AppModule {}
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/src/app.resolver.ts b/dev-packages/e2e-tests/test-applications/nestjs-graphql/src/app.resolver.ts
@@ -0,0 +1,14 @@
+import { Query, Resolver } from '@nestjs/graphql';
+
+@Resolver()
+export class AppResolver {
+  @Query(() => String)
+  test(): string {
+    return 'Test endpoint!';
+  }
+
+  @Query(() => String)
+  error(): string {
+    throw new Error('This is an exception!');
+  }
+}
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/src/instrument.ts b/dev-packages/e2e-tests/test-applications/nestjs-graphql/src/instrument.ts
@@ -0,0 +1,8 @@
+import * as Sentry from '@sentry/nestjs';
+
+Sentry.init({
+  environment: 'qa', // dynamic sampling bias to keep transactions
+  dsn: process.env.E2E_TEST_DSN,
+  tunnel: `http://localhost:3031/`, // proxy server
+  tracesSampleRate: 1,
+});
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/src/main.ts b/dev-packages/e2e-tests/test-applications/nestjs-graphql/src/main.ts
@@ -0,0 +1,15 @@
+// Import this first
+import './instrument';
+
+// Import other modules
+import { NestFactory } from '@nestjs/core';
+import { AppModule } from './app.module';
+
+const PORT = 3030;
+
+async function bootstrap() {
+  const app = await NestFactory.create(AppModule);
+  await app.listen(PORT);
+}
+
+bootstrap();
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/start-event-proxy.mjs b/dev-packages/e2e-tests/test-applications/nestjs-graphql/start-event-proxy.mjs
@@ -0,0 +1,6 @@
+import { startEventProxyServer } from '@sentry-internal/test-utils';
+
+startEventProxyServer({
+  port: 3031,
+  proxyServerName: 'nestjs-graphql',
+});
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-graphql/tests/errors.test.ts
@@ -0,0 +1,49 @@
+import { expect, test } from '@playwright/test';
+import { waitForError } from '@sentry-internal/test-utils';
+
+test('Sends exception to Sentry', async ({ baseURL }) => {
+  const errorEventPromise = waitForError('nestjs-graphql', event => {
+    return !event.type && event.exception?.values?.[0]?.value === 'This is an exception!';
+  });
+
+  const response = await fetch(`${baseURL}/graphql`, {
+    method: 'POST',
+    headers: {
+      'Content-Type': 'application/json',
+    },
+    body: JSON.stringify({
+      query: `query { error }`,
+    }),
+  });
+
+  const json_response = await response.json();
+  const errorEvent = await errorEventPromise;
+
+  expect(json_response?.errors[0]).toEqual({
+    message: 'This is an exception!',
+    locations: expect.any(Array),
+    path: ['error'],
+    extensions: {
+      code: 'INTERNAL_SERVER_ERROR',
+      stacktrace: expect.any(Array),
+    },
+  });
+
+  expect(errorEvent.exception?.values).toHaveLength(1);
+  expect(errorEvent.exception?.values?.[0]?.value).toBe('This is an exception!');
+
+  expect(errorEvent.request).toEqual({
+    method: 'POST',
+    cookies: {},
+    data: '{"query":"query { error }"}',
+    headers: expect.any(Object),
+    url: 'http://localhost:3030/graphql',
+  });
+
+  expect(errorEvent.transaction).toEqual('POST /graphql');
+
+  expect(errorEvent.contexts?.trace).toEqual({
+    trace_id: expect.any(String),
+    span_id: expect.any(String),
+  });
+});
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/tsconfig.build.json b/dev-packages/e2e-tests/test-applications/nestjs-graphql/tsconfig.build.json
@@ -0,0 +1,4 @@
+{
+  "extends": "./tsconfig.json",
+  "exclude": ["node_modules", "test", "dist"]
+}
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-graphql/tsconfig.json b/dev-packages/e2e-tests/test-applications/nestjs-graphql/tsconfig.json
@@ -0,0 +1,22 @@
+{
+  "compilerOptions": {
+    "module": "commonjs",
+    "declaration": true,
+    "removeComments": true,
+    "emitDecoratorMetadata": true,
+    "experimentalDecorators": true,
+    "allowSyntheticDefaultImports": true,
+    "target": "ES2021",
+    "sourceMap": true,
+    "outDir": "./dist",
+    "baseUrl": "./",
+    "incremental": true,
+    "skipLibCheck": true,
+    "strictNullChecks": false,
+    "noImplicitAny": false,
+    "strictBindCallApply": false,
+    "forceConsistentCasingInFileNames": false,
+    "noFallthroughCasesInSwitch": false,
+    "moduleResolution": "Node16"
+  }
+}
diff --git a/packages/nestjs/src/setup.ts b/packages/nestjs/src/setup.ts
@@ -6,7 +6,7 @@ import type {
   NestInterceptor,
   OnModuleInit,
 } from '@nestjs/common';
-import { Catch, Global, Injectable, Module } from '@nestjs/common';
+import { Catch, Global, HttpException, Injectable, Logger, Module } from '@nestjs/common';
 import { APP_INTERCEPTOR, BaseExceptionFilter } from '@nestjs/core';
 import {
   SEMANTIC_ATTRIBUTE_SENTRY_OP,
@@ -31,7 +31,11 @@ import { isExpectedError } from './helpers';
  */
 class SentryTracingInterceptor implements NestInterceptor {
   // used to exclude this class from being auto-instrumented
-  public static readonly __SENTRY_INTERNAL__ = true;
+  public readonly __SENTRY_INTERNAL__: boolean;
+
+  public constructor() {
+    this.__SENTRY_INTERNAL__ = true;
+  }
 
   /**
    * Intercepts HTTP requests to set the transaction name for Sentry tracing.
@@ -61,7 +65,12 @@ export { SentryTracingInterceptor };
  * Global filter to handle exceptions and report them to Sentry.
  */
 class SentryGlobalFilter extends BaseExceptionFilter {
-  public static readonly __SENTRY_INTERNAL__ = true;
+  public readonly __SENTRY_INTERNAL__: boolean;
+
+  public constructor() {
+    super();
+    this.__SENTRY_INTERNAL__ = true;
+  }
 
   /**
    * Catches exceptions and reports them to Sentry unless they are expected errors.
@@ -78,11 +87,51 @@ class SentryGlobalFilter extends BaseExceptionFilter {
 Catch()(SentryGlobalFilter);
 export { SentryGlobalFilter };
 
+/**
+ * Global filter to handle exceptions and report them to Sentry.
+ *
+ * The BaseExceptionFilter does not work well in GraphQL applications.
+ * By default, Nest GraphQL applications use the ExternalExceptionFilter, which just rethrows the error:
+ * https://github.com/nestjs/nest/blob/master/packages/core/exceptions/external-exception-filter.ts
+ *
+ * The ExternalExceptinFilter is not exported, so we reimplement this filter here.
+ */
+class SentryGlobalGraphQLFilter {
+  private static readonly _logger = new Logger('ExceptionsHandler');
+  public readonly __SENTRY_INTERNAL__: boolean;
+
+  public constructor() {
+    this.__SENTRY_INTERNAL__ = true;
+  }
+
+  /**
+   * Catches exceptions and reports them to Sentry unless they are HttpExceptions.
+   */
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  public catch(exception: unknown, host: ArgumentsHost): void {
+    // neither report nor log HttpExceptions
+    if (exception instanceof HttpException) {
+      throw exception;
+    }
+    if (exception instanceof Error) {
+      SentryGlobalGraphQLFilter._logger.error(exception.message, exception.stack);
+    }
+    captureException(exception);
+    throw exception;
+  }
+}
+Catch()(SentryGlobalGraphQLFilter);
+export { SentryGlobalGraphQLFilter };
+
 /**
  * Service to set up Sentry performance tracing for Nest.js applications.
  */
 class SentryService implements OnModuleInit {
-  public static readonly __SENTRY_INTERNAL__ = true;
+  public readonly __SENTRY_INTERNAL__: boolean;
+
+  public constructor() {
+    this.__SENTRY_INTERNAL__ = true;
+  }
 
   /**
    * Initializes the Sentry service and registers span attributes.

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+@sentry:registry=http://127.0.0.1:4873`
	`2`	`+@sentry-internal:registry=http://127.0.0.1:4873`