Skip to content

Commit 9d659a5

Browse files
mydeaAbhiPrasadandreiborza
authored
fix(node): Ensure traces are propagated without spans in Node 22+ (#16221)
Today, if the `httpIntegration` is used without spans (e.g. in a custom OTEL setup or when setting `httpIntegration({ spans: false })` manually), outgoing requests will not have traces propagated. This PR fixes this by using the `http.client.request.created` diagnostics channel to add the trace headers in this scenario. However, sadly this channel was only added in Node 22, so it does not work on versions before that. I suppose this is still worth adding because it is better than what we have today (which is that it does not work at all). We may think about making this work for Node <22, but this would require us monkey patching http again, which we really do not want to do... Also note that as of now this should not really affect the vast majority of cases, as unless you specifically opt out of spans today this will always work as we always add the otel http instrumentation by default. And in custom otel setups, users will usually have this set up anyhow. (Also, fetch works in all environments as expected). If we want to, in a follow up, avoid adding the otel spans instrumentation if performance is disabled, we need to think about this... an option could be to always adding the instrumentation on node <22, and only skip it on Node 22+. But this can be looked at separately, this PR at least makes things better than before on Node 22+... This supersedes #15735 --------- Co-authored-by: Abhijeet Prasad <[email protected]> Co-authored-by: Andrei <[email protected]>
1 parent 02f280c commit 9d659a5

File tree

5 files changed

+340
-0
lines changed

5 files changed

+340
-0
lines changed

‎dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/instrument.mjs

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import * as Sentry from '@sentry/node';
2+
import { loggingTransport } from '@sentry-internal/node-integration-tests';
3+
4+
Sentry.init({
5+
dsn: 'https://[email protected]/1337',
6+
release: '1.0',
7+
tracePropagationTargets: [/\/v0/, 'v1'],
8+
integrations: [Sentry.httpIntegration({ spans: false })],
9+
transport: loggingTransport,
10+
// Ensure this gets a correct hint
11+
beforeBreadcrumb(breadcrumb, hint) {
12+
breadcrumb.data = breadcrumb.data || {};
13+
const req = hint?.request;
14+
breadcrumb.data.ADDED_PATH = req?.path;
15+
return breadcrumb;
16+
},
17+
});

‎dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.mjs

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import * as Sentry from '@sentry/node';
2+
import * as http from 'http';
3+
4+
async function run() {
5+
Sentry.addBreadcrumb({ message: 'manual breadcrumb' });
6+
7+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`);
8+
await makeHttpGet(`${process.env.SERVER_URL}/api/v1`);
9+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`);
10+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`);
11+
12+
Sentry.captureException(new Error('foo'));
13+
}
14+
15+
run();
16+
17+
function makeHttpRequest(url) {
18+
return new Promise(resolve => {
19+
http
20+
.request(url, httpRes => {
21+
httpRes.on('data', () => {
22+
// we don't care about data
23+
});
24+
httpRes.on('end', () => {
25+
resolve();
26+
});
27+
})
28+
.end();
29+
});
30+
}
31+
32+
function makeHttpGet(url) {
33+
return new Promise(resolve => {
34+
http.get(url, httpRes => {
35+
httpRes.on('data', () => {
36+
// we don't care about data
37+
});
38+
httpRes.on('end', () => {
39+
resolve();
40+
});
41+
});
42+
});
43+
}

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

+204
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
import { describe, expect } from 'vitest';
2+
import { conditionalTest } from '../../../../utils';
3+
import { createEsmAndCjsTests } from '../../../../utils/runner';
4+
import { createTestServer } from '../../../../utils/server';
5+
6+
describe('outgoing http requests with tracing & spans disabled', () => {
7+
createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => {
8+
conditionalTest({ min: 22 })('node >=22', () => {
9+
test('outgoing http requests are correctly instrumented with tracing & spans disabled', async () => {
10+
expect.assertions(11);
11+
12+
const [SERVER_URL, closeTestServer] = await createTestServer()
13+
.get('/api/v0', headers => {
14+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/));
15+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000');
16+
expect(headers['baggage']).toEqual(expect.any(String));
17+
})
18+
.get('/api/v1', headers => {
19+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/));
20+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000');
21+
expect(headers['baggage']).toEqual(expect.any(String));
22+
})
23+
.get('/api/v2', headers => {
24+
expect(headers['baggage']).toBeUndefined();
25+
expect(headers['sentry-trace']).toBeUndefined();
26+
})
27+
.get('/api/v3', headers => {
28+
expect(headers['baggage']).toBeUndefined();
29+
expect(headers['sentry-trace']).toBeUndefined();
30+
})
31+
.start();
32+
33+
await createRunner()
34+
.withEnv({ SERVER_URL })
35+
.ensureNoErrorOutput()
36+
.expect({
37+
event: {
38+
exception: {
39+
values: [
40+
{
41+
type: 'Error',
42+
value: 'foo',
43+
},
44+
],
45+
},
46+
breadcrumbs: [
47+
{
48+
message: 'manual breadcrumb',
49+
timestamp: expect.any(Number),
50+
},
51+
{
52+
category: 'http',
53+
data: {
54+
'http.method': 'GET',
55+
url: `${SERVER_URL}/api/v0`,
56+
status_code: 200,
57+
ADDED_PATH: '/api/v0',
58+
},
59+
timestamp: expect.any(Number),
60+
type: 'http',
61+
},
62+
{
63+
category: 'http',
64+
data: {
65+
'http.method': 'GET',
66+
url: `${SERVER_URL}/api/v1`,
67+
status_code: 200,
68+
ADDED_PATH: '/api/v1',
69+
},
70+
timestamp: expect.any(Number),
71+
type: 'http',
72+
},
73+
{
74+
category: 'http',
75+
data: {
76+
'http.method': 'GET',
77+
url: `${SERVER_URL}/api/v2`,
78+
status_code: 200,
79+
ADDED_PATH: '/api/v2',
80+
},
81+
timestamp: expect.any(Number),
82+
type: 'http',
83+
},
84+
{
85+
category: 'http',
86+
data: {
87+
'http.method': 'GET',
88+
url: `${SERVER_URL}/api/v3`,
89+
status_code: 200,
90+
ADDED_PATH: '/api/v3',
91+
},
92+
timestamp: expect.any(Number),
93+
type: 'http',
94+
},
95+
],
96+
},
97+
})
98+
.start()
99+
.completed();
100+
101+
closeTestServer();
102+
});
103+
});
104+
105+
// On older node versions, outgoing requests do not get trace-headers injected, sadly
106+
// This is because the necessary diagnostics channel hook is not available yet
107+
conditionalTest({ max: 21 })('node <22', () => {
108+
test('outgoing http requests generate breadcrumbs correctly with tracing & spans disabled', async () => {
109+
expect.assertions(9);
110+
111+
const [SERVER_URL, closeTestServer] = await createTestServer()
112+
.get('/api/v0', headers => {
113+
// This is not instrumented, sadly
114+
expect(headers['baggage']).toBeUndefined();
115+
expect(headers['sentry-trace']).toBeUndefined();
116+
})
117+
.get('/api/v1', headers => {
118+
// This is not instrumented, sadly
119+
expect(headers['baggage']).toBeUndefined();
120+
expect(headers['sentry-trace']).toBeUndefined();
121+
})
122+
.get('/api/v2', headers => {
123+
expect(headers['baggage']).toBeUndefined();
124+
expect(headers['sentry-trace']).toBeUndefined();
125+
})
126+
.get('/api/v3', headers => {
127+
expect(headers['baggage']).toBeUndefined();
128+
expect(headers['sentry-trace']).toBeUndefined();
129+
})
130+
.start();
131+
132+
await createRunner()
133+
.withEnv({ SERVER_URL })
134+
.ensureNoErrorOutput()
135+
.expect({
136+
event: {
137+
exception: {
138+
values: [
139+
{
140+
type: 'Error',
141+
value: 'foo',
142+
},
143+
],
144+
},
145+
breadcrumbs: [
146+
{
147+
message: 'manual breadcrumb',
148+
timestamp: expect.any(Number),
149+
},
150+
{
151+
category: 'http',
152+
data: {
153+
'http.method': 'GET',
154+
url: `${SERVER_URL}/api/v0`,
155+
status_code: 200,
156+
ADDED_PATH: '/api/v0',
157+
},
158+
timestamp: expect.any(Number),
159+
type: 'http',
160+
},
161+
{
162+
category: 'http',
163+
data: {
164+
'http.method': 'GET',
165+
url: `${SERVER_URL}/api/v1`,
166+
status_code: 200,
167+
ADDED_PATH: '/api/v1',
168+
},
169+
timestamp: expect.any(Number),
170+
type: 'http',
171+
},
172+
{
173+
category: 'http',
174+
data: {
175+
'http.method': 'GET',
176+
url: `${SERVER_URL}/api/v2`,
177+
status_code: 200,
178+
ADDED_PATH: '/api/v2',
179+
},
180+
timestamp: expect.any(Number),
181+
type: 'http',
182+
},
183+
{
184+
category: 'http',
185+
data: {
186+
'http.method': 'GET',
187+
url: `${SERVER_URL}/api/v3`,
188+
status_code: 200,
189+
ADDED_PATH: '/api/v3',
190+
},
191+
timestamp: expect.any(Number),
192+
type: 'http',
193+
},
194+
],
195+
},
196+
})
197+
.start()
198+
.completed();
199+
200+
closeTestServer();
201+
});
202+
});
203+
});
204+
});

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

+73
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,17 @@ import {
1818
getCurrentScope,
1919
getIsolationScope,
2020
getSanitizedUrlString,
21+
getTraceData,
2122
httpRequestToRequestData,
2223
logger,
24+
LRUMap,
2325
parseUrl,
2426
stripUrlQueryAndFragment,
2527
withIsolationScope,
2628
} from '@sentry/core';
29+
import { shouldPropagateTraceForUrl } from '@sentry/opentelemetry';
2730
import { DEBUG_BUILD } from '../../debug-build';
31+
import { mergeBaggageHeaders } from '../../utils/baggage';
2832
import { getRequestUrl } from '../../utils/getRequestUrl';
2933

3034
const INSTRUMENTATION_NAME = '@sentry/instrumentation-http';
@@ -49,6 +53,15 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & {
4953
*/
5054
extractIncomingTraceFromHeader?: boolean;
5155

56+
/**
57+
* Whether to propagate Sentry trace headers in outgoing requests.
58+
* By default this is done by the HttpInstrumentation, but if that is not added (e.g. because tracing is disabled)
59+
* then this instrumentation can take over.
60+
*
61+
* @default `false`
62+
*/
63+
propagateTraceInOutgoingRequests?: boolean;
64+
5265
/**
5366
* Do not capture breadcrumbs for outgoing HTTP requests to URLs where the given callback returns `true`.
5467
* For the scope of this instrumentation, this callback only controls breadcrumb creation.
@@ -102,8 +115,12 @@ const MAX_BODY_BYTE_LENGTH = 1024 * 1024;
102115
* https://github.com/open-telemetry/opentelemetry-js/blob/f8ab5592ddea5cba0a3b33bf8d74f27872c0367f/experimental/packages/opentelemetry-instrumentation-http/src/http.ts
103116
*/
104117
export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpInstrumentationOptions> {
118+
private _propagationDecisionMap: LRUMap<string, boolean>;
119+
105120
public constructor(config: SentryHttpInstrumentationOptions = {}) {
106121
super(INSTRUMENTATION_NAME, VERSION, config);
122+
123+
this._propagationDecisionMap = new LRUMap<string, boolean>(100);
107124
}
108125

109126
/** @inheritdoc */
@@ -127,6 +144,11 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
127144
this._onOutgoingRequestFinish(data.request, undefined);
128145
}) satisfies ChannelListener;
129146

147+
const onHttpClientRequestCreated = ((_data: unknown) => {
148+
const data = _data as { request: http.ClientRequest };
149+
this._onOutgoingRequestCreated(data.request);
150+
}) satisfies ChannelListener;
151+
130152
/**
131153
* You may be wondering why we register these diagnostics-channel listeners
132154
* in such a convoluted way (as InstrumentationNodeModuleDefinition...)˝,
@@ -153,12 +175,20 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
153175
// In this case, `http.client.response.finish` is not triggered
154176
subscribe('http.client.request.error', onHttpClientRequestError);
155177

178+
// NOTE: This channel only exist since Node 23
179+
// Before that, outgoing requests are not patched
180+
// and trace headers are not propagated, sadly.
181+
if (this.getConfig().propagateTraceInOutgoingRequests) {
182+
subscribe('http.client.request.created', onHttpClientRequestCreated);
183+
}
184+
156185
return moduleExports;
157186
},
158187
() => {
159188
unsubscribe('http.server.request.start', onHttpServerRequestStart);
160189
unsubscribe('http.client.response.finish', onHttpClientResponseFinish);
161190
unsubscribe('http.client.request.error', onHttpClientRequestError);
191+
unsubscribe('http.client.request.created', onHttpClientRequestCreated);
162192
},
163193
),
164194
new InstrumentationNodeModuleDefinition(
@@ -209,6 +239,49 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
209239
}
210240
}
211241

242+
/**
243+
* This is triggered when an outgoing request is created.
244+
* It has access to the request object, and can mutate it before the request is sent.
245+
*/
246+
private _onOutgoingRequestCreated(request: http.ClientRequest): void {
247+
const url = getRequestUrl(request);
248+
const ignoreOutgoingRequests = this.getConfig().ignoreOutgoingRequests;
249+
const shouldPropagate =
250+
typeof ignoreOutgoingRequests === 'function' ? !ignoreOutgoingRequests(url, getRequestOptions(request)) : true;
251+
252+
if (!shouldPropagate) {
253+
return;
254+
}
255+
256+
// Manually add the trace headers, if it applies
257+
// Note: We do not use `propagation.inject()` here, because our propagator relies on an active span
258+
// Which we do not have in this case
259+
const tracePropagationTargets = getClient()?.getOptions().tracePropagationTargets;
260+
const addedHeaders = shouldPropagateTraceForUrl(url, tracePropagationTargets, this._propagationDecisionMap)
261+
? getTraceData()
262+
: undefined;
263+
264+
if (!addedHeaders) {
265+
return;
266+
}
267+
268+
const { 'sentry-trace': sentryTrace, baggage } = addedHeaders;
269+
270+
// We do not want to overwrite existing header here, if it was already set
271+
if (sentryTrace && !request.getHeader('sentry-trace')) {
272+
request.setHeader('sentry-trace', sentryTrace);
273+
logger.log(INSTRUMENTATION_NAME, 'Added sentry-trace header to outgoing request');
274+
}
275+
276+
if (baggage) {
277+
// For baggage, we make sure to merge this into a possibly existing header
278+
const newBaggage = mergeBaggageHeaders(request.getHeader('baggage'), baggage);
279+
if (newBaggage) {
280+
request.setHeader('baggage', newBaggage);
281+
}
282+
}
283+
}
284+
212285
/**
213286
* Patch a server.emit function to handle process isolation for incoming requests.
214287
* This will only patch the emit function if it was not already patched.

0 commit comments

Comments
 (0)