8000 fix(node): Ensure fetch/http breadcrumbs are created correctly (#12137) · benjick/sentry-javascript@a7aa8a0 · GitHub
[go: up one dir, main page]
Skip to content 8000

Commit a7aa8a0

Browse files
mydeamydea
authored
fix(node): Ensure fetch/http breadcrumbs are created correctly (getsentry#12137)
This PR ensures that (node) fetch and http requests generate breadcrumbs correctly, even if tracing is disabled. It also adds tests for this, and ensures we pass the request/response to the hint correctly. Fixes getsentry#12132
1 parent a30fbbc commit a7aa8a0

File tree

6 files changed

+351
-37
lines changed

6 files changed

+351
-37
lines changed

‎dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/scenario.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import { loggingTransport } from '@sentry-internal/node-integration-tests';
2+
import * as Sentry from '@sentry/node';
3+
4+
Sentry.init({
5+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
6+
release: '1.0',
7+
tracePropagationTargets: [/\/v0/, 'v1'],
8+
integrations: [],
9+
transport: loggingTransport,
10+
// Ensure this gets a correct hint
11+
beforeBreadcrumb(breadcrumb, hint) {
12+
breadcrumb.data = breadcrumb.data || {};
13+
const req = hint?.request as { path?: string };
14+
breadcrumb.data.ADDED_PATH = req?.path;
15+
return breadcrumb;
16+
},
17+
});
18+
19+
async function run(): Promise<void> {
20+
Sentry.addBreadcrumb({ message: 'manual breadcrumb' });
21+
22+
// Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
23+
await new Promise(resolve => setTimeout(resolve, 100));
24+
await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
25+
await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
26+
await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
27+
await fetch(`${process.env.SERVER_URL}/api/v3`).then(res => res.text());
28+
29+
Sentry.captureException(new Error('foo'));
30+
}
31+
32+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
33+
run();

‎dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/test.ts

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
import { conditionalTest } from '../../../../utils';
2+
import { createRunner } from '../../../../utils/runner';
3+
import { createTestServer } from '../../../../utils/server';
4+
5+
conditionalTest({ min: 18 })('outgoing fetch', () => {
6+
test('outgoing fetch requests create breadcrumbs', done => {
7+
createTestServer(done)
8+
.start()
9+
.then(SERVER_URL => {
10+
createRunner(__dirname, 'scenario.ts')
11+
.withEnv({ SERVER_URL })
12+
.ensureNoErrorOutput()
13+
.ignore('session', 'sessions')
14+
.expect({
15+
event: {
16+
breadcrumbs: [
17+
{
18+
message: 'manual breadcrumb',
19+
timestamp: expect.any(Number),
20+
},
21+
{
22+
category: 'http',
23+
data: {
24+
'http.method': 'GET',
25+
url: `${SERVER_URL}/api/v0`,
26+
status_code: 404,
27+
ADDED_PATH: '/api/v0',
28+
},
29+
timestamp: expect.any(Number),
30+
type: 'http',
31+
},
32+
{
33+
category: 'http',
34+
data: {
35+
'http.method': 'GET',
36+
url: `${SERVER_URL}/api/v1`,
37+
status_code: 404,
38+
ADDED_PATH: '/api/v1',
39+
},
40+
timestamp: expect.any(Number),
41+
type: 'http',
42+
},
43+
{
44+
category: 'http',
45+
data: {
46+
'http.method': 'GET',
47+
url: `${SERVER_URL}/api/v2`,
48+
status_code: 404,
49+
ADDED_PATH: '/api/v2',
50+
},
51+
timestamp: expect.any(Number),
52+
type: 'http',
53+
},
54+
{
55+
category: 'http',
56+
data: {
57+
'http.method': 'GET',
58+
url: `${SERVER_URL}/api/v3`,
59+
status_code: 404,
60+
ADDED_PATH: '/api/v3',
61+
},
62+
timestamp: expect.any(Number),
63+
type: 'http',
64+
},
65+
],
66+
exception: {
67+
values: [
68+
{
69+
type: 'Error',
70+
value: 'foo',
71+
},
72+
],
73+
},
74+
},
75+
})
76+
.start(done);
77+
});
78+
});
79+
});

‎dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/scenario.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import { loggingTransport } from '@sentry-internal/node-integration-tests';
2+
import * as Sentry from '@sentry/node';
3+
4+
Sentry.init({
5+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
6+
release: '1.0',
7+
tracePropagationTargets: [/\/v0/, 'v1'],
8+
integrations: [],
9+
transport: loggingTransport,
10+
// Ensure this gets a correct hint
11+
beforeBreadcrumb(breadcrumb, hint) {
12+
breadcrumb.data = breadcrumb.data || {};
13+
const req = hint?.request as { path?: string };
14+
breadcrumb.data.ADDED_PATH = req?.path;
15+
return breadcrumb;
16+
},
17+
});
18+
19+
import * as http from 'http';
20+
21+
async function run(): Promise<void> {
22+
Sentry.addBreadcrumb({ message: 'manual breadcrumb' });
23+
24+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`);
25+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v1`);
26+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`);
27+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`);
28+
29+
Sentry.captureException(new Error('foo'));
30+
}
31+
32+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
33+
run();
34+
35+
function makeHttpRequest(url: string): Promise<void> {
36+
return new Promise<void>(resolve => {
37+
http
38+
.request(url, httpRes => {
39+
httpRes.on('data', () => {
40+
// we don't care about data
41+
});
42+
httpRes.on('end', () => {
43+
resolve();
44+
});
45+
})
46+
.end();
47+
});
48+
}

