Skip to content

Commit

Permalink
feat(node): Support Express v5 (#15380)
Browse files Browse the repository at this point in the history
The goal is for Express to eventually support publishing events to
`diagnostics_channel` (#15107) so that Open Telemetry can instrument it
without monkey-patching internal code. However, this might take a while
and it would be great to support Express v5 now. This PR is a stop-gap
solution until that work is complete and published.

This PR vendors the code added in my otel PR: 
- open-telemetry/opentelemetry-js-contrib#2437 
- Adds a new instrumentation specifically for hooking express v5
- Copies the Express v4 integration tests to test v5
- The only changes in the tests is the removal of a couple of complex
regex tests where the regexes are no longer supported by Express.
- Modifies the NestJs v11 tests which now support full Express spans
  • Loading branch information
timfish authored Feb 20, 2025
1 parent d804dd4 commit 1aa5bbe
Show file tree
Hide file tree
Showing 68 changed files with 3,166 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,11 @@ test('Does not send HttpExceptions to Sentry', async ({ baseURL }) => {
});

const transactionEventPromise400 = waitForTransaction('nestjs-11', transactionEvent => {
// todo(express-5): parametrize /test-expected-400-exception/:id
return transactionEvent?.transaction === 'GET /test-expected-400-exception/123';
return transactionEvent?.transaction === 'GET /test-expected-400-exception/:id';
});

const transactionEventPromise500 = waitForTransaction('nestjs-11', transactionEvent => {
// todo(express-5): parametrize /test-expected-500-exception/:id
return transactionEvent?.transaction === 'GET /test-expected-500-exception/123';
return transactionEvent?.transaction === 'GET /test-expected-500-exception/:id';
});

const response400 = await fetch(`${baseURL}/test-expected-400-exception/123`);
Expand All @@ -81,13 +79,11 @@ test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => {
errorEventOccurred = true;
}

// todo(express-5): parametrize /test-expected-rpc-exception/:id
return event?.transaction === 'GET /test-expected-rpc-exception/123';
return event?.transaction === 'GET /test-expected-rpc-exception/:id';
});

const transactionEventPromise = waitForTransaction('nestjs-11', transactionEvent => {
// todo(express-5): parametrize /test-expected-rpc-exception/:id
return transactionEvent?.transaction === 'GET /test-expected-rpc-exception/123';
return transactionEvent?.transaction === 'GET /test-expected-rpc-exception/:id';
});

