Skip to content

Commit de60306

Browse files
mydeamydea
authored
feat(node): Ensure express spans have better data (#12107)
This ensures we have correct op, name & origin for all express middleware spans. I also updated the E2E test to actually check this as well.
1 parent 5317793 commit de60306

File tree

9 files changed

+202
-53
lines changed

9 files changed

+202
-53
lines changed

‎dev-packages/e2e-tests/test-applications/node-express-esm-loader/tests/server.test.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,10 @@ test('Should record a transaction for route with parameters', async ({ request }
6868
'http.route': '/',
6969
'otel.kind': 'INTERNAL',
7070
'sentry.origin': 'auto.http.otel.express',
71+
'sentry.op': 'middleware.express',
7172
},
72-
description: 'middleware - query',
73+
op: 'middleware.express',
74+
description: 'query',
7375
origin: 'auto.http.otel.express',
7476
parent_span_id: expect.any(String),
7577
span_id: expect.any(String),
@@ -86,8 +88,10 @@ test('Should record a transaction for route with parameters', async ({ request }
8688
'http.route': '/',
8789
'otel.kind': 'INTERNAL',
8890
'sentry.origin': 'auto.http.otel.express',
91+
'sentry.op': 'middleware.express',
8992
},
90-
description: 'middleware - expressInit',
93+
op: 'middleware.express',
94+
description: 'expressInit',
9195
origin: 'auto.http.otel.express',
9296
parent_span_id: expect.any(String),
9397
span_id: expect.any(String),
@@ -104,8 +108,10 @@ test('Should record a transaction for route with parameters', async ({ request }
104108
'http.route': '/test-transaction/:param',
105109
'otel.kind': 'INTERNAL',
106110
'sentry.origin': 'auto.http.otel.express',
111+
'sentry.op': 'request_handler.express',
107112
},
108-
description: 'request handler - /test-transaction/:param',
113+
op: 'request_handler.express',
114+
description: '/test-transaction/:param',
109115
origin: 'auto.http.otel.express',
110116
parent_span_id: expect.any(String),
111117
span_id: expect.any(String),

‎dev-packages/e2e-tests/test-applications/node-express/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
"@types/node": "18.15.1",
2121
"express": "4.19.2",
2222
"typescript": "4.9.5",
23-
"zod": "^3.22.4"
23+
"zod": "~3.22.4"
2424
},
2525
"devDependencies": {
2626
"@sentry-internal/event-proxy-server": "link:../../../event-proxy-server",

‎dev-packages/e2e-tests/test-applications/node-express/tests/error.test.ts renamed to ‎dev-packages/e2e-tests/test-applications/node-express/tests/errors.test.ts

File renamed without changes.

‎dev-packages/e2e-tests/test-applications/node-express/tests/transaction.test.ts

Lines changed: 0 additions & 45 deletions
This file was deleted.

‎dev-packages/e2e-tests/test-applications/node-express/tests/transactions.test.ts

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
import { expect, test } from '@playwright/test';
2+
import { waitForTransaction } from '@sentry-internal/event-proxy-server';
3+
import axios, { AxiosError } from 'axios';
4+
5+
const authToken = process.env.E2E_TEST_AUTH_TOKEN;
6+
const sentryTestOrgSlug = process.env.E2E_TEST_SENTRY_ORG_SLUG;
7+
const sentryTestProject = process.env.E2E_TEST_SENTRY_TEST_PROJECT;
8+
const EVENT_POLLING_TIMEOUT = 90_000;
9+
10+
test('Sends an API route transaction', async ({ baseURL }) => {
11+
const pageloadTransactionEventPromise = waitForTransaction('node-express', transactionEvent => {
12+
return (
13+
transactionEvent?.contexts?.trace?.op === 'http.server' &&
14+
transactionEvent?.transaction === 'GET /test-transaction'
15+
);
16+
});
17+
18+
await axios.get(`${baseURL}/test-transaction`);
19+
20+
const transactionEvent = await pageloadTransactionEventPromise;
21+
const transactionEventId = transactionEvent.event_id;
22+
23+
expect(transactionEvent.contexts?.trace).toEqual({
24+
data: {
25+
'sentry.source': 'route',
26+
'sentry.origin': 'auto.http.otel.http',
27+
'sentry.op': 'http.server',
28+
'sentry.sample_rate': 1,
29+
url: 'http://localhost:3030/test-transaction',
30+
'otel.kind': 'SERVER',
31+
'http.response.status_code': 200,
32+
'http.url': 'http://localhost:3030/test-transaction',
33+
'http.host': 'localhost:3030',
34+
'net.host.name': 'localhost',
35+
'http.method': 'GET',
36+
'http.scheme': 'http',
37+
'http.target': '/test-transaction',
38+
'http.user_agent': 'axios/1.6.7',
39+
'http.flavor': '1.1',
40+
'net.transport': 'ip_tcp',
41+
'net.host.ip': expect.any(String),
42+
'net.host.port': expect.any(Number),
43+
'net.peer.ip': expect.any(String),
44+
'net.peer.port': expect.any(Number),
45+
'http.status_code': 200,
46+
'http.status_text': 'OK',
47+
'http.route': '/test-transaction',
48+
},
49+
op: 'http.server',
50+
span_id: expect.any(String),
51+
status: 'ok',
52+
trace_id: expect.any(String),
53+
origin: 'auto.http.otel.http',
54+
});
55+
56+
expect(transactionEvent).toEqual(
57+
expect.objectContaining({
58+
transaction: 'GET /test-transaction',
59+
type: 'transaction',
60+
transaction_info: {
61+
source: 'route',
62+
},
63+
}),
64+
);
65+
66+
const spans = transactionEvent.spans || [];
67+
68+
expect(spans).toContainEqual({
69+
data: {
70+
'sentry.origin': 'auto.http.otel.express',
71+
'sentry.op': 'middleware.express',
72+
'http.route': '/',
73+
'express.name': 'query',
74+
'express.type': 'middleware',
75+
'otel.kind': 'INTERNAL',
76+
},
77+
description: 'query',
78+
op: 'middleware.express',
79+
origin: 'auto.http.otel.express',
80+
parent_span_id: expect.any(String),
81+
span_id: expect.any(String),
82+
start_timestamp: expect.any(Number),
83+
status: 'ok',
84+
timestamp: expect.any(Number),
85+
trace_id: expect.any(String),
86+
});
87+
88+
expect(spans).toContainEqual({
89+
data: {
90+
'sentry.origin': 'auto.http.otel.express',
91+
'sentry.op': 'middleware.express',
92+
'http.route': '/',
93+
'express.name': 'expressInit',
94+
'express.type': 'middleware',
95+
'otel.kind': 'INTERNAL',
96+
},
97+
description: 'expressInit',
98+
op: 'middleware.express',
99+
origin: 'auto.http.otel.express',
100+
parent_span_id: expect.any(String),
101+
span_id: expect.any(String),
102+
start_timestamp: expect.any(Number),
103+
status: 'ok',
104+
timestamp: expect.any(Number),
105+
trace_id: expect.any(String),
106+
});
107+
108+
expect(spans).toContainEqual({
109+
data: {
110+
'sentry.origin': 'auto.http.otel.express',
111+
'sentry.op': 'request_handler.express',
112+
'http.route': '/test-transaction',
113+
'express.name': '/test-transaction',
114+
'express.type': 'request_handler',
115+
'otel.kind': 'INTERNAL',
116+
},
117+
description: '/test-transaction',
118+
op: 'request_handler.express',
119+
origin: 'auto.http.otel.express',
120+
parent_span_id: expect.any(String),
121+
span_id: expect.any(String),
122+
start_timestamp: expect.any(Number),
123+
status: 'ok',
124+
timestamp: expect.any(Number),
125+
trace_id: expect.any(String),
126+
});
127+
128+
await expect
129+
.poll(
130+
async () => {
131+
try {
132+
const response = await axios.get(
133+
`https://sentry.io/api/0/projects/${sentryTestOrgSlug}/${sentryTestProject}/events/${transactionEventId}/`,
134+
{ headers: { Authorization: `Bearer ${authToken}` } },
135+
);
136+
137+
return response.status;
138+
} catch (e) {
139+
if (e instanceof AxiosError && e.response) {
140+
if (e.response.status !== 404) {
141+
throw e;
142+
} else {
143+
return e.response.status;
144+
}
145+
} else {
146+
throw e;
147+
}
148+
}
149+
},
150+
{
151+
timeout: EVENT_POLLING_TIMEOUT,
152+
},
153+
)
154+
.toBe(200);
155+
});

‎dev-packages/e2e-tests/test-applications/node-nestjs/tests/transactions.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,10 @@ test('Sends an API route transaction', async ({ baseURL }) => {
6363
'http.route': '/test-transaction',
6464
'otel.kind': 'INTERNAL',
6565
'sentry.origin': 'auto.http.otel.express',
66+
'sentry.op': 'request_handler.express',
6667
},
67-
description: 'request handler - /test-transaction',
68+
op: 'request_handler.express',
69+
description: '/test-transaction',
6870
parent_span_id: expect.any(String),
6971
span_id: expect.any(String),
7072
start_timestamp: expect.any(Number),

‎dev-packages/node-integration-tests/suites/express/tracing/test.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,17 @@ describe('express tracing experimental', () => {
2929
'express.name': 'corsMiddleware',
3030
'express.type': 'middleware',
3131
}),
32-
description: 'middleware - corsMiddleware',
32+
description: 'corsMiddleware',
33+
op: 'middleware.express',
34+
origin: 'auto.http.otel.express',
35+
}),
36+
expect.objectContaining({
37+
data: expect.objectContaining({
38+
'express.name': '/test/express',
39+
'express.type': 'request_handler',
40+
}),
41+
description: '/test/express',
42+
op: 'request_handler.express',
3343
origin: 'auto.http.otel.express',
3444
}),
3545
]),

‎package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010
"build:watch": "lerna run build:watch",
1111
"build:dev:watch": "lerna run build:dev:watch",
1212
"build:types:watch": "ts-node scripts/build-types-watch.ts",
13-
"build:tarball": "lerna run build:tarball",
13+
"build:tarball": "run-s clean:tarballs build:tarballs",
14+
"build:tarballs": "lerna run build:tarball",
1415
"circularDepCheck": "lerna run circularDepCheck",
1516
"clean": "run-s clean:build clean:caches",
1617
"clean:build": "lerna run clean",

‎packages/node/src/integrations/tracing/express.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
import type * as http from 'http';
22
import { ExpressInstrumentation } from '@opentelemetry/instrumentation-express';
3-
import { defineIntegration, getDefaultIsolationScope, isEnabled } from '@sentry/core';
3+
import {
4+
SEMANTIC_ATTRIBUTE_SENTRY_OP,
5+
defineIntegration,
6+
getDefaultIsolationScope,
7+
isEnabled,
8+
spanToJSON,
9+
} from '@sentry/core';
410
import { captureException, getClient, getIsolationScope } from '@sentry/core';
511
import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry';
612
import type { IntegrationFn } from '@sentry/types';
@@ -19,6 +25,20 @@ const _expressIntegration = (() => {
1925
new ExpressInstrumentation({
2026
requestHook(span) {
2127
addOriginToSpan(span, 'auto.http.otel.express');
28+
29+
const attributes = spanToJSON(span).data || {};
30+
// this is one of: middleware, request_handler, router
31+
const type = attributes['express.type'];
32+
33+
if (type) {
34+
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, `${type}.express`);
35+
}
36+
37+
// Also update the name, we don't need to "middleware - " prefix
38+
const name = attributes['express.name'];
39+
if (typeof name === 'string') {
40+
span.updateName(name);
41+
}
2242
},
2343
spanNameHook(info, defaultName) {
2444
if (getIsolationScope() === getDefaultIsolationScope()) {

0 commit comments

Comments
 (0)