‎dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/test.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { createRunner } from '../../../../utils/runner';
2+
import { createTestServer } from '../../../../utils/server';
3+
4+
test('outgoing http requests create breadcrumbs', done => {
5+
createTestServer(done)
6+
.start()
7+
.then(SERVER_URL => {
8+
createRunner(__dirname, 'scenario.ts')
9+
.withEnv({ SERVER_URL })
10+
.ensureNoErrorOutput()
11+
.ignore('session', 'sessions')
12+
.expect({
13+
event: {
14+
breadcrumbs: [
15+
{
16+
message: 'manual breadcrumb',
17+
timestamp: expect.any(Number),
18+
},
19+
{
20+
category: 'http',
21+
data: {
22+
'http.method': 'GET',
23+
url: `${SERVER_URL}/api/v0`,
24+
status_code: 404,
25+
ADDED_PATH: '/api/v0',
26+
},
27+
timestamp: expect.any(Number),
28+
type: 'http',
29+
},
30+
{
31+
category: 'http',
32+
data: {
33+
'http.method': 'GET',
34+
url: `${SERVER_URL}/api/v1`,
35+
status_code: 404,
36+
ADDED_PATH: '/api/v1',
37+
},
38+
timestamp: expect.any(Number),
39+
type: 'http',
40+
},
41+
{
42+
category: 'http',
43+
data: {
44+
'http.method': 'GET',
45+
url: `${SERVER_URL}/api/v2`,
46+
status_code: 404,
47+
ADDED_PATH: '/api/v2',
48+
},
49+
timestamp: expect.any(Number),
50+
type: 'http',
51+
},
52+
{
53+
category: 'http',
54+
data: {
55+
'http.method': 'GET',
56+
url: `${SERVER_URL}/api/v3`,
57+
status_code: 404,
58+
ADDED_PATH: '/api/v3',
59+
},
60+
timestamp: expect.any(Number),
61+
type: 'http',
62+
},
63+
],
64+
exception: {
65+
values: [
66+
{
67+
type: 'Error',
68+
value: 'foo',
69+
},
70+
],
71+
},
72+
},
73+
})
74+
.start(done);
75+
});
76+
});

‎packages/node/src/integrations/http.ts

Lines changed: 48 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
import type { ClientRequest, IncomingMessage, ServerResponse } from 'node:http';
1+
import type { ClientRequest, ServerResponse } from 'node:http';
22
import type { Span } from '@opentelemetry/api';
3-
import { SpanKind } from '@opentelemetry/api';
43
import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
54
import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry';
65

@@ -13,10 +12,10 @@ import {
1312
isSentryRequestUrl,
1413
setCapturedScopesOnSpan,
1514
} from '@sentry/core';
16-
import { getClient, getRequestSpanData, getSpanKind } from '@sentry/opentelemetry';
17-
import type { IntegrationFn } from '@sentry/types';
15+
import { getClient } from '@sentry/opentelemetry';
16+
import type { IntegrationFn, SanitizedRequestData } from '@sentry/types';
1817

