Skip to content

Commit 832a7bf

Browse files
mydeamydea
authored
feat(node): Ensure tracing without performance (TWP) works (#11564)
This fixes the node SDK to make tracing without performance work as expected. This has three aspects: 1. Ensure we always have an url to check against `tracePropagationTargets`, even if the span is not sampled. 2. Ensure we use the correct propagation context always 3. Ensure we actually consistently handle ougoing http.client spans without an active transaction The fix for 2. was to have a fallback behavior if we don't have a scope linked to a context yet (we'll just use the current scope then). This is needed because we won't necessarily have an assigned scope yet at the time this is called, because at this point in time this is still a floating context. This also meant removing the fallback code to look at the remote span context (because we'll always have a scope now). The fix for 3. was to move this out of the http integration and do this in the sampler instead. The fix for 1. was a bit more elaborate, because we require different things to solve the http instrumentation and the fetch instrumentation. For fetch, we leverage our sampler, which can attach trace state that even unsampled spans will have. We attach the URL as trace state which we can read in the propagator, ensuring this works even for unsampled spans. Sadly, for http this is not sufficient, because our usage of `onlyIfParentForOutgoingRequest` lead to some spans being created outside of the sampler. By removing this option (see 3) and doing stuff in the sampler instead, we can ensure that all spans for http are handled properly. This also bumps our node-fetch otel instrumentation to [1.2.0](https://github.com/gas-buddy/opentelemetry-instrumentation-fetch-node/releases/tag/v1.2.0) which fixes headers for Node 21 (tests were actually failing without this!).
1 parent 409624b commit 832a7bf

File tree

27 files changed

+761
-188
lines changed

27 files changed

+761
-188
lines changed

‎dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/server.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import { loggingTransport } from '@sentry-internal/node-integration-tests';
2+
import * as Sentry from '@sentry/node';
3+
4+
export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } };
5+
6+
Sentry.init({
7+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
8+
release: '1.0',
9+
environment: 'prod',
10+
integrations: [
11+
// TODO: This used to use the Express integration
12+
],
13+
tracesSampleRate: 1.0,
14+
transport: loggingTransport,
15+
});
16+
17+
import http from 'http';
18+
import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
19+
import cors from 'cors';
20+
import express from 'express';
21+
22+
const app = express();
23+
24+
app.use(cors());
25+
26+
app.get('/test/express', (_req, res) => {
27+
const headers = http.get('http://somewhere.not.sentry/').getHeaders();
28+
29+
// Responding with the headers outgoing request headers back to the assertions.
30+
res.send({ test_data: headers });
31+
});
32+
33+
Sentry.setupExpressErrorHandler(app);
34+
35+
startExpressServerAndSendPortToRunner(app);

‎dev-packages/node-integration-tests/suites/express/sentry-trace/trace-header-assign/test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ afterAll(() => {
77
});
88

99
test('Should assign `sentry-trace` header which sets parent trace id of an outgoing request.', async () => {
10-
const runner = createRunner(__dirname, '..', 'server.ts').start();
10+
const runner = createRunner(__dirname, 'server.ts').start();
1111

1212
const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
1313
'sentry-trace': '12312012123120121231201212312012-1121201211212012-0',

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
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+
});
11+
12+
async function run(): Promise<void> {
13+
// Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
14+
await new Promise(resolve => setTimeout(resolve, 100));
15+
await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
16+
await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
17+
await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
18+
await fetch(`${process.env.SERVER_URL}/api/v3`).then(res => res.text());
19+
20+
Sentry.captureException(new Error('foo'));
21+
}
22+
23+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
24+
run();

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
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 are correctly instrumented without tracesSampleRate', done => {
7+
expect.assertions(15);
8+
9+
createTestServer(done)
10+
.get('/api/v0', headers => {
11+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/));
12+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000');
13+
expect(headers['baggage']).toEqual(expect.any(String));
14+
expect(headers['__requestUrl']).toBeUndefined();
15+
})
16+
.get('/api/v1', headers => {
17+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/));
18+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000');
19+
expect(headers['baggage']).toEqual(expect.any(String));
20+
expect(headers['__requestUrl']).toBeUndefined();
21+
})
22+
.get('/api/v2', headers => {
23+
expect(headers['baggage']).toBeUndefined();
24+
expect(headers['sentry-trace']).toBeUndefined();
25+
expect(headers['__requestUrl']).toBeUndefined();
26+
})
27+
.get('/api/v3', headers => {
28+
expect(headers['baggage']).toBeUndefined();
29+
expect(headers['sentry-trace']).toBeUndefined();
30+
expect(headers['__requestUrl']).toBeUndefined();
31+
})
32+
.start()
33+
.then(SERVER_URL => {
34+
createRunner(__dirname, 'scenario.ts')
35+
.withEnv({ SERVER_URL })
36+
.ensureNoErrorOutput()
37+
.ignore('session', 'sessions')
38+
.expect({
39+
event: {
40+
exception: {
41+
values: [
42+
{
43+
type: 'Error',
44+
value: 'foo',
45+
},
46+
],
47+
},
48+
},
49+
})
50+
.start(done);
51+
});
52+
});
53+
});

‎dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
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+
tracesSampleRate: 1.0,
9+
integrations: [],
10+
transport: loggingTransport,
11+
});
12+
13+
async function run(): Promise<void> {
14+
// Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
15+
await new Promise(resolve => setTimeout(resolve, 100));
16+
await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
17+
await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
18+
await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
19+
await fetch(`${process.env.SERVER_URL}/api/v3`).then(res => res.text());
20+
21+
Sentry.captureException(new Error('foo'));
22+
}
23+
24+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
25+
run();

