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 7 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"
}
}
24 changes: 24 additions & 0 deletions packages/nestjs/src/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,30 @@ 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 {
public static readonly __SENTRY_INTERNAL__ = true;
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed this but let's remove the static keyword from this field. Also in all the other places where we have it. We are not using it in a static way and defining a static in JS is considered a side-effect and that becomes problematic in some scenarios. (it inhibits tree shaking for example)


/**
* Catches exceptions and reports them to Sentry.
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
public catch(exception: unknown, host: ArgumentsHost): void {
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.
*/
Expand Down
Loading