19-
import { stripUrlQueryAndFragment } from '@sentry/utils';
18+
import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils';
2019
import type { NodeClient } from '../sdk/client';
2120
import { setIsolationScope } from '../sdk/scope';
2221
import type { HTTPModuleRequestIncomingMessage } from '../transports/http-module';
@@ -127,18 +126,23 @@ const _httpIntegration = ((options: HttpOptions = {}) => {
127126

128127
isolationScope.setTransactionName(bestEffortTransactionName);
129128
},
130-
responseHook: (span, res) => {
131-
if (_breadcrumbs) {
132-
_addRequestBreadcrumb(span, res);
133-
}
134-
129+
responseHook: () => {
135130
const client = getClient<NodeClient>();
136131
if (client && client.getOptions().autoSessionTracking) {
137132
setImmediate(() => {
138133
client['_captureRequestSession']();
139134
});
140135
}
141136
},
137+
applyCustomAttributesOnSpan: (
138+
_span: Span,
139+
request: ClientRequest | HTTPModuleRequestIncomingMessage,
140+
response: HTTPModuleRequestIncomingMessage | ServerResponse,
141+
) => {
142+
if (_breadcrumbs) {
143+
_addRequestBreadcrumb(request, response);
144+
}
145+
},
142146
}),
143147
);
144148
},
@@ -152,12 +156,16 @@ const _httpIntegration = ((options: HttpOptions = {}) => {
152156
export const httpIntegration = defineIntegration(_httpIntegration);
153157

154158
/** Add a breadcrumb for outgoing requests. */
155-
function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMessage | ServerResponse): void {
156-
if (getSpanKind(span) !== SpanKind.CLIENT) {
159+
function _addRequestBreadcrumb(
160+
request: ClientRequest | HTTPModuleRequestIncomingMessage,
161+
response: HTTPModuleRequestIncomingMessage | ServerResponse,
162+
): void {
163+
// Only generate breadcrumbs for outgoing requests
164+
if (!_isClientRequest(request)) {
157165
return;
158166
}
159167

160-
const data = getRequestSpanData(span);
168+
const data = getBreadcrumbData(request);
161169
addBreadcrumb(
162170
{
163171
category: 'http',
@@ -169,19 +177,42 @@ function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMe
169177
},
170178
{
171179
event: 'response',
172-
// TODO FN: Do we need access to `request` here?
173-
// If we do, we'll have to use the `applyCustomAttributesOnSpan` hook instead,
174-
// but this has worse context semantics than request/responseHook.
180+
request,
175181
response,
176182
},
177183
);
178184
}
179185

186+
function getBreadcrumbData(request: ClientRequest): Partial<SanitizedRequestData> {
187+
try {
188+
// `request.host` does not contain the port, but the host header does
189+
const host = request.getHeader('host') || request.host;
190+
const url = new URL(request.path, `${request.protocol}//${host}`);
191+
const parsedUrl = parseUrl(url.toString());
192+
193+
const data: Partial<SanitizedRequestData> = {
194+
url: getSanitizedUrlString(parsedUrl),
195+
'http.method': request.method || 'GET',
196+
};
197+
198+
if (parsedUrl.search) {
199+
data['http.query'] = parsedUrl.search;
200+
}
201+
if (parsedUrl.hash) {
202+
data['http.fragment'] = parsedUrl.hash;
203+
}
204+
205+
return data;
206+
} catch {
207+
return {};
208+
}
209+
}
210+
180211
/**
181212
* Determines if @param req is a ClientRequest, meaning the request was created within the express app
182213
* and it's an outgoing request.
183214
* Checking for properties instead of using `instanceOf` to avoid importing the request classes.
184215
*/
185-
function _isClientRequest(req: ClientRequest | IncomingMessage): req is ClientRequest {
216+
function _isClientRequest(req: ClientRequest | HTTPModuleRequestIncomingMessage): req is ClientRequest {
186217
return 'outputData' in req && 'outputSize' in req && !('client' in req) && !('statusCode' in req);
187218
}

0 commit comments

Comments
 (0)
0