From e35e87b08087468fffeffa25c93c626ae24621d5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 28 Mar 2024 14:53:17 +0100 Subject: [PATCH 1/2] test(node): Node HTTP instrumentation integration tests --- .../suites/tracing/spans/scenario.ts | 6 +- .../suites/tracing/spans/test.ts | 76 ++++++++++-------- .../tracePropagationTargets/scenario.ts | 10 ++- .../tracing/tracePropagationTargets/test.ts | 77 ++++++++----------- .../node-integration-tests/utils/runner.ts | 9 ++- .../node-integration-tests/utils/server.ts | 37 +++++++++ 6 files changed, 132 insertions(+), 83 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts index 333ed6f3f605..8af365587b06 100644 --- a/dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts @@ -1,17 +1,19 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; import * as Sentry from '@sentry/node'; Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', tracesSampleRate: 1.0, + transport: loggingTransport, }); import * as http from 'http'; // eslint-disable-next-line @typescript-eslint/no-floating-promises Sentry.startSpan({ name: 'test_transaction' }, async () => { - http.get('http://match-this-url.com/api/v0'); - http.get('http://match-this-url.com/api/v1'); + http.get(`${process.env.SERVER_URL}/api/v0`); + http.get(`${process.env.SERVER_URL}/api/v1`); // Give it a tick to resolve... await new Promise(resolve => setTimeout(resolve, 100)); diff --git a/dev-packages/node-integration-tests/suites/tracing/spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/spans/test.ts index df63ff25a4e7..1fc99604ac33 100644 --- a/dev-packages/node-integration-tests/suites/tracing/spans/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/spans/test.ts @@ -1,36 +1,48 @@ -import nock from 'nock'; +import { createRunner } from '../../../utils/runner'; +import { createHeaderTestServer } from '../../../utils/server'; -import { TestEnv, assertSentryTransaction } from '../../../utils'; +test('should capture spans for outgoing http requests', done => { + expect.assertions(3); -// TODO: Convert this test to the new test runner -// eslint-disable-next-line jest/no-disabled-tests -test.skip('should capture spans for outgoing http requests', async () => { - const match1 = nock('http://match-this-url.com').get('/api/v0').reply(200); - const match2 = nock('http://match-this-url.com').get('/api/v1').reply(200); - - const env = await TestEnv.init(__dirname); - const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' }); - - expect(match1.isDone()).toBe(true); - expect(match2.isDone()).toBe(true); - - expect(envelope).toHaveLength(3); - - assertSentryTransaction(envelope[2], { - transaction: 'test_transaction', - spans: [ - { - description: 'GET http://match-this-url.com/api/v0', - op: 'http.client', - origin: 'auto.http.node.http', - status: 'ok', - }, - { - description: 'GET http://match-this-url.com/api/v1', - op: 'http.client', - origin: 'auto.http.node.http', - status: 'ok', + createHeaderTestServer(done) + .get('/api/v0', () => { + // Just ensure we're called + expect(true).toBe(true); + }) + .get( + '/api/v1', + () => { + // Just ensure we're called + expect(true).toBe(true); }, - ], - }); + 404, + ) + .start() + .then(SERVER_URL => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .expect({ + transaction: { + transaction: 'test_transaction', + spans: expect.arrayContaining([ + expect.objectContaining({ + description: expect.stringMatching(/GET .*\/api\/v0/), + op: 'http.client', + origin: 'auto.http.otel.http', + status: 'ok', + }), + expect.objectContaining({ + description: expect.stringMatching(/GET .*\/api\/v1/), + op: 'http.client', + origin: 'auto.http.otel.http', + status: 'unknown_error', + data: expect.objectContaining({ + 'http.response.status_code': 404, + }), + }), + ]), + }, + }) + .start(done); + }); }); diff --git a/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/scenario.ts index 0947b37aef5a..c346b617b9e6 100644 --- a/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/scenario.ts +++ b/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/scenario.ts @@ -1,3 +1,4 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; import * as Sentry from '@sentry/node'; Sentry.init({ @@ -6,13 +7,14 @@ Sentry.init({ tracesSampleRate: 1.0, tracePropagationTargets: [/\/v0/, 'v1'], integrations: [], + transport: loggingTransport, }); import * as http from 'http'; Sentry.startSpan({ name: 'test_span' }, () => { - http.get('http://match-this-url.com/api/v0'); - http.get('http://match-this-url.com/api/v1'); - http.get('http://dont-match-this-url.com/api/v2'); - http.get('http://dont-match-this-url.com/api/v3'); + http.get(`${process.env.SERVER_URL}/api/v0`); + http.get(`${process.env.SERVER_URL}/api/v1`); + http.get(`${process.env.SERVER_URL}/api/v2`); + http.get(`${process.env.SERVER_URL}/api/v3`); }); diff --git a/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts b/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts index f607454e0b65..3af64805ebe2 100644 --- a/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts @@ -1,44 +1,35 @@ -import nock from 'nock'; - -import { TestEnv, runScenario } from '../../../utils'; - -// TODO: Convert this test to the new test runner -// eslint-disable-next-line jest/no-disabled-tests -test.skip('HttpIntegration should instrument correct requests when tracePropagationTargets option is provided', async () => { - const match1 = nock('http://match-this-url.com') - .get('/api/v0') - .matchHeader('baggage', val => typeof val === 'string') - .matchHeader('sentry-trace', val => typeof val === 'string') - .reply(200); - - const match2 = nock('http://match-this-url.com') - .get('/api/v1') - .matchHeader('baggage', val => typeof val === 'string') - .matchHeader('sentry-trace', val => typeof val === 'string') - .reply(200); - - const match3 = nock('http://dont-match-this-url.com') - .get('/api/v2') - .matchHeader('baggage', val => val === undefined) - .matchHeader('sentry-trace', val => val === undefined) - .reply(200); - - const match4 = nock('http://dont-match-this-url.com') - .get('/api/v3') - .matchHeader('baggage', val => val === undefined) - .matchHeader('sentry-trace', val => val === undefined) - .reply(200); - - const env = await TestEnv.init(__dirname); - await runScenario(env.url); - - env.server.close(); - nock.cleanAll(); - - await new Promise(resolve => env.server.close(resolve)); - - expect(match1.isDone()).toBe(true); - expect(match2.isDone()).toBe(true); - expect(match3.isDone()).toBe(true); - expect(match4.isDone()).toBe(true); +import { createRunner } from '../../../utils/runner'; +import { createHeaderTestServer } from '../../../utils/server'; + +test('HttpIntegration should instrument correct requests when tracePropagationTargets option is provided', done => { + expect.assertions(9); + + createHeaderTestServer(done) + .get('/api/v0', headers => { + expect(typeof headers['baggage']).toBe('string'); + expect(typeof headers['sentry-trace']).toBe('string'); + }) + .get('/api/v1', headers => { + expect(typeof headers['baggage']).toBe('string'); + expect(typeof headers['sentry-trace']).toBe('string'); + }) + .get('/api/v2', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + }) + .get('/api/v3', headers => { + expect(headers['baggage']).toBeUndefined(); + expect(headers['sentry-trace']).toBeUndefined(); + }) + .start() + .then(SERVER_URL => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .expect({ + transaction: { + // we're not too concerned with the actual transaction here since this is tested elsewhere + }, + }) + .start(done); + }); }); diff --git a/dev-packages/node-integration-tests/utils/runner.ts b/dev-packages/node-integration-tests/utils/runner.ts index a9f3a94d4e3d..97b6b4c6d9ce 100644 --- a/dev-packages/node-integration-tests/utils/runner.ts +++ b/dev-packages/node-integration-tests/utils/runner.ts @@ -123,6 +123,7 @@ export function createRunner(...paths: string[]) { const expectedEnvelopes: Expected[] = []; const flags: string[] = []; const ignored: EnvelopeItemType[] = []; + let withEnv: Record = {}; let withSentryServer = false; let dockerOptions: DockerOptions | undefined; let ensureNoErrorOutput = false; @@ -141,6 +142,10 @@ export function createRunner(...paths: string[]) { expectError = true; return this; }, + withEnv: function (env: Record) { + withEnv = env; + return this; + }, withFlags: function (...args: string[]) { flags.push(...args); return this; @@ -260,8 +265,8 @@ export function createRunner(...paths: string[]) { } const env = mockServerPort - ? { ...process.env, SENTRY_DSN: `http://public@localhost:${mockServerPort}/1337` } - : process.env; + ? { ...process.env, ...withEnv, SENTRY_DSN: `http://public@localhost:${mockServerPort}/1337` } + : { ...process.env, ...withEnv }; // eslint-disable-next-line no-console if (process.env.DEBUG) console.log('starting scenario', testPath, flags, env.SENTRY_DSN); diff --git a/dev-packages/node-integration-tests/utils/server.ts b/dev-packages/node-integration-tests/utils/server.ts index 01af9558f0ab..18f6d3dc5c66 100644 --- a/dev-packages/node-integration-tests/utils/server.ts +++ b/dev-packages/node-integration-tests/utils/server.ts @@ -31,3 +31,40 @@ export function createBasicSentryServer(onEnvelope: (env: Envelope) => void): Pr }); }); } + +type HeaderAssertCallback = (headers: Record) => void; + +/** Creates a test server that can be used to check headers */ +// eslint-disable-next-line @typescript-eslint/explicit-function-return-type +export function createHeaderTestServer(done: (error: unknown) => void) { + const gets: Array<[string, HeaderAssertCallback, number]> = []; + + return { + get: function (path: string, callback: HeaderAssertCallback, result = 200) { + gets.push([path, callback, result]); + return this; + }, + start: async (): Promise => { + const app = express(); + + for (const [path, callback, result] of gets) { + app.get(path, (req, res) => { + try { + callback(req.headers); + } catch (e) { + done(e); + } + + res.status(result).send(); + }); + } + + return new Promise(resolve => { + const server = app.listen(0, () => { + const address = server.address() as AddressInfo; + resolve(`http://localhost:${address.port}`); + }); + }); + }, + }; +} From f83604de9f3a8843a5c329b827b54460cd37606b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 28 Mar 2024 14:55:34 +0100 Subject: [PATCH 2/2] rename server fn --- .../node-integration-tests/suites/tracing/spans/test.ts | 4 ++-- .../suites/tracing/tracePropagationTargets/test.ts | 4 ++-- dev-packages/node-integration-tests/utils/server.ts | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/spans/test.ts index 1fc99604ac33..45d79eb6f23b 100644 --- a/dev-packages/node-integration-tests/suites/tracing/spans/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/spans/test.ts @@ -1,10 +1,10 @@ import { createRunner } from '../../../utils/runner'; -import { createHeaderTestServer } from '../../../utils/server'; +import { createTestServer } from '../../../utils/server'; test('should capture spans for outgoing http requests', done => { expect.assertions(3); - createHeaderTestServer(done) + createTestServer(done) .get('/api/v0', () => { // Just ensure we're called expect(true).toBe(true); diff --git a/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts b/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts index 3af64805ebe2..e87e9f3df1bc 100644 --- a/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/tracePropagationTargets/test.ts @@ -1,10 +1,10 @@ import { createRunner } from '../../../utils/runner'; -import { createHeaderTestServer } from '../../../utils/server'; +import { createTestServer } from '../../../utils/server'; test('HttpIntegration should instrument correct requests when tracePropagationTargets option is provided', done => { expect.assertions(9); - createHeaderTestServer(done) + createTestServer(done) .get('/api/v0', headers => { expect(typeof headers['baggage']).toBe('string'); expect(typeof headers['sentry-trace']).toBe('string'); diff --git a/dev-packages/node-integration-tests/utils/server.ts b/dev-packages/node-integration-tests/utils/server.ts index 18f6d3dc5c66..f68f1a9c80d4 100644 --- a/dev-packages/node-integration-tests/utils/server.ts +++ b/dev-packages/node-integration-tests/utils/server.ts @@ -36,7 +36,7 @@ type HeaderAssertCallback = (headers: Record void) { +export function createTestServer(done: (error: unknown) => void) { const gets: Array<[string, HeaderAssertCallback, number]> = []; return {