Skip to content
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

feat(nestjs): Add SentryGlobalGraphQLFilter #13545

Merged
merged 11 commits into from
Sep 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,7 @@ jobs:
'nestjs-basic',
'nestjs-distributed-tracing',
'nestjs-with-submodules',
'nestjs-graphql',
'node-exports-test-app',
'node-koa',
'node-connect',
Expand Down
56 changes: 56 additions & 0 deletions dev-packages/e2e-tests/test-applications/nestjs-graphql/.gitignore
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@sentry:registry=http://127.0.0.1:4873
@sentry-internal:registry=http://127.0.0.1:4873
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"$schema": "https://json.schemastore.org/nest-cli",
"collection": "@nestjs/schematics",
"sourceRoot": "src",
"compilerOptions": {
"deleteOutDir": true
}
}
Original file line number Diff line number Diff line change
@@ -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"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { getPlaywrightConfig } from '@sentry-internal/test-utils';

const config = getPlaywrightConfig({
startCommand: `pnpm start`,
});

export default config;
Original file line number Diff line number Diff line change
@@ -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 {}
Original file line number Diff line number Diff line change
@@ -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!');
}
}
Original file line number Diff line number Diff line change
@@ -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,
});
Original file line number Diff line number Diff line change
@@ -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();
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { startEventProxyServer } from '@sentry-internal/test-utils';

startEventProxyServer({
port: 3031,
proxyServerName: 'nestjs-graphql',
});
Original file line number Diff line number Diff line change
@@ -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),
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"extends": "./tsconfig.json",
"exclude": ["node_modules", "test", "dist"]
}
Original file line number Diff line number Diff line change
@@ -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"
}
}
57 changes: 53 additions & 4 deletions packages/nestjs/src/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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;
Comment on lines +119 to +120
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: I am a bit sus because we identified previously for HTTP contexts taht throwing in error filters will not make the behaviour transparent. Is this the case for graphql?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be fine, the response from the test error endpoint looks like this:

{
  errors: [
    {
      message: 'This is an exception!',
      locations: [Array],
      path: [Array],
      extensions: [Object]
    }
  ],
  data: null
}

Copy link
Member

@lforst lforst Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check what the server returns before/after you added the filter? We need to be careful not to modify any responses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, responses are the same with/without filter. I also now added the logging logic from the ExtenalExceptionFilter so now output also aligns perfectly.

}
}
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.
Expand Down
Loading