‎dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
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 sampled fetch requests without active span are correctly instrumented', done => {
7+
expect.assertions(11);
8+
9+
createTestServer(done)
10+
.get('/api/v0', headers => {
11+
expect(headers['baggage']).toEqual(expect.any(String));
12+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/));
13+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000');
14+
})
15+
.get('/api/v1', headers => {
16+
expect(headers['baggage']).toEqual(expect.any(String));
17+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/));
18+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000');
19+
})
20+
.get('/api/v2', headers => {
21+
expect(headers['baggage']).toBeUndefined();
22+
expect(headers['sentry-trace']).toBeUndefined();
23+
})
24+
.get('/api/v3', headers => {
25+
expect(headers['baggage']).toBeUndefined();
26+
expect(headers['sentry-trace']).toBeUndefined();
27+
})
28+
.start()
29+
.then(SERVER_URL => {
30+
createRunner(__dirname, 'scenario.ts')
31+
.withEnv({ SERVER_URL })
32+
.ignore('session', 'sessions')
33+
.expect({
34+
event: {
35+
exception: {
36+
values: [
37+
{
38+
type: 'Error',
39+
value: 'foo',
40+
},
41+
],
42+
},
43+
},
44+
})
45+
.start(done);
46+
});
47+
});
48+
});

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
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+
tracesSampleRate: 1.0,
8+
tracePropagationTargets: [/\/v0/, 'v1'],
9+
integrations: [],
10+
transport: loggingTransport,
11+
});
12+
13+
async function run(): Promise<void> {
14+
await Sentry.startSpan({ name: 'test_span' }, async () => {
15+
// Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
16+
await new Promise(resolve => setTimeout(resolve, 100));
17+
await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
18+
await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
19+
await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
20+
await fetch(`${process.env.SERVER_URL}/api/v3`).then(res => res.text());
21+
});
22+
}
23+
24+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
25+
run();

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
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 sampled fetch requests are correctly instrumented', done => {
7+
expect.assertions(11);
8+
9+
createTestServer(done)
10+
.get('/api/v0', headers => {
11+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/));
12+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1');
13+
expect(headers['baggage']).toEqual(expect.any(String));
14+
})
15+
.get('/api/v1', headers => {
16+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-1$/));
17+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-1');
18+
expect(headers['baggage']).toEqual(expect.any(String));
19+
})
20+
.get('/api/v2', headers => {
21+
expect(headers['baggage']).toBeUndefined();
22+
expect(headers['sentry-trace']).toBeUndefined();
23+
})
24+
.get('/api/v3', headers => {
25+
expect(headers['baggage']).toBeUndefined();
26+
expect(headers['sentry-trace']).toBeUndefined();
27+
})
28+
.start()
29+
.then(SERVER_URL => {
30+
createRunner(__dirname, 'scenario.ts')
31+
.withEnv({ SERVER_URL })
32+
.expect({
33+
transaction: {
34+
// we're not too concerned with the actual transaction here since this is tested elsewhere
35+
},
36+
})
37+
.start(done);
38+
});
39+
});
40+
});

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
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+
tracesSampleRate: 0,
9+
integrations: [],
10+
transport: loggingTransport,
11+
});
12+
13+
async function run(): Promise<void> {
14+
// Wrap in span that is not sampled
15+
await Sentry.startSpan({ name: 'outer' }, async () => {
16+
// Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
17+
await new Promise(resolve => setTimeout(resolve, 100));
18+
await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
19+
await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
20+
await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
21+
await fetch(`${process.env.SERVER_URL}/api/v3`).then(res => res.text());
22+
});
23+
24+
Sentry.captureException(new Error('foo'));
25+
}
26+
27+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
28+
run();

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
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 are correctly instrumented when not sampled', done => {
7+
expect.assertions(11);
8+
9+
createTestServer(done)
10+
.get('/api/v0', headers => {
11+
expect(headers['baggage']).toEqual(expect.any(String));
12+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/));
13+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0');
14+
})
15+
.get('/api/v1', headers => {
16+
expect(headers['baggage']).toEqual(expect.any(String));
17+
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})-0$/));
18+
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000-0');
19+
})
20+
.get('/api/v2', headers => {
21+
expect(headers['baggage']).toBeUndefined();
22+
expect(headers['sentry-trace']).toBeUndefined();
23+
})
24+
.get('/api/v3', headers => {
25+
expect(headers['baggage']).toBeUndefined();
26+
expect(headers['sentry-trace']).toBeUndefined();
27+
})
28+
.start()
29+
.then(SERVER_URL => {
30+
createRunner(__dirname, 'scenario.ts')
31+
.withEnv({ SERVER_URL })
32+
.ignore('session', 'sessions')
33+
.expect({
34+
event: {
35+
exception: {
36+
values: [
37+
{
38+
type: 'Error',
39+
value: 'foo',
40+
},
41+
],
42+
},
43+
},
44+
})
45+
.start(done);
46+
});
47+
});
48+
});

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
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+
});
11+
12+
import * as http from 'http';
13+
14+
async function run(): Promise<void> {
15+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`);
16+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v1`);
17+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`);
18+
await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`);
19+
20+
Sentry.captureException(new Error('foo'));
21+
}
22+
23+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
24+
run();
25+
26+
function makeHttpRequest(url: string): Promise<void> {
27+
return new Promise<void>(resolve => {
28+
http
29+
.request(url, httpRes => {
30+
httpRes.on('data', () => {
31+
// we don't care about data
32+
});
33+
httpRes.on('end', () => {
34+
resolve();
35+
});
36+
})
37+
.end();
38+
});
39+
}

0 commit comments

Comments
 (0)