const response = await fetch(`${baseURL}/test-expected-rpc-exception/123`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {

expect(transactionEvent.contexts?.trace).toEqual({
data: {
'sentry.source': 'url', // todo(express-5): 'route'
'sentry.source': 'route',
'sentry.origin': 'auto.http.otel.http',
'sentry.op': 'http.server',
'sentry.sample_rate': 1,
Expand All @@ -37,7 +37,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {
'net.peer.port': expect.any(Number),
'http.status_code': 200,
'http.status_text': 'OK',
// 'http.route': '/test-transaction', // todo(express-5): add this line again
'http.route': '/test-transaction',
},
op: 'http.server',
span_id: expect.stringMatching(/[a-f0-9]{16}/),
Expand All @@ -49,7 +49,6 @@ test('Sends an API route transaction', async ({ baseURL }) => {
expect(transactionEvent).toEqual(
expect.objectContaining({
spans: expect.arrayContaining([
/* todo(express-5): add this part again
{
data: {
'express.name': '/test-transaction',
Expand All @@ -67,7 +66,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {
timestamp: expect.any(Number),
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
origin: 'auto.http.otel.express',
}, */
},
{
data: {
'sentry.origin': 'manual',
Expand Down Expand Up @@ -117,7 +116,7 @@ test('Sends an API route transaction', async ({ baseURL }) => {
transaction: 'GET /test-transaction',
type: 'transaction',
transaction_info: {
source: 'url', // todo(express-5): 'route'
source: 'route',
},
}),
);
Expand Down Expand Up @@ -272,8 +271,7 @@ test('API route transaction includes nest pipe span for valid request', async ({
const transactionEventPromise = waitForTransaction('nestjs-11', transactionEvent => {
return (
transactionEvent?.contexts?.trace?.op === 'http.server' &&
// todo(express-5): parametrize test-pipe-instrumentation/:id
transactionEvent?.transaction === 'GET /test-pipe-instrumentation/123' &&
transactionEvent?.transaction === 'GET /test-pipe-instrumentation/:id' &&
transactionEvent?.request?.url?.includes('/test-pipe-instrumentation/123')
);
});
Expand Down Expand Up @@ -310,8 +308,7 @@ test('API route transaction includes nest pipe span for invalid request', async
const transactionEventPromise = waitForTransaction('nestjs-11', transactionEvent => {
return (
transactionEvent?.contexts?.trace?.op === 'http.server' &&
// todo(express-5): parametrize test-pipe-instrumentation/:id
transactionEvent?.transaction === 'GET /test-pipe-instrumentation/abc' &&
transactionEvent?.transaction === 'GET /test-pipe-instrumentation/:id' &&
transactionEvent?.request?.url?.includes('/test-pipe-instrumentation/abc')
);
});
Expand Down
2 changes: 2 additions & 0 deletions dev-packages/node-integration-tests/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
"build:types": "tsc -p tsconfig.types.json",
"clean": "rimraf -g **/node_modules && run-p clean:script",
"clean:script": "node scripts/clean.js",
"express-v5-install": "cd suites/express-v5 && yarn --no-lockfile",
"lint": "eslint . --format stylish",
"fix": "eslint . --format stylish --fix",
"type-check": "tsc",
"pretest": "yarn express-v5-install",
"test": "jest --config ./jest.config.js",
"test:no-prisma": "jest --config ./jest.config.js",
"test:watch": "yarn test --watch"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
transport: loggingTransport,
});

import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
import express from 'express';

const app = express();

Sentry.setTag('global', 'tag');

app.get('/test/withScope', () => {
Sentry.withScope(scope => {
scope.setTag('local', 'tag');
throw new Error('test_error');
});
});

app.get('/test/isolationScope', () => {
Sentry.getIsolationScope().setTag('isolation-scope', 'tag');
throw new Error('isolation_test_error');
});

Sentry.setupExpressErrorHandler(app);

startExpressServerAndSendPortToRunner(app);
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

/**
* Why does this test exist?
*
* We recently discovered that errors caught by global handlers will potentially loose scope data from the active scope
* where the error was originally thrown in. The simple example in this test (see subject.ts) demonstrates this behavior
* (in a Node environment but the same behavior applies to the browser; see the test there).
*
* This test nevertheless covers the behavior so that we're aware.
*/
test('withScope scope is NOT applied to thrown error caught by global handler', done => {
createRunner(__dirname, 'server.ts')
.expect({
event: {
exception: {
values: [
{
mechanism: {
type: 'middleware',
handled: false,
},
type: 'Error',
value: 'test_error',
stacktrace: {
frames: expect.arrayContaining([
expect.objectContaining({
function: expect.any(String),
lineno: expect.any(Number),
colno: expect.any(Number),
}),
]),
},
},
],
},
// 'local' tag is not applied to the event
tags: expect.not.objectContaining({ local: expect.anything() }),
},
})
.start(done)
.makeRequest('get', '/test/withScope', { expectError: true });
});

/**
* This test shows that the isolation scope set tags are applied correctly to the error.
*/
test('isolation scope is applied to thrown error caught by global handler', done => {
createRunner(__dirname, 'server.ts')
.expect({
event: {
exception: {
values: [
{
mechanism: {
type: 'middleware',
handled: false,
},
type: 'Error',
value: 'isolation_test_error',
stacktrace: {
frames: expect.arrayContaining([
expect.objectContaining({
function: expect.any(String),
lineno: expect.any(Number),
colno: expect.any(Number),
}),
]),
},
},
],
},
tags: {
global: 'tag',
'isolation-scope': 'tag',
},
},
})
.start(done)
.makeRequest('get', '/test/isolationScope', { expectError: true });
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
transport: loggingTransport,
tracesSampleRate: 1,
});

import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
import express from 'express';

const app = express();

app.get('/test/express/:id', req => {
throw new Error(`test_error with id ${req.params.id}`);
});

Sentry.setupExpressErrorHandler(app);

startExpressServerAndSendPortToRunner(app);
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

test('should capture and send Express controller error with txn name if tracesSampleRate is 0', done => {
createRunner(__dirname, 'server.ts')
.ignore('transaction')
.expect({
event: {
exception: {
values: [
{
mechanism: {
type: 'middleware',
handled: false,
},
type: 'Error',
value: 'test_error with id 123',
stacktrace: {
frames: expect.arrayContaining([
expect.objectContaining({
function: expect.any(String),
lineno: expect.any(Number),
colno: expect.any(Number),
}),
]),
},
},
],
},
transaction: 'GET /test/express/:id',
},
})
.start(done)
.makeRequest('get', '/test/express/123', { expectError: true });
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://[email protected]/1337',
release: '1.0',
transport: loggingTransport,
});

import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
import express from 'express';

const app = express();

app.get('/test/express/:id', req => {
throw new Error(`test_error with id ${req.params.id}`);
});

Sentry.setupExpressErrorHandler(app);

startExpressServerAndSendPortToRunner(app);
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

test('should capture and send Express controller error if tracesSampleRate is not set.', done => {
createRunner(__dirname, 'server.ts')
.ignore('transaction')
.expect({
event: {
exception: {
values: [
{
mechanism: {
type: 'middleware',
handled: false,
},
type: 'Error',
value: 'test_error with id 123',
stacktrace: {
frames: expect.arrayContaining([
expect.objectContaining({
function: expect.any(String),
lineno: expect.any(Number),
colno: expect.any(Number),
}),
]),
},
},
],
},
},
})
.start(done)
.makeRequest('get', '/test/express/123', { expectError: true });
});
Loading

0 comments on commit 1aa5bbe

Please sign in